| Message ID | 1370674687-13849-5-git-send-email-xiawenc@linux.vnet.ibm.com |
|---|---|
| State | New |
| Headers | show |
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 <xiawenc@linux.vnet.ibm.com> > Signed-off-by: Pavel Hrdina <phrdina@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(); } 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.
于 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 <xiawenc@linux.vnet.ibm.com> >> Signed-off-by: Pavel Hrdina <phrdina@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. > 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?.
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 <xiawenc@linux.vnet.ibm.com> > >>Signed-off-by: Pavel Hrdina <phrdina@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.
于 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 <xiawenc@linux.vnet.ibm.com> >>>> Signed-off-by: Pavel Hrdina <phrdina@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 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. >
On Sat, Jun 08, 2013 at 02:58:00PM +0800, Wenchao Xia wrote: > + 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(); > + } If you respin, this would be a little clearer: assert(id || name); if (id && name) { ... } else if (id) { ... } else if (name) { ... } The advantage is that the assert(3) condition is included in the error message that gets printed. Stefan
于 2013-6-11 16:26, Stefan Hajnoczi 写道: > On Sat, Jun 08, 2013 at 02:58:00PM +0800, Wenchao Xia wrote: >> + 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(); >> + } > > If you respin, this would be a little clearer: > > assert(id || name); > > if (id && name) { > ... > } else if (id) { > ... > } else if (name) { > ... > } > > The advantage is that the assert(3) condition is included in the error > message that gets printed. > > Stefan > OK, will do it in next version.
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(); + } + + g_free(sn_tab); + return ret; +} + int bdrv_can_snapshot(BlockDriverState *bs) { BlockDriver *drv = bs->drv; diff --git a/include/block/snapshot.h b/include/block/snapshot.h index eaf61f0..9d06dc1 100644 --- a/include/block/snapshot.h +++ b/include/block/snapshot.h @@ -26,6 +26,7 @@ #define SNAPSHOT_H #include "qemu-common.h" +#include "qapi/error.h" typedef struct QEMUSnapshotInfo { char id_str[128]; /* unique snapshot id */ @@ -40,6 +41,11 @@ typedef struct QEMUSnapshotInfo { int bdrv_snapshot_find(BlockDriverState *bs, QEMUSnapshotInfo *sn_info, const char *name); +bool bdrv_snapshot_find_by_id_and_name(BlockDriverState *bs, + const char *id, + const char *name, + QEMUSnapshotInfo *sn_info, + Error **errp); int bdrv_can_snapshot(BlockDriverState *bs); int bdrv_snapshot_create(BlockDriverState *bs, QEMUSnapshotInfo *sn_info);