Re: [Qemu-devel] [PATCH 4/7] block: Add "read-only" to the options QDict

[Alberto Garcia]

On Thu 15 Sep 2016 12:51:27 PM CEST, Kevin Wolf wrote:
>> @@ -707,6 +707,9 @@ static void bdrv_inherited_options(int *child_flags, 
>> QDict *child_options,
>>      qdict_copy_default(child_options, parent_options, 
>> BDRV_OPT_CACHE_DIRECT);
>>      qdict_copy_default(child_options, parent_options, 
>> BDRV_OPT_CACHE_NO_FLUSH);
>>  
>> +    /* Inherit the read-only option from the parent if it's not set */
>> +    qdict_copy_default(child_options, parent_options, BDRV_OPT_READ_ONLY);
>> +
>>      /* Our block drivers take care to send flushes and respect unmap policy,
>>       * so we can default to enable both on lower layers regardless of the
>>       * corresponding parent options. */
>
> We need another qdict_copy_default() in bdrv_temp_snapshot_options(),
> I think, so that flags and options stay consistent there.

You're right, I assumed that with read-only=on,snapshot=on the temporary
file would still be r/w.

I'll fix it.

>> +    read_only = qemu_opt_get_bool(opts, BDRV_OPT_READ_ONLY, false);
>> +
>>      /* bdrv_open() defaults to the values in bdrv_flags (for compatibility
>>       * with other callers) rather than what we want as the real defaults.
>>       * Apply the defaults here instead. */
>>      qdict_set_default_str(bs_opts, BDRV_OPT_CACHE_DIRECT, "off");
>>      qdict_set_default_str(bs_opts, BDRV_OPT_CACHE_NO_FLUSH, "off");
>> +    qdict_set_default_str(bs_opts, BDRV_OPT_READ_ONLY,
>> +                          read_only ? "on" : "off");
>
> Why do you parse read_only into the QemuOpts just to add it right back
> to bs_opts? Wouldn't it be easier to remove it from qemu_root_bds_opts
> and do a simple set_default_str("on") here? (Which is how the cache
> options work.)

Yeah, I was mirroring blockdev_init() (there it's easier to keep it in
qemu_common_drive_opts), but in this case there's no need to do it.

I'll fix it too.

Thanks!

Berto