From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:38283) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1UlEck-0004gR-Sf for qemu-devel@nongnu.org; Sat, 08 Jun 2013 04:35:56 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1UlEcV-0007vB-Ml for qemu-devel@nongnu.org; Sat, 08 Jun 2013 04:35:46 -0400 Received: from mx1.redhat.com ([209.132.183.28]:17084) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1UlEcV-0007v5-Fd for qemu-devel@nongnu.org; Sat, 08 Jun 2013 04:35:31 -0400 Date: Sat, 8 Jun 2013 16:35:28 +0800 From: Fam Zheng Message-ID: <20130608083528.GE9648@localhost.nay.redhat.com> 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> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <51B2E442.8030602@linux.vnet.ibm.com> Content-Transfer-Encoding: quoted-printable 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: Wenchao Xia Cc: kwolf@redhat.com, phrdina@redhat.com, armbru@redhat.com, qemu-devel@nongnu.org, lcapitulino@redhat.com, stefanha@redhat.com, pbonzini@redhat.com, dietmar@proxmox.com On Sat, 06/08 15:58, Wenchao Xia wrote: > =E4=BA=8E 2013-6-8 15:31, Fam Zheng =E5=86=99=E9=81=93: > >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, QEMUS= napshotInfo *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 ma= tching > >>+ * 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 fille= d, 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 =3D false; > >>+ > >>+ nb_sns =3D bdrv_snapshot_list(bs, &sn_tab); > >>+ if (nb_sns < 0) { > >>+ error_setg_errno(errp, -nb_sns, "Failed to get a snapshot li= st"); > >>+ return false; > >>+ } else if (nb_sns =3D=3D 0) { > >>+ return false; > >>+ } > >>+ > >>+ if (id && name) { > >>+ for (i =3D 0; i < nb_sns; i++) { > >>+ sn =3D &sn_tab[i]; > >>+ if (!strcmp(sn->id_str, id) && !strcmp(sn->name, name)) = { > >>+ *sn_info =3D *sn; > >>+ ret =3D true; > >>+ break; > >>+ } > >>+ } > >>+ } else if (id) { > >>+ for (i =3D 0; i < nb_sns; i++) { > >>+ sn =3D &sn_tab[i]; > >>+ if (!strcmp(sn->id_str, id)) { > >>+ *sn_info =3D *sn; > >>+ ret =3D true; > >>+ break; > >>+ } > >>+ } > >>+ } else if (name) { > >>+ for (i =3D 0; i < nb_sns; i++) { > >>+ sn =3D &sn_tab[i]; > >>+ if (!strcmp(sn->name, name)) { > >>+ *sn_info =3D *sn; > >>+ ret =3D true; > >>+ break; > >>+ } > >>+ } > >>+ } else { > >>+ /* program error */ > >>+ abort(); > >>+ } > > > >Looks duplicated. How about: > > > > if (id || name) { > > for (i =3D 0; i < nb_sns; i++) { > > sn =3D &sn_tab[i]; > > if ((!id || !strcmp(sn->id_str, id)) && > > (!name || !strcmp(sn->name, name))) { > > *sn_info =3D *sn; > > ret =3D 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 th= e 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 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. >=20 > >And why do we have to abort here? It is not completely nonsense to me = to > >return first snapshot with id =3D=3D NULL and name =3D=3D NULL. > > > Just to tip program error. An snapshot with id =3D=3D NULL and name =3D= =3D > NULL is not possible, isn't it?. OK. --=20 Fam