| Message ID | 1364903250-10429-6-git-send-email-xiawenc@linux.vnet.ibm.com |
|---|---|
| State | New |
| Headers | show |
On 04/02/2013 05:47 AM, Wenchao Xia wrote: > This patch adds function bdrv_query_snapshot_info_list(), which will > retrieve snapshot info of an image in qmp object format. The implementation > is based on the code moved from qemu-img.c with modification to fit more > for qmp based block layer API. > > Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com> > --- > block/qapi.c | 55 ++++++++++++++++++++++++++++++++++++++----------- > include/block/qapi.h | 4 ++- > qemu-img.c | 5 +++- > 3 files changed, 49 insertions(+), 15 deletions(-) > +/* > + * return 0 on success, @p_list will be set only on success, and caller need to s/need/needs/ > + * check *p_list on success. I wonder if this wording would be any better: Returns 0 on success, with *p_list either set to describe snapshot information, or NULL because there are no snapshots. Returns -1 on error, with *p_list untouched. > + */ > +int bdrv_query_snapshot_info_list(BlockDriverState *bs, > + SnapshotInfoList **p_list, > + Error **errp) > { At any rate, my only commentary was on grammar and a possible wording for a comment, while the code itself is fine from my viewpoint; so feel free to add: Reviewed-by: Eric Blake <eblake@redhat.com> > +++ b/qemu-img.c > @@ -1735,7 +1735,10 @@ static ImageInfoList *collect_image_info_list(const char *filename, > > info = g_new0(ImageInfo, 1); > bdrv_collect_image_info(bs, info, filename); > - bdrv_collect_snapshots(bs, info); > + if (!bdrv_query_snapshot_info_list(bs, &info->snapshots, NULL) && > + info->snapshots) { > + info->has_snapshots = true; > + } Hmm. info->snapshots starts life as NULL (thanks to g_new0), and is untouched on error. Since you are ignoring any errors, you technically could write: bdrv_query_snapshot_info_list(bs, &info->snapshots, NULL); if (info->snapshots) { info->has_snapshots = true; } for the same semantics. That means that as of this commit, no caller cares about the return value of bdrv_query_snapshot_info_list (they only care about whether info->snapshots was changed to non-null), so it could return void for a slightly simpler implementation. But I don't know if any later patches in the series start to care about which error was returned.
于 2013-4-3 9:17, Eric Blake 写道: > On 04/02/2013 05:47 AM, Wenchao Xia wrote: >> This patch adds function bdrv_query_snapshot_info_list(), which will >> retrieve snapshot info of an image in qmp object format. The implementation >> is based on the code moved from qemu-img.c with modification to fit more >> for qmp based block layer API. >> >> Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com> >> --- >> block/qapi.c | 55 ++++++++++++++++++++++++++++++++++++++----------- >> include/block/qapi.h | 4 ++- >> qemu-img.c | 5 +++- >> 3 files changed, 49 insertions(+), 15 deletions(-) > >> +/* >> + * return 0 on success, @p_list will be set only on success, and caller need to > > s/need/needs/ > >> + * check *p_list on success. > > I wonder if this wording would be any better: > > Returns 0 on success, with *p_list either set to describe snapshot > information, or NULL because there are no snapshots. Returns -1 on > error, with *p_list untouched. > >> + */ >> +int bdrv_query_snapshot_info_list(BlockDriverState *bs, >> + SnapshotInfoList **p_list, >> + Error **errp) >> { > > At any rate, my only commentary was on grammar and a possible wording > for a comment, while the code itself is fine from my viewpoint; so feel > free to add: > > Reviewed-by: Eric Blake <eblake@redhat.com> > >> +++ b/qemu-img.c >> @@ -1735,7 +1735,10 @@ static ImageInfoList *collect_image_info_list(const char *filename, >> >> info = g_new0(ImageInfo, 1); >> bdrv_collect_image_info(bs, info, filename); >> - bdrv_collect_snapshots(bs, info); >> + if (!bdrv_query_snapshot_info_list(bs, &info->snapshots, NULL) && >> + info->snapshots) { >> + info->has_snapshots = true; >> + } > > Hmm. info->snapshots starts life as NULL (thanks to g_new0), and is > untouched on error. Since you are ignoring any errors, you technically > could write: > > bdrv_query_snapshot_info_list(bs, &info->snapshots, NULL); > if (info->snapshots) { > info->has_snapshots = true; > } > > for the same semantics. That means that as of this commit, no caller > cares about the return value of bdrv_query_snapshot_info_list (they only > care about whether info->snapshots was changed to non-null), so it could > return void for a slightly simpler implementation. > > But I don't know if any later patches in the series start to care about > which error was returned. > Yes it would be cared in ImageInfo retrieving later. This section in qemu-img.c would be replaced later so did not check strictly the returned value.
Eric Blake <eblake@redhat.com> writes: > On 04/02/2013 05:47 AM, Wenchao Xia wrote: >> This patch adds function bdrv_query_snapshot_info_list(), which will >> retrieve snapshot info of an image in qmp object format. The implementation >> is based on the code moved from qemu-img.c with modification to fit more >> for qmp based block layer API. >> >> Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com> >> --- >> block/qapi.c | 55 ++++++++++++++++++++++++++++++++++++++----------- >> include/block/qapi.h | 4 ++- >> qemu-img.c | 5 +++- >> 3 files changed, 49 insertions(+), 15 deletions(-) > >> +/* >> + * return 0 on success, @p_list will be set only on success, and caller need to > > s/need/needs/ > >> + * check *p_list on success. > > I wonder if this wording would be any better: > > Returns 0 on success, with *p_list either set to describe snapshot > information, or NULL because there are no snapshots. Returns -1 on > error, with *p_list untouched. It actually returns -errno then, doesn't it? > >> + */ >> +int bdrv_query_snapshot_info_list(BlockDriverState *bs, >> + SnapshotInfoList **p_list, >> + Error **errp) >> { > > At any rate, my only commentary was on grammar and a possible wording > for a comment, while the code itself is fine from my viewpoint; so feel > free to add: > > Reviewed-by: Eric Blake <eblake@redhat.com> > >> +++ b/qemu-img.c >> @@ -1735,7 +1735,10 @@ static ImageInfoList >> *collect_image_info_list(const char *filename, >> >> info = g_new0(ImageInfo, 1); >> bdrv_collect_image_info(bs, info, filename); >> - bdrv_collect_snapshots(bs, info); >> + if (!bdrv_query_snapshot_info_list(bs, &info->snapshots, NULL) && >> + info->snapshots) { >> + info->has_snapshots = true; >> + } > > Hmm. info->snapshots starts life as NULL (thanks to g_new0), and is > untouched on error. Since you are ignoring any errors, you technically > could write: > > bdrv_query_snapshot_info_list(bs, &info->snapshots, NULL); > if (info->snapshots) { > info->has_snapshots = true; > } > > for the same semantics. That means that as of this commit, no caller > cares about the return value of bdrv_query_snapshot_info_list (they only > care about whether info->snapshots was changed to non-null), so it could > return void for a slightly simpler implementation. Return the list, or NULL. > But I don't know if any later patches in the series start to care about > which error was returned. Me neither :)
于 2013-4-10 23:11, Markus Armbruster 写道: > Eric Blake <eblake@redhat.com> writes: > >> On 04/02/2013 05:47 AM, Wenchao Xia wrote: >>> This patch adds function bdrv_query_snapshot_info_list(), which will >>> retrieve snapshot info of an image in qmp object format. The implementation >>> is based on the code moved from qemu-img.c with modification to fit more >>> for qmp based block layer API. >>> >>> Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com> >>> --- >>> block/qapi.c | 55 ++++++++++++++++++++++++++++++++++++++----------- >>> include/block/qapi.h | 4 ++- >>> qemu-img.c | 5 +++- >>> 3 files changed, 49 insertions(+), 15 deletions(-) >> >>> +/* >>> + * return 0 on success, @p_list will be set only on success, and caller need to >> >> s/need/needs/ >> >>> + * check *p_list on success. >> >> I wonder if this wording would be any better: >> >> Returns 0 on success, with *p_list either set to describe snapshot >> information, or NULL because there are no snapshots. Returns -1 on >> error, with *p_list untouched. > > It actually returns -errno then, doesn't it? > Yes, I forgot change the comments in the version changing, thank you for the carefully review. >> >>> + */ >>> +int bdrv_query_snapshot_info_list(BlockDriverState *bs, >>> + SnapshotInfoList **p_list, >>> + Error **errp) >>> { >> >> At any rate, my only commentary was on grammar and a possible wording >> for a comment, while the code itself is fine from my viewpoint; so feel >> free to add: >> >> Reviewed-by: Eric Blake <eblake@redhat.com> >> >>> +++ b/qemu-img.c >>> @@ -1735,7 +1735,10 @@ static ImageInfoList >>> *collect_image_info_list(const char *filename, >>> >>> info = g_new0(ImageInfo, 1); >>> bdrv_collect_image_info(bs, info, filename); >>> - bdrv_collect_snapshots(bs, info); >>> + if (!bdrv_query_snapshot_info_list(bs, &info->snapshots, NULL) && >>> + info->snapshots) { >>> + info->has_snapshots = true; >>> + } >> >> Hmm. info->snapshots starts life as NULL (thanks to g_new0), and is >> untouched on error. Since you are ignoring any errors, you technically >> could write: >> >> bdrv_query_snapshot_info_list(bs, &info->snapshots, NULL); >> if (info->snapshots) { >> info->has_snapshots = true; >> } >> >> for the same semantics. That means that as of this commit, no caller >> cares about the return value of bdrv_query_snapshot_info_list (they only >> care about whether info->snapshots was changed to non-null), so it could >> return void for a slightly simpler implementation. > > Return the list, or NULL. > >> But I don't know if any later patches in the series start to care about >> which error was returned. > > Me neither :) >
diff --git a/block/qapi.c b/block/qapi.c index e2b1b6b..03369a5 100644 --- a/block/qapi.c +++ b/block/qapi.c @@ -25,29 +25,56 @@ #include "block/qapi.h" #include "block/block_int.h" -void bdrv_collect_snapshots(BlockDriverState *bs , ImageInfo *info) +/* + * return 0 on success, @p_list will be set only on success, and caller need to + * check *p_list on success. + */ +int bdrv_query_snapshot_info_list(BlockDriverState *bs, + SnapshotInfoList **p_list, + Error **errp) { int i, sn_count; QEMUSnapshotInfo *sn_tab = NULL; - SnapshotInfoList *info_list, *cur_item = NULL; + SnapshotInfoList *info_list, *cur_item = NULL, *head = NULL; + SnapshotInfo *info; + sn_count = bdrv_snapshot_list(bs, &sn_tab); + if (sn_count < 0) { + const char *dev = bdrv_get_device_name(bs); + switch (sn_count) { + case -ENOMEDIUM: + error_setg(errp, "Device '%s' is not inserted", dev); + break; + case -ENOTSUP: + error_setg(errp, + "Device '%s' does not support internal snapshots", + dev); + break; + default: + error_setg_errno(errp, -sn_count, + "Can't list snapshots of device '%s'", dev); + break; + } + return sn_count; + } for (i = 0; i < sn_count; i++) { - info->has_snapshots = true; - info_list = g_new0(SnapshotInfoList, 1); - info_list->value = g_new0(SnapshotInfo, 1); - info_list->value->id = g_strdup(sn_tab[i].id_str); - info_list->value->name = g_strdup(sn_tab[i].name); - info_list->value->vm_state_size = sn_tab[i].vm_state_size; - info_list->value->date_sec = sn_tab[i].date_sec; - info_list->value->date_nsec = sn_tab[i].date_nsec; - info_list->value->vm_clock_sec = sn_tab[i].vm_clock_nsec / 1000000000; - info_list->value->vm_clock_nsec = sn_tab[i].vm_clock_nsec % 1000000000; + info = g_new0(SnapshotInfo, 1); + info->id = g_strdup(sn_tab[i].id_str); + info->name = g_strdup(sn_tab[i].name); + info->vm_state_size = sn_tab[i].vm_state_size; + info->date_sec = sn_tab[i].date_sec; + info->date_nsec = sn_tab[i].date_nsec; + info->vm_clock_sec = sn_tab[i].vm_clock_nsec / 1000000000; + info->vm_clock_nsec = sn_tab[i].vm_clock_nsec % 1000000000; + + info_list = g_new0(SnapshotInfoList, 1); + info_list->value = info; /* XXX: waiting for the qapi to support qemu-queue.h types */ if (!cur_item) { - info->snapshots = cur_item = info_list; + head = cur_item = info_list; } else { cur_item->next = info_list; cur_item = info_list; @@ -56,6 +83,8 @@ void bdrv_collect_snapshots(BlockDriverState *bs , ImageInfo *info) } g_free(sn_tab); + *p_list = head; + return 0; } void bdrv_collect_image_info(BlockDriverState *bs, diff --git a/include/block/qapi.h b/include/block/qapi.h index 4586578..91dc41b 100644 --- a/include/block/qapi.h +++ b/include/block/qapi.h @@ -28,7 +28,9 @@ #include "qapi-types.h" #include "block/block.h" -void bdrv_collect_snapshots(BlockDriverState *bs , ImageInfo *info); +int bdrv_query_snapshot_info_list(BlockDriverState *bs, + SnapshotInfoList **p_list, + Error **errp); void bdrv_collect_image_info(BlockDriverState *bs, ImageInfo *info, const char *filename); diff --git a/qemu-img.c b/qemu-img.c index a020ccc..51043ef 100644 --- a/qemu-img.c +++ b/qemu-img.c @@ -1735,7 +1735,10 @@ static ImageInfoList *collect_image_info_list(const char *filename, info = g_new0(ImageInfo, 1); bdrv_collect_image_info(bs, info, filename); - bdrv_collect_snapshots(bs, info); + if (!bdrv_query_snapshot_info_list(bs, &info->snapshots, NULL) && + info->snapshots) { + info->has_snapshots = true; + } elem = g_new0(ImageInfoList, 1); elem->value = info;
This patch adds function bdrv_query_snapshot_info_list(), which will retrieve snapshot info of an image in qmp object format. The implementation is based on the code moved from qemu-img.c with modification to fit more for qmp based block layer API. Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com> --- block/qapi.c | 55 ++++++++++++++++++++++++++++++++++++++----------- include/block/qapi.h | 4 ++- qemu-img.c | 5 +++- 3 files changed, 49 insertions(+), 15 deletions(-)