From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:37186) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1VjVoi-00010f-Qx for qemu-devel@nongnu.org; Thu, 21 Nov 2013 10:05:23 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1VjVoc-0007be-O9 for qemu-devel@nongnu.org; Thu, 21 Nov 2013 10:05:16 -0500 Received: from mx1.redhat.com ([209.132.183.28]:50475) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1VjVoc-0007bF-F4 for qemu-devel@nongnu.org; Thu, 21 Nov 2013 10:05:10 -0500 Date: Thu, 21 Nov 2013 16:05:05 +0100 From: Kevin Wolf Message-ID: <20131121150505.GI3454@dhcp-200-207.str.redhat.com> References: <1384875448-13115-1-git-send-email-kwolf@redhat.com> <1384875448-13115-2-git-send-email-kwolf@redhat.com> <20131121143309.GA17336@stefanha-thinkpad.redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20131121143309.GA17336@stefanha-thinkpad.redhat.com> Subject: Re: [Qemu-devel] [PATCH 1/2] block: Enable BDRV_O_SNAPSHOT with driver-specific options List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Stefan Hajnoczi Cc: qemu-devel@nongnu.org, stefanha@redhat.com Am 21.11.2013 um 15:33 hat Stefan Hajnoczi geschrieben: > On Tue, Nov 19, 2013 at 04:37:27PM +0100, Kevin Wolf wrote: > > @@ -1114,6 +1093,24 @@ int bdrv_open(BlockDriverState *bs, const char *filename, QDict *options, > > goto fail; > > } > > > > + /* Prepare a new options QDict for the temporary file, where user > > + * options refer to the backing file */ > > + if (!options) { > > + options = qdict_new(); > > + } > > You can drop this because options is never NULL: > > options = qdict_clone_shallow(options); > > /* For snapshot=on, create a temporary qcow2 overlay */ > if (flags & BDRV_O_SNAPSHOT) { > ... You're right, I'll remove it in v2. > > + if (filename) { > > + qdict_put(options, "file.filename", qstring_from_str(filename)); > > + } > > + if (drv) { > > + qdict_put(options, "driver", qstring_from_str(drv->format_name)); > > + } > > + > > + snapshot_options = qdict_new(); > > + qdict_put(snapshot_options, "backing", options); > > + qdict_flatten(snapshot_options); > > + > > + options = snapshot_options; > > One thing I'm not sure about after these operations have been performed: > > bs->options = options; > ... > if (flags & BDRV_O_SNAPSHOT) { > ... > > So bs->options does not reflect what we ended up with in the > BDRV_O_SNAPSHOT case. > > But git grep -- '->options' shows no users of this field. Therefore it > won't cause a problem yet. But can you explain what's going on here? > Either we should keep bs->options up-to-date or we should drop the > field. I think bs->options was meant to be used in cases where we reopen a BDS. If it's currently unused, I guess this means we have some bugs. :-) Should bs->options be the original options passed by the user or the ones modified by BDRV_O_SNAPSHOT? I'm not completely sure, but I think the modified ones make more sense; it would also make it easier to move the whole BDRV_O_SNAPSHOT logic out to drive_init (which I plan to do in a next step). Kevin