From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:54784) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1TvKcX-0005dD-NK for qemu-devel@nongnu.org; Tue, 15 Jan 2013 23:29:03 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1TvKcW-0005NU-B1 for qemu-devel@nongnu.org; Tue, 15 Jan 2013 23:29:01 -0500 Received: from e28smtp05.in.ibm.com ([122.248.162.5]:43579) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1TvKcV-0005HP-N8 for qemu-devel@nongnu.org; Tue, 15 Jan 2013 23:29:00 -0500 Received: from /spool/local by e28smtp05.in.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted for from ; Wed, 16 Jan 2013 09:57:45 +0530 Received: from d28relay04.in.ibm.com (d28relay04.in.ibm.com [9.184.220.61]) by d28dlp01.in.ibm.com (Postfix) with ESMTP id D11BFE004C for ; Wed, 16 Jan 2013 09:59:11 +0530 (IST) Received: from d28av03.in.ibm.com (d28av03.in.ibm.com [9.184.220.65]) by d28relay04.in.ibm.com (8.13.8/8.13.8/NCO v10.0) with ESMTP id r0G4Smqf16449732 for ; Wed, 16 Jan 2013 09:58:49 +0530 Received: from d28av03.in.ibm.com (loopback [127.0.0.1]) by d28av03.in.ibm.com (8.14.4/8.13.1/NCO v10.0 AVout) with ESMTP id r0G4Snta010455 for ; Wed, 16 Jan 2013 15:28:50 +1100 Message-ID: <50F62C7D.5000008@linux.vnet.ibm.com> Date: Wed, 16 Jan 2013 12:28:45 +0800 From: Wenchao Xia 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> <50F52E41.90700@linux.vnet.ibm.com> <50F598A2.9070903@redhat.com> In-Reply-To: <50F598A2.9070903@redhat.com> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit 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: Eric Blake Cc: aliguori@us.ibm.com, phrdina@redhat.com, stefanha@gmail.com, qemu-devel@nongnu.org, lcapitulino@redhat.com, pbonzini@redhat.com, armbru@redhat.com 于 2013-1-16 1:57, Eric Blake 写道: > On 01/15/2013 03:24 AM, Wenchao Xia wrote: > >>>> >>>> +int bdrv_snapshot_find(BlockDriverState *bs, QEMUSnapshotInfo *sn_info, >>>> + const char *name) > >>>> + if (!strcmp(sn->id_str, name) || !strcmp(sn->name, name)) { >>> > >>> >>> 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, > >> how about: >> /* if id is not NULL, try find it with id, if not exist, return NULL >> * if id is NULL and name is not NULL, try find it with name. >> */ if id and name is NULL, direct return fail. >> int bdrv_snapshot_find(BlockDriverState *bs, QEMUSnapshotInfo *sn_info, >> const char *id, const char *name) > > That would be pushing the burden onto the callers to decide whether they > are doing an id lookup, a name lookup, or both. In QMP terms, that > means that your QMP command to delete a snapshot would now need an > additional optional argument to decide whether the associated name is > only an id, only a name, or can match either. But I'm not sure you want > that. > > What I was trying to get at is that given a single string "2", it does > seem nicer to do both an id and a name lookup, and return the first hit; > you just need to document that ids take preference over names (and thus, > naming a snapshot "2" may make the snapshot become invisible by name, > but not by id, if a later snapshot creation causes id 2 to be used). > Then your QMP command for deleting a snapshot no longer needs to care > whether "2" is an id or a name, just whether it matches. > OK, I guess following is better and clear: /* Try to find the snapshot with @id and @name, @id have higher * priority in searching. Both @id and @name can be NULL. */ int bdrv_snapshot_find(BlockDriverState *bs, QEMUSnapshotInfo *sn_info, const char *id, const char *name) Then caller at hmp/qmp layer can decide how to use it. For info of snapshot retrieving in this serial, things are simple. for create/delete of snapshot in future, I think a check with id_wellformed() and an interface like the proposal from Pavel is needed. http://lists.gnu.org/archive/html/qemu-devel/2013-01/msg01551.html > Hmm, while typing this, I thought of another snag. Suppose you have a > VM with two disks, but where only the first disk previously had a > snapshot with id 1. If I create a new snapshot across both disks, does > that mean disk 1 gets id 2 while disk 2 gets id 1, or do both disks get > id 2, even though that means disk 2 skips over id 1? As long as the > snapshot is named, you can refer to the name to get the same snapshot > across both disks, regardless of what id it has. But if the name is > numeric, and id takes preference over name when doing a lookup, we could > get ourselves into the situation where a snapshot created with name "2" > can eventually never be restored in one piece, because the individual > disks have different ids for the same snapshot name. So maybe we DO > need a way after all for QMP to specify whether a name lookup is for id, > name, or both. > -- Best Regards Wenchao Xia