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