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

[Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>]

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