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

[Kevin Wolf]

Am 12.09.2016 um 15:48 hat Alberto Garcia geschrieben:
> 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.

Yes, that too.

> > 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.

Ah, you're right. Somehow I thought that it takes the QDict rather than
the QemuOpts. But qemu_opts_absorb_qdict() should be fine with
additional QDict entries that can't be parsed into the QemuOpts, it
should just leave them where they are. That's the same as would happen
with driver-specific options.

Or if that doesn't work for some reason, extracting file.* earlier
should indeed be possible, I guess. You just need to make sure that you
check at the end that it's really empty if it wasn't used.

Anyway, bdrv_open_inherit() is a real mess. :-(

> >> 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.

Makes sense.

Kevin