Re: [PATCH 3/3] block: drop BlockBackendRootState::read_only

[Vladimir Sementsov-Ogievskiy]

27.05.2021 17:46, Kevin Wolf wrote:
> Am 26.05.2021 um 23:15 hat Vladimir Sementsov-Ogievskiy geschrieben:
> > Instead of keeping additional boolean field, let's store the
> > information in BDRV_O_RDWR bit of BlockBackendRootState::open_flags.
> >
> > Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>
> > diff --git a/blockdev.c b/blockdev.c
> > index 834c2304a1..c36884c691 100644
> > --- a/blockdev.c
> > +++ b/blockdev.c
> > @@ -576,6 +576,9 @@ static BlockBackend *blockdev_init(const char *file, QDict 
> > *bs_opts,
> >       }
> >   
> >       read_only = qemu_opt_get_bool(opts, BDRV_OPT_READ_ONLY, false);
> > +    if (!read_only) {
> > +        bdrv_flags |= BDRV_O_RDWR;
> > +    }
> >   
> >       /* init */
> >       if ((!file || !*file) && !qdict_size(bs_opts)) {
> > @@ -584,7 +587,6 @@ static BlockBackend *blockdev_init(const char *file, QDict 
> > *bs_opts,
> >           blk = blk_new(qemu_get_aio_context(), 0, BLK_PERM_ALL);
> >           blk_rs = blk_get_root_state(blk);
> >           blk_rs->open_flags    = bdrv_flags;
> > -        blk_rs->read_only     = read_only;
> >           blk_rs->detect_zeroes = detect_zeroes;
> >   
> >           qobject_unref(bs_opts);
>
> That we're now using different flags for blk_new_open() looked a bit
> suspicious, though the new state intuitively makes more sense than the
> old one.
>
> Anyway, this patch breaks ahci-test:
>
> # starting QEMU: exec build/x86_64-softmmu/qemu-system-x86_64 -qtest 
> unix:/tmp/qtest-54104.sock -qtest-log /dev/null -chardev 
> socket,path=/tmp/qtest-54104.qmp,id=char0 -mon chardev=char0,mode=control 
> -display none -drive 
> file=/tmp/qtest.mGtxeg,if=none,id=drive0,cache=writeback,rerror=stop,werror=stop,format=raw
>  -M q35 -device ide-hd,drive=drive0 -incoming unix:/tmp/qtest-migration.XVRZZi 
> -accel qtest
> qemu-system-x86_64: -drive 
> file=/tmp/qtest.mGtxeg,if=none,id=drive0,cache=writeback,rerror=stop,werror=stop,format=raw:
>  Block node is read-only

Interesting (me again not running make check, sorry for this :\. And that's 
fixed by

diff --git a/blockdev.c b/blockdev.c
index c36884c691..2f3f9bc746 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -609,7 +609,7 @@ static BlockBackend *blockdev_init(const char *file, QDict 
*bs_opts,
             bdrv_flags |= BDRV_O_INACTIVE;
         }
 
-        blk = blk_new_open(file, NULL, bs_opts, bdrv_flags, errp);
+        blk = blk_new_open(file, NULL, bs_opts, bdrv_flags & ~BDRV_O_RDWR, 
errp);
         if (!blk) {
             goto err_no_bs_opts;
         }


so to make a RW node we should not pass both option and flag ??

Most probably that's because in blk_new_open we do

        if (flags & BDRV_O_RDWR) {
            perm |= BLK_PERM_WRITE;
        }


Probably this should be refactored somehow, as it's strange that we tread 
read-only option and O_RDWR flags in a different ways.. Still not here. I'll 
resend so that this patch modifies only BlockBackendRootState, not the logic of 
blk_new_open() call.

--
Best regards,
Vladimir