Re: [Qemu-block] bdrv_reopen() and backing images

[Alberto Garcia]

On Fri 09 Sep 2016 07:17:22 PM CEST, Kevin Wolf wrote:

>> So the other alternative is to add "read-only" as an option and keep
>> it in sync with the BDRV_O_RDWR flag. This is similar to what commit
>> 91a097e7478940483 did with the cache options.
>> 
>> There's a problem here, mostly related to the bdrv_open_inherit()
>> code. The goal of this change is to inherit the "read-only" option
>> only if it hasn't been explicitly set, e.g. with
>> qdict_set_default_str().  However that means that we don't know the
>> new value unless we parse the QDict (scenario 4 above), and if we
>> don't do it the flags and options will be (temporarily) out of
>> sync. I think we can deal with that, and and that's what I'm trying
>> at the moment. It's significantly less code, although I think it
>> requires moving things from bdrv_open_inherit() to bdrv_open_common()
>> (so we can use the QemuOpts object).
>
> Hm, is it that much code that must be moved, actually?
>
> Very early in bdrv_open_inherit() we call bdrv_fill_options(), which
> basically converts everything into the QDict form. Specifically, for
> flag we call update_options_from_flags(), so if you add read-only there,
> the flags are repesented in the QDict (but not the other way around).

Right, but we need to do it the other way around. bdrv_backing_options()
clears BDRV_O_RDWR unconditionally, but what we need to do is not to
touch that flag and use qdict_set_default_str("read-only=on") instead.

> At the moment, we call update_flags_from_options() for the other
> direction only in bdrv_open_common(), but if we move that earlier (or
> maybe just do it a second time instead of moving, not sure if we make
> later changes to the QDict that require another call where we currently
> do it), would that break anything? We could perhaps stick it right below
> the update_options_from_flags() call in bdrv_fill_options().

We cannot because at that point the QDict still has all the "file.*"
options, so creating the QemuOpts fails. Maybe we can extract those from
the QDict first... I'll give it a try.

>> I'm also wondering: if the only reason why we need to do all this is
>> to distinguish the flags that were set explicitly from the ones that
>> were inherited, isn't it simpler and more efficient to use a mask
>> instead?
>> 
>>     struct BlockDriverState {
>>         /* ... */
>>         int open_flags;
>>         int open_flags_mask;
>>         /* ... */
>>     };
>> 
>> I haven't explored this possibility much yet, but intuitively it
>> doesn't look bad. It would even allow us to tell bdrv_reopen() which
>> flags we want to alter and which ones we don't want to touch.
>
> There are actually multiple (even though related) reasons.
>
> The first is consistency, and having the QDict as the single
> authoritative source for options just makes the interfaces simpler.
> We're not there yet and block drivers still get flags passed, but I'd
> hope to remove that sooner or later.
>
> The second one is that the QDict is an exact mapping of the blockdev-add
> QAPI structure. When the request comes from QMP, you don't get a legacy
> filename string or flag, but just the options. This is the primary
> interface to support, and other (internal and external) interfaces
> should map to it if possible.
>
> The third one is that in the QDict, we support setting the option for
> child nodes (e.g. backing.file.read-only=on). This can't be done with
> flags.

Good point.

> So it's pretty clear to me that getting the option into the QDict is the
> way to go. Whether or not we keep it in the flags additionally is
> secondary, so if it makes our lives easier, we can do that. But the flag
> would then mainly be for checking the state (and as a shortcut for
> internal callers who don't want to create a QDict) rather than for
> the actual configuration code.

Yeah, I think in most cases we're simply checking the value of the flag,
and for them I think it's worth keeping it.

Berto