From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:59257) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Tv6Ui-0006xX-7Q for qemu-devel@nongnu.org; Tue, 15 Jan 2013 08:24:06 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Tv6Uc-0003gq-DT for qemu-devel@nongnu.org; Tue, 15 Jan 2013 08:24:00 -0500 Received: from mx1.redhat.com ([209.132.183.28]:23786) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Tv6Uc-0003ge-41 for qemu-devel@nongnu.org; Tue, 15 Jan 2013 08:23:54 -0500 Message-ID: <50F55718.8030307@redhat.com> Date: Tue, 15 Jan 2013 14:18:16 +0100 From: Pavel Hrdina MIME-Version: 1.0 References: <1358147387-8221-1-git-send-email-xiawenc@linux.vnet.ibm.com> <1358147387-8221-8-git-send-email-xiawenc@linux.vnet.ibm.com> <50F4973F.4090608@redhat.com> <87fw22hcyw.fsf@blackfin.pond.sub.org> In-Reply-To: <87fw22hcyw.fsf@blackfin.pond.sub.org> Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH V3 07/11] block: export function bdrv_find_snapshot() List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Markus Armbruster Cc: aliguori@us.ibm.com, stefanha@gmail.com, qemu-devel@nongnu.org, lcapitulino@redhat.com, pbonzini@redhat.com, Wenchao Xia On 01/15/2013 01:01 PM, Markus Armbruster wrote: > Eric Blake writes: > >> On 01/14/2013 12:09 AM, Wenchao Xia wrote: >>> This patch move it from savevm.c to block.c and export it. >>> >>> Signed-off-by: Wenchao Xia >>> --- >>> block.c | 23 +++++++++++++++++++++++ >>> include/block/block.h | 2 ++ >>> savevm.c | 22 ---------------------- >>> 3 files changed, 25 insertions(+), 22 deletions(-) >>> >>> diff --git a/block.c b/block.c >>> index 8192d8e..b7d2f03 100644 >>> --- a/block.c >>> +++ b/block.c >>> @@ -3351,6 +3351,29 @@ int bdrv_snapshot_load_tmp(BlockDriverState *bs, >>> return -ENOTSUP; >>> } >>> >>> +int bdrv_snapshot_find(BlockDriverState *bs, QEMUSnapshotInfo *sn_info, >>> + const char *name) >>> +{ >>> + QEMUSnapshotInfo *sn_tab, *sn; >>> + int nb_sns, i, ret; >>> + >>> + ret = -ENOENT; >>> + nb_sns = bdrv_snapshot_list(bs, &sn_tab); >>> + if (nb_sns < 0) { >>> + return ret; >>> + } >>> + for (i = 0; i < nb_sns; i++) { >>> + sn = &sn_tab[i]; >>> + if (!strcmp(sn->id_str, name) || !strcmp(sn->name, name)) { >> >> It is possible (albeit probably stupid) to create a qcow2 file where >> snapshot names are merely numeric strings. In fact, just to see what >> would happen, I once[1] created a file where: >> >> snapshot id '1' was named '2' >> snapshot id '2' was named 'foo' >> >> This code comparison favors ids over names; so if I request to delvm 2, >> I end up removing the second snapshot, not the first. This is okay, but >> probably worth documenting, and probably worth making sure that all code >> that looks up a snapshot by name or id goes through this function so >> that we get the same behavior everywhere. My experiment was done >> several months ago, but my recollection was that at the time, there was >> an inconsistency where 'qemu-img snapshot' picked a different snapshot >> for the request of '2' than the online 'delvm' monitor command of qemu; >> making it unsafe to rely on either behavior in that version of qemu >> source code. >> >> [1]https://bugzilla.redhat.com/show_bug.cgi?id=733143 > > In QemuOpts, we restricted IDs to [[:alpha:]][[:alnum:]-._]*, see > id_wellformed(). I'd recommend the same for new interfaces. But this > is an old one, and we shouldn't make existing snapshots inaccessible > just because their names were chosen unwisely. > There is my proposal for handling snapshots id and name. http://lists.gnu.org/archive/html/qemu-devel/2013-01/msg01551.html In general, we will have two options for operation with snapshots, an 'id' option and a 'name' option. You could choose one of them or both to specify which snapshot you want to load/delete/create/modify. This behaviour should be the same for online and offline snapshots even for vm-snapshots or block-snapshots. With these modifications old snapshots should also works. Pavel