From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:41132) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Zm8K5-0005sX-OS for qemu-devel@nongnu.org; Tue, 13 Oct 2015 18:45:34 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Zm8K4-0001vF-7e for qemu-devel@nongnu.org; Tue, 13 Oct 2015 18:45:33 -0400 Date: Tue, 13 Oct 2015 18:45:24 -0400 From: Jeff Cody Message-ID: <20151013224524.GO11943@localhost.localdomain> References: <3cad1f87e0ee00374ee40c0c744456dc86e5b35f.1444640617.git.berto@igalia.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <3cad1f87e0ee00374ee40c0c744456dc86e5b35f.1444640617.git.berto@igalia.com> Subject: Re: [Qemu-devel] [Qemu-block] [PATCH v7 1/5] block: check for existing device IDs in external_snapshot_prepare() List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Alberto Garcia Cc: Kevin Wolf , Stefan Hajnoczi , qemu-devel@nongnu.org, qemu-block@nongnu.org, Max Reitz On Mon, Oct 12, 2015 at 12:16:13PM +0300, Alberto Garcia wrote: > The 'snapshot-node-name' parameter of blockdev-snapshot-sync allows > setting the node name of the image that is going to be created. > > Before creating the image, external_snapshot_prepare() checks that the > name is not already being used. The check is however incomplete since > it only considers existing node names, but node names must not clash > with device IDs either because they share the same namespace. > > If the user attempts to create a snapshot using the name of an > existing device for the 'snapshot-node-name' parameter the operation > will eventually fail, but only after the new image has been created. > > This patch replaces bdrv_find_node() with bdrv_lookup_bs() to extend > the check to existing device IDs, and thus detect possible name > clashes before the new image is created. > > Signed-off-by: Alberto Garcia > --- > blockdev.c | 5 +++-- > 1 file changed, 3 insertions(+), 2 deletions(-) > > diff --git a/blockdev.c b/blockdev.c > index 4731843..0898d1f 100644 > --- a/blockdev.c > +++ b/blockdev.c > @@ -1570,8 +1570,9 @@ static void external_snapshot_prepare(BlkTransactionState *common, > return; > } > > - if (has_snapshot_node_name && bdrv_find_node(snapshot_node_name)) { > - error_setg(errp, "New snapshot node name already existing"); > + if (has_snapshot_node_name && > + bdrv_lookup_bs(snapshot_node_name, snapshot_node_name, NULL)) { > + error_setg(errp, "New snapshot node name already in use"); > return; > } > > -- > 2.6.1 > > A downfall is we don't communicate back if the collision is with a device name or a node name, but I guess that is relatively minor (and probably not worth splitting this into two different calls to bdrv_lookup_bs() to differentiate). Reviewed-by: Jeff Cody