diff mbox

[V7,06/14] block: add image info query function bdrv_query_image_info()

Message ID 1361875228-15769-7-git-send-email-xiawenc@linux.vnet.ibm.com
State New
Headers show

Commit Message

Wayne Xia Feb. 26, 2013, 10:40 a.m. UTC 
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 <xiawenc@linux.vnet.ibm.com>
---
 block.c               |   38 ++++++++++++++++++++++++++++++++------
 include/block/block.h |    5 +----
 qemu-img.c            |   13 +++++--------
 3 files changed, 38 insertions(+), 18 deletions(-)

Comments

Markus Armbruster Feb. 27, 2013, 3:30 p.m. UTC | #1 
Wenchao Xia <xiawenc@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 <xiawenc@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);
Wayne Xia Feb. 28, 2013, 1:59 a.m. UTC | #2 
于 2013-2-27 23:30, Markus Armbruster 写道:
> Wenchao Xia <xiawenc@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 <xiawenc@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.
> 
  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);
>
Stefan Hajnoczi Feb. 28, 2013, 3:47 p.m. UTC | #3 
On Tue, Feb 26, 2013 at 06:40:20PM +0800, Wenchao Xia wrote:
> 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);
> +    if (info_list != NULL) {
> +        info->has_snapshots = true;
> +        info->snapshots = info_list;
> +    }
> +}

This is a pretty trivial function that is used only once.  Inlining the
code is clearer IMO.
Wayne Xia March 1, 2013, 1:44 a.m. UTC | #4 
于 2013-2-28 23:47, Stefan Hajnoczi 写道:
> On Tue, Feb 26, 2013 at 06:40:20PM +0800, Wenchao Xia wrote:
>> 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);
>> +    if (info_list != NULL) {
>> +        info->has_snapshots = true;
>> +        info->snapshots = info_list;
>> +    }
>> +}
>
> This is a pretty trivial function that is used only once.  Inlining the
> code is clearer IMO.
>
OK.
diff mbox

Patch

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);
+    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;
+}
+
 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;
         }
 
         elem = g_new0(ImageInfoList, 1);