Re: [Qemu-devel] [PATCH V7 06/14] block: add image info query function bdrv_query_image_info()

[Wenchao Xia]

于 2013-2-27 23:30, Markus Armbruster 写道:
> 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.
> 
  OK.

>> +    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.
> 
  OK.

>> +
>>   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) {
> 
  OK.

>>           }
>>   
>>           elem = g_new0(ImageInfoList, 1);
> 


-- 
Best Regards

Wenchao Xia