Re: [Qemu-devel] [PATCH 28/34] block: Introduce bs->explicit_options

[Kevin Wolf <kwolf@redhat.com>]

Am 15.05.2015 um 19:47 hat Max Reitz geschrieben:
> 3) In bdrv_open():
> The QDict is just the one given to bdrv_open().
> 
> 3a) bdrv_open() call from bdrv_append_temp_snapshot():
> "file.driver" and "file.filename" are set, and these are the only
> options in the whole QDict. Well... I'd argue that these are options
> supplied are supplied from the flags (BDRV_O_SNAPSHOT, to be exact),
> but I guess I can turn a blind eye to this case.
> [...]
> In total, 3a looks a bit fishy, but I guess it's alright.

In fact, not only the options are derived from a flag, but the whole BDS
is, so I think this is a different case anywy. Might prove rather hard
to create a working qcow2 BDS without setting any child options. :-)

Anyway, I think we'll eventually move this to drive_init(), and at that
point I'd definitely consider it provided by the user, even if he used a
syntax that doesn't make obvious what exact options he provided.

> Concluding, I can say that setting bs->explicit_options at that
> point will not result in automatically derived options being
> included there (except for 3a). A problem I do see is that as can be
> seen above, deriving this is not trivial, and keeping this the case
> isn't either. We have to make sure that bdrv_open_inherit() will
> never set any option in @options which contains a dot, neither may
> any of the functions it calls (do we need appropriate documentation
> for child_role->inherit_options()?).

Yes, we need to ensure that. But in fact, I don't think that's very
hard to maintain. On the contrary, it's just common sense that
bdrv_open_inherit() only tinkers with "local" options. Especially in the
.inherit_options() functions, which don't even know if the node has any
children, it seems unlikely that an option for children is set.

I mean, why would any of these functions even _want_ to add an option
with a dot in it?

But if you prefer, I can put a comment at the inherit_options function
pointer declaration.

> As noted above in point 1a II, @filename may be a JSON filename in
> bdrv_open_inherit(). I think these would be user-supplied options,
> so they should be put into bs->explicit_options, too. If they are
> not, 1a II is invalid and we have to make sure that none of the
> options supplied there can end up in any bs->explicit_options of any
> child BDS.

Yes, I would agree that json: objects should be treated exactly the same
as if the options were already contained in the QDict.

> Also note that above I did not check whether bs->explicit_options
> will contain all user-specified options. I only made sure that it
> doesn't contain automatically derived options. But as long as
> blkdebug doesn't absorb options like "image.filename", it should be
> fine (no options prefixed with a bdref_key matching a BDS child role
> we are still intending to open may be removed, but I don't think
> that's the case, ever).

Actually, even if blkdebug absorbed "image.filename" (which would be a
stupid thing to do because it breaks a useful option naming convention),
it would be an explicit option to blkdebug, but not to raw-posix,
because raw-posix would never even see it. So the code should do the
right thing there.

Or phrased differently: If explicit_options isn't a subset of options,
something went wrong.

> Oh, and also we have to make sure that setting
> reopen_state->bs->explicit_options does not result in derived
> options being set. It's generated from @options given to
> bdrv_reopen_queue_child(), joined with bs->explicit_options
> (induction hypothesis: bs->explicit_options is good). Is @options
> good, too? So far yes, because it's always empty (except for
> qemu-io, where it comes directly from the user).

In my book, the callers of bdrv_reopen_*() are users. ;-)

Or less sloppily: They are triggered by user actions. A guest or an NBD
client can't reopen an image.

> >@@ -1886,6 +1896,9 @@ void bdrv_reopen_commit(BDRVReopenState *reopen_state)
> >      }
> >      /* set BDS specific flags now */
> >+    QDECREF(reopen_state->bs->explicit_options);
> >+
> >+    reopen_state->bs->explicit_options   = reopen_state->explicit_options;
> >      reopen_state->bs->open_flags         = reopen_state->flags;
> >      reopen_state->bs->enable_write_cache = !!(reopen_state->flags &
> >                                                BDRV_O_CACHE_WB);
> >@@ -1909,6 +1922,8 @@ void bdrv_reopen_abort(BDRVReopenState *reopen_state)
> >      if (drv->bdrv_reopen_abort) {
> >          drv->bdrv_reopen_abort(reopen_state);
> >      }
> >+
> >+    QDECREF(reopen_state->explicit_options);
> 
> I think this must be done in bdrv_reopen_multiple(). Otherwise,
> reopen_state->explicit_options is leaked for the one BDS where
> bdrv_reopen_prepare() failed.

And actually for all of the following BDSes as well.

I'll leave it in abort for symmetry with commit, but add a QDECREF for
yet unprepared BDSes.

> What I'd like to have for a R-b: No leak of
> reopen_state->explicit_options, and an answer to the question
> whether options coming from a JSON filename should be part of
> bs->explicit_options (right now, they are for all child BDSs, but
> not for the top BDS, because bdrv_fill_options() is called after
> bs->explicit_options is set).

Oh. I guess this should be fixed so that they are considered
explicit even for the top level. But we don't want the rest of
bdrv_fill_options() to be considered explicit, so it seems I'll have
to split it.

Kevin