From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:34531) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1UlEFZ-0006EO-Ol for qemu-devel@nongnu.org; Sat, 08 Jun 2013 04:11:52 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1UlEFY-0001Zm-Lf for qemu-devel@nongnu.org; Sat, 08 Jun 2013 04:11:49 -0400 Received: from e23smtp09.au.ibm.com ([202.81.31.142]:38340) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1UlEFY-0001ZU-3j for qemu-devel@nongnu.org; Sat, 08 Jun 2013 04:11:48 -0400 Received: from /spool/local by e23smtp09.au.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted for from ; Sun, 9 Jun 2013 05:08:30 +1000 Received: from d23relay03.au.ibm.com (d23relay03.au.ibm.com [9.190.235.21]) by d23dlp01.au.ibm.com (Postfix) with ESMTP id BF16D2CE8044 for ; Sat, 8 Jun 2013 18:11:39 +1000 (EST) Received: from d23av01.au.ibm.com (d23av01.au.ibm.com [9.190.234.96]) by d23relay03.au.ibm.com (8.13.8/8.13.8/NCO v10.0) with ESMTP id r5886Nd631457330 for ; Sat, 8 Jun 2013 18:11:31 +1000 Received: from d23av01.au.ibm.com (loopback [127.0.0.1]) by d23av01.au.ibm.com (8.14.4/8.13.1/NCO v10.0 AVout) with ESMTP id r5880U77005153 for ; Sat, 8 Jun 2013 18:00:30 +1000 Message-ID: <51B2E442.8030602@linux.vnet.ibm.com> Date: Sat, 08 Jun 2013 15:58:58 +0800 From: Wenchao Xia MIME-Version: 1.0 References: <1370674687-13849-1-git-send-email-xiawenc@linux.vnet.ibm.com> <1370674687-13849-5-git-send-email-xiawenc@linux.vnet.ibm.com> <20130608073132.GA9648@localhost.nay.redhat.com> In-Reply-To: <20130608073132.GA9648@localhost.nay.redhat.com> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit Subject: Re: [Qemu-devel] [PATCH 04/11] snapshot: new function bdrv_snapshot_find_by_id_and_name() List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: qemu-devel@nongnu.org, kwolf@redhat.com, phrdina@redhat.com, armbru@redhat.com, lcapitulino@redhat.com, stefanha@redhat.com, pbonzini@redhat.com, dietmar@proxmox.com 于 2013-6-8 15:31, Fam Zheng 写道: > On Sat, 06/08 14:58, Wenchao Xia wrote: >> To make it clear about id and name in searching, add this API >> to distinguish them. Caller can choose to search by id or name, >> *errp will be set only for exception. >> >> Some code are modified based on Pavel's patch. >> >> Signed-off-by: Wenchao Xia >> Signed-off-by: Pavel Hrdina >> --- >> block/snapshot.c | 74 ++++++++++++++++++++++++++++++++++++++++++++++ >> include/block/snapshot.h | 6 ++++ >> 2 files changed, 80 insertions(+), 0 deletions(-) >> >> diff --git a/block/snapshot.c b/block/snapshot.c >> index 6c6d9de..0a9af4e 100644 >> --- a/block/snapshot.c >> +++ b/block/snapshot.c >> @@ -48,6 +48,80 @@ int bdrv_snapshot_find(BlockDriverState *bs, QEMUSnapshotInfo *sn_info, >> return ret; >> } >> >> +/** >> + * Look up an internal snapshot by @id and @name. >> + * @bs: block device to search >> + * @id: unique snapshot ID, or NULL >> + * @name: snapshot name, or NULL >> + * @sn_info: location to store information on the snapshot found >> + * @errp: location to store error, will be set only for exception >> + * >> + * This function will traverse snapshot list in @bs to search the matching >> + * one, @id and @name are the matching condition: >> + * If both @id and @name are specified, find the first one with id @id and >> + * name @name. >> + * If only @id is specified, find the first one with id @id. >> + * If only @name is specified, find the first one with name @name. >> + * if none is specified, abort(). >> + * >> + * Returns: true when a snapshot is found and @sn_info will be filled, false >> + * when error or not found. If all operation succeed but no matching one is >> + * found, @errp will NOT be set. >> + */ >> +bool bdrv_snapshot_find_by_id_and_name(BlockDriverState *bs, >> + const char *id, >> + const char *name, >> + QEMUSnapshotInfo *sn_info, >> + Error **errp) >> +{ >> + QEMUSnapshotInfo *sn_tab, *sn; >> + int nb_sns, i; >> + bool ret = false; >> + >> + nb_sns = bdrv_snapshot_list(bs, &sn_tab); >> + if (nb_sns < 0) { >> + error_setg_errno(errp, -nb_sns, "Failed to get a snapshot list"); >> + return false; >> + } else if (nb_sns == 0) { >> + return false; >> + } >> + >> + if (id && name) { >> + for (i = 0; i < nb_sns; i++) { >> + sn = &sn_tab[i]; >> + if (!strcmp(sn->id_str, id) && !strcmp(sn->name, name)) { >> + *sn_info = *sn; >> + ret = true; >> + break; >> + } >> + } >> + } else if (id) { >> + for (i = 0; i < nb_sns; i++) { >> + sn = &sn_tab[i]; >> + if (!strcmp(sn->id_str, id)) { >> + *sn_info = *sn; >> + ret = true; >> + break; >> + } >> + } >> + } else if (name) { >> + for (i = 0; i < nb_sns; i++) { >> + sn = &sn_tab[i]; >> + if (!strcmp(sn->name, name)) { >> + *sn_info = *sn; >> + ret = true; >> + break; >> + } >> + } >> + } else { >> + /* program error */ >> + abort(); >> + } > > Looks duplicated. How about: > > if (id || name) { > for (i = 0; i < nb_sns; i++) { > sn = &sn_tab[i]; > if ((!id || !strcmp(sn->id_str, id)) && > (!name || !strcmp(sn->name, name))) { > *sn_info = *sn; > ret = true; > break; > } > } > } else { > abort(); > } > Less code, but slightly slower since more "if" inside "for". I think three "for" also show more clear about judgement logic. > And why do we have to abort here? It is not completely nonsense to me to > return first snapshot with id == NULL and name == NULL. > Just to tip program error. An snapshot with id == NULL and name == NULL is not possible, isn't it?. -- Best Regards Wenchao Xia