Wenchao Xia <xiaw...@linux.vnet.ibm.com> writes: > This patch add function bdrv_query_image_info(), which will return > image info in qmp object format. The implementation code are based > on the code moved from qemu-img.c, but use block layer function to get > snapshot info. > A check with bdrv_can_read_snapshot(), was done before collecting > snapshot info. > > Signed-off-by: Wenchao Xia <xiaw...@linux.vnet.ibm.com> > --- > block.c | 38 ++++++++++++++++++++++++++++++++------ > include/block/block.h | 5 +---- > qemu-img.c | 13 +++++-------- > 3 files changed, 38 insertions(+), 18 deletions(-) > > diff --git a/block.c b/block.c > index 8d0145a..71fc9e7 100644 > --- a/block.c > +++ b/block.c > @@ -2880,15 +2880,33 @@ SnapshotInfoList > *bdrv_query_snapshot_infolist(BlockDriverState *bs, > return head; > } > > -void collect_image_info(BlockDriverState *bs, > - ImageInfo *info, > - const char *filename) > +/* collect all internal snapshot info in a image for ImageInfo */ > +static void collect_snapshots_info(BlockDriverState *bs, > + ImageInfo *info, > + Error **errp) > +{ > + SnapshotInfoList *info_list; > + > + if (!bdrv_can_read_snapshot(bs)) { > + return; > + } > + info_list = bdrv_query_snapshot_infolist(bs, NULL, NULL, errp);
Suggest to store straight into info_list->snapshots, like you did in the previous patch. > + if (info_list != NULL) { > + info->has_snapshots = true; > + info->snapshots = info_list; > + } > +} > + > +static void collect_image_info(BlockDriverState *bs, > + ImageInfo *info) > { > uint64_t total_sectors; > - char backing_filename[1024]; > + const char *backing_filename; > char backing_filename2[1024]; > BlockDriverInfo bdi; > + const char *filename; > > + filename = bs->filename; > bdrv_get_geometry(bs, &total_sectors); > > info->filename = g_strdup(filename); > @@ -2908,8 +2926,8 @@ void collect_image_info(BlockDriverState *bs, > info->dirty_flag = bdi.is_dirty; > info->has_dirty_flag = true; > } > - bdrv_get_backing_filename(bs, backing_filename, > sizeof(backing_filename)); > - if (backing_filename[0] != '\0') { > + backing_filename = bs->backing_file; > + if (backing_filename && backing_filename[0] != '\0') { > info->backing_filename = g_strdup(backing_filename); > info->has_backing_filename = true; > bdrv_get_full_backing_filename(bs, backing_filename2, > @@ -2928,6 +2946,14 @@ void collect_image_info(BlockDriverState *bs, > } > } > > +ImageInfo *bdrv_query_image_info(BlockDriverState *bs, Error **errp) > +{ > + ImageInfo *info = g_new0(ImageInfo, 1); > + collect_image_info(bs, info); > + collect_snapshots_info(bs, info, errp); > + return info; > +} Please return NULL on error. > + > BlockInfo *bdrv_query_info(BlockDriverState *bs) > { > BlockInfo *info = g_malloc0(sizeof(*info)); > diff --git a/include/block/block.h b/include/block/block.h > index 51a14f2..f033807 100644 > --- a/include/block/block.h > +++ b/include/block/block.h > @@ -326,6 +326,7 @@ SnapshotInfoList > *bdrv_query_snapshot_infolist(BlockDriverState *bs, > SnapshotFilterFunc filter, > void *opaque, > Error **errp); > +ImageInfo *bdrv_query_image_info(BlockDriverState *bs, Error **errp); > BlockInfo *bdrv_query_info(BlockDriverState *s); > BlockStats *bdrv_query_stats(const BlockDriverState *bs); > bool bdrv_can_read_snapshot(BlockDriverState *bs); > @@ -456,8 +457,4 @@ int bdrv_debug_breakpoint(BlockDriverState *bs, const > char *event, > const char *tag); > int bdrv_debug_resume(BlockDriverState *bs, const char *tag); > bool bdrv_debug_is_suspended(BlockDriverState *bs, const char *tag); > - > -void collect_image_info(BlockDriverState *bs, > - ImageInfo *info, > - const char *filename); > #endif > diff --git a/qemu-img.c b/qemu-img.c > index 1034cc5..90f4bf4 100644 > --- a/qemu-img.c > +++ b/qemu-img.c > @@ -1257,6 +1257,7 @@ static ImageInfoList *collect_image_info_list(const > char *filename, > ImageInfoList *head = NULL; > ImageInfoList **last = &head; > GHashTable *filenames; > + Error *err = NULL; > > filenames = g_hash_table_new_full(g_str_hash, str_equal_func, NULL, > NULL); > > @@ -1278,14 +1279,10 @@ static ImageInfoList *collect_image_info_list(const > char *filename, > goto err; > } > > - info = g_new0(ImageInfo, 1); > - collect_image_info(bs, info, filename); > - if (bdrv_can_read_snapshot(bs)) { > - info->snapshots = bdrv_query_snapshot_infolist(bs, NULL, > - NULL, NULL); > - if (info->snapshots) { > - info->has_snapshots = true; > - } > + info = bdrv_query_image_info(bs, &err); > + if (error_is_set(&err)) { > + bdrv_delete(bs); > + goto err; Leaks info. Easy error to make, because returning non-null on error is rather surprising. That's why I want you to return NULL then. And then this can be simplified to info = bdrv_query_image_info(bs, NULL); if (!info) { > } > > elem = g_new0(ImageInfoList, 1);