From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:39466) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bkDEl-0005MV-AZ for qemu-devel@nongnu.org; Wed, 14 Sep 2016 12:40:40 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bkDEk-0005AV-DY for qemu-devel@nongnu.org; Wed, 14 Sep 2016 12:40:39 -0400 Date: Wed, 14 Sep 2016 18:40:29 +0200 From: Kevin Wolf Message-ID: <20160914164029.GH4649@noname.redhat.com> References: <5fd02facca962ec5c64ef45a43413d4c5571d42c.1473867966.git.berto@igalia.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <5fd02facca962ec5c64ef45a43413d4c5571d42c.1473867966.git.berto@igalia.com> Subject: Re: [Qemu-devel] [PATCH 2/7] block: Set BDRV_O_ALLOW_RDWR and snapshot_options before storing the flags List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Alberto Garcia Cc: qemu-devel@nongnu.org, qemu-block@nongnu.org, Max Reitz Am 14.09.2016 um 17:52 hat Alberto Garcia geschrieben: > If an image is opened with snapshot=on, its flags are modified by > bdrv_backing_options() and then bs->open_flags is updated accordingly. > This last step is unnecessary if we calculate the new flags before > setting bs->open_flags. > > Soon we'll introduce the "read-only" option, and then we'll need to be > able to modify its value in the QDict when snapshot=on. This is more > cumbersome if bs->options is already set. This patch simplifies that. > > The code that sets BDRV_O_ALLOW_RDWR is also moved for the same > reason. > > Signed-off-by: Alberto Garcia > --- > block.c | 23 +++++++++++------------ > 1 file changed, 11 insertions(+), 12 deletions(-) > > diff --git a/block.c b/block.c > index 1c75a6f..7cae841 100644 > --- a/block.c > +++ b/block.c > @@ -1627,6 +1627,17 @@ static BlockDriverState *bdrv_open_inherit(const char *filename, > goto fail; > } > > + if (flags & BDRV_O_RDWR) { > + flags |= BDRV_O_ALLOW_RDWR; > + } > + > + if (flags & BDRV_O_SNAPSHOT) { > + snapshot_options = qdict_new(); > + bdrv_temp_snapshot_options(&snapshot_flags, snapshot_options, > + flags, options); > + bdrv_backing_options(&flags, options, flags, options); > + } > + > bs->open_flags = flags; > bs->options = options; > options = qdict_clone_shallow(options); Here I think we get different semantics now: bdrv_backing_options() only affected the cloned QDict before, and now it affects bs->options, too. Given that the flags are already updated and don't contain BDRV_O_SNAPSHOT any more, I suspect that this is a silent bug fix. If so, it might be better to split it out into a separate patch before this one, or at the very least should be mentioned in the commit message. If it's not a fix, however, it's probably a bug. :-) > @@ -1651,18 +1662,6 @@ static BlockDriverState *bdrv_open_inherit(const char *filename, > > /* Open image file without format layer */ > if ((flags & BDRV_O_PROTOCOL) == 0) { > - if (flags & BDRV_O_RDWR) { > - flags |= BDRV_O_ALLOW_RDWR; > - } > - if (flags & BDRV_O_SNAPSHOT) { > - snapshot_options = qdict_new(); > - bdrv_temp_snapshot_options(&snapshot_flags, snapshot_options, > - flags, options); > - bdrv_backing_options(&flags, options, flags, options); > - } > - > - bs->open_flags = flags; > - > file = bdrv_open_child(filename, options, "file", bs, > &child_file, true, &local_err); > if (local_err) { Kevin