[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH V7 06/14] block: add image info query function b
From: |
Markus Armbruster |
Subject: |
Re: [Qemu-devel] [PATCH V7 06/14] block: add image info query function bdrv_query_image_info() |
Date: |
Wed, 27 Feb 2013 16:30:30 +0100 |
User-agent: |
Gnus/5.13 (Gnus v5.13) Emacs/24.1 (gnu/linux) |
Wenchao Xia <address@hidden> 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 <address@hidden>
> ---
> 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);
[Qemu-devel] [PATCH V7 06/14] block: add image info query function bdrv_query_image_info(), Wenchao Xia, 2013/02/26
[Qemu-devel] [PATCH V7 07/14] block: rename bdrv_query_info() to bdrv_query_block_info(), Wenchao Xia, 2013/02/26
[Qemu-devel] [PATCH V7 08/14] qmp: add interface query-images., Wenchao Xia, 2013/02/26
[Qemu-devel] [PATCH V7 09/14] block: move bdrv_snapshot_find() to block.c, Wenchao Xia, 2013/02/26