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 <xiaw...@linux.vnet.ibm.com> > >>Signed-off-by: Pavel Hrdina <phrd...@redhat.com> > >>--- > >> 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 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. > > >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. -- Fam