From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:40306) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZrlXS-0004hS-5n for qemu-devel@nongnu.org; Thu, 29 Oct 2015 07:38:41 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ZrlXN-0005qu-T3 for qemu-devel@nongnu.org; Thu, 29 Oct 2015 07:38:38 -0400 Date: Thu, 29 Oct 2015 12:38:26 +0100 From: Kevin Wolf Message-ID: <20151029113826.GC3854@noname.redhat.com> References: <1431105726-3682-1-git-send-email-kwolf@redhat.com> <1431105726-3682-29-git-send-email-kwolf@redhat.com> <55563115.7020806@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <55563115.7020806@redhat.com> Subject: Re: [Qemu-devel] [PATCH 28/34] block: Introduce bs->explicit_options List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Max Reitz Cc: armbru@redhat.com, qemu-block@nongnu.org, qemu-devel@nongnu.org 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