From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:58182) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1UlVT7-0001Kq-P5 for qemu-devel@nongnu.org; Sat, 08 Jun 2013 22:34:59 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1UlVT3-0006GY-Dr for qemu-devel@nongnu.org; Sat, 08 Jun 2013 22:34:57 -0400 Received: from e23smtp09.au.ibm.com ([202.81.31.142]:33236) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1UlVT2-0006G7-Rx for qemu-devel@nongnu.org; Sat, 08 Jun 2013 22:34:53 -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 23:31:37 +1000 Received: from d23relay03.au.ibm.com (d23relay03.au.ibm.com [9.190.235.21]) by d23dlp02.au.ibm.com (Postfix) with ESMTP id BFF452BB0052 for ; Sun, 9 Jun 2013 12:34:48 +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 r592Yea829229066 for ; Sun, 9 Jun 2013 12:34:40 +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 r592YmAA014087 for ; Sun, 9 Jun 2013 12:34:48 +1000 Message-ID: <51B3E990.6040907@linux.vnet.ibm.com> Date: Sun, 09 Jun 2013 10:33:52 +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> <51B2E442.8030602@linux.vnet.ibm.com> <20130608083528.GE9648@localhost.nay.redhat.com> In-Reply-To: <20130608083528.GE9648@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 16:35, Fam Zheng 写道: > On Sat, 06/08 15:58, Wenchao Xia wrote: >> 于 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. > > No I don't think if-in-for or for-in-if *here* makes any meaningful > difference in performance, if we really need it fast, we'd better sort the My instinct is forbid if in for, just my custom. > list it first and binary search. And I don't see it clearer to duplicate > the same logic for three times, If I want to understand it, I need to Three cases makes work flow clear, and it is easy when I want to change a logic in one case. > compare if#1 and if#2 to get they are the same, and then compare #2 and > #3 again, just to know that the three are no different. > There is difference requiring reader think and extend it out into three cases in his mind: if ((!id || !strcmp(sn->id_str, id)) && (!name || !strcmp(sn->name, name))) It is a coding style issue, usually I don't mind to write more C lines to make things simple. Hope to get maintainer's idea. >> >>> 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?. > > OK. > -- Best Regards Wenchao Xia