Patchwork [V10,11/17] qmp: add ImageInfo in BlockDeviceInfo used by query-block

login
register
mail settings
Submitter Wayne Xia
Date March 22, 2013, 2:19 p.m.
Message ID <1363961953-13561-12-git-send-email-xiawenc@linux.vnet.ibm.com>
Download mbox | patch
Permalink /patch/230104/
State New
Headers show

Comments

Wayne Xia - March 22, 2013, 2:19 p.m.
Now image info will be retrieved as an embbed json object inside
BlockDeviceInfo, backing chain info and all related internal snapshot
info can be got in the enhanced recursive structure of ImageInfo.

Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
---
 block/qapi.c         |   39 ++++++++++++++++++++++++++--
 include/block/qapi.h |    4 ++-
 qapi-schema.json     |    5 +++-
 qmp-commands.hx      |   67 +++++++++++++++++++++++++++++++++++++++++++++++++-
 4 files changed, 109 insertions(+), 6 deletions(-)
Kevin Wolf - March 28, 2013, 9:54 a.m.
Am 22.03.2013 um 15:19 hat Wenchao Xia geschrieben:
>   Now image info will be retrieved as an embbed json object inside
> BlockDeviceInfo, backing chain info and all related internal snapshot
> info can be got in the enhanced recursive structure of ImageInfo.
> 
> Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
> ---
>  block/qapi.c         |   39 ++++++++++++++++++++++++++--
>  include/block/qapi.h |    4 ++-
>  qapi-schema.json     |    5 +++-
>  qmp-commands.hx      |   67 +++++++++++++++++++++++++++++++++++++++++++++++++-
>  4 files changed, 109 insertions(+), 6 deletions(-)
> 
> diff --git a/block/qapi.c b/block/qapi.c
> index df73f5b..9051947 100644
> --- a/block/qapi.c
> +++ b/block/qapi.c
> @@ -200,9 +200,15 @@ int bdrv_query_image_info(BlockDriverState *bs,
>      return 0;
>  }
>  
> -BlockInfo *bdrv_query_info(BlockDriverState *bs)
> +/* return 0 on success, and @p_info will be set only on success. */
> +int bdrv_query_info(BlockDriverState *bs,
> +                    BlockInfo **p_info,
> +                    Error **errp)
>  {
>      BlockInfo *info = g_malloc0(sizeof(*info));
> +    BlockDriverState *bs0;
> +    ImageInfo **p_image_info;
> +    int ret = 0;

ret is never changed, so this function always returns 0. I would suggest
to drop ret and make the function return type void.

>      info->device = g_strdup(bs->device_name);
>      info->type = g_strdup("unknown");
>      info->locked = bdrv_dev_is_medium_locked(bs);
> @@ -256,8 +262,29 @@ BlockInfo *bdrv_query_info(BlockDriverState *bs)
>              info->inserted->iops_wr =
>                             bs->io_limits.iops[BLOCK_IO_LIMIT_WRITE];
>          }
> +
> +        bs0 = bs;
> +        p_image_info = &info->inserted->image;
> +        while (1) {
> +            if (bdrv_query_image_info(bs0, p_image_info, errp)) {
> +                goto err;
> +            }
> +            if (bs0->drv && bs0->backing_hd) {
> +                bs0 = bs0->backing_hd;
> +                (*p_image_info)->has_backing_image = true;
> +                p_image_info = &((*p_image_info)->backing_image);
> +            } else {
> +                break;
> +            }
> +        }
>      }
> -    return info;
> +
> +    *p_info = info;
> +    return 0;
> +
> + err:
> +    qapi_free_BlockInfo(info);
> +    return ret;
>  }
>  
>  SnapshotInfoList *qmp_query_snapshots(Error **errp)
> @@ -284,11 +311,17 @@ BlockInfoList *qmp_query_block(Error **errp)
>  
>      while ((bs = bdrv_next(bs))) {
>          BlockInfoList *info = g_malloc0(sizeof(*info));
> -        info->value = bdrv_query_info(bs);
> +        if (bdrv_query_info(bs, &info->value, errp)) {
> +            goto err;
> +        }

Consequently, you've got the error handling wrong here, the if condition
is never true. It should look more or less like this (the syntax details
may be wrong):

Error *local_err;
bdrv_query_info(bs, &info->value, &local_err);
if (error_is_set(local_err)) {
    error_propagate(err, local_err);
    goto err;
}

Kevin
Wayne Xia - March 29, 2013, 2:35 a.m.
于 2013-3-28 17:54, Kevin Wolf 写道:
> Am 22.03.2013 um 15:19 hat Wenchao Xia geschrieben:
>>    Now image info will be retrieved as an embbed json object inside
>> BlockDeviceInfo, backing chain info and all related internal snapshot
>> info can be got in the enhanced recursive structure of ImageInfo.
>>
>> Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
>> ---
>>   block/qapi.c         |   39 ++++++++++++++++++++++++++--
>>   include/block/qapi.h |    4 ++-
>>   qapi-schema.json     |    5 +++-
>>   qmp-commands.hx      |   67 +++++++++++++++++++++++++++++++++++++++++++++++++-
>>   4 files changed, 109 insertions(+), 6 deletions(-)
>>
>> diff --git a/block/qapi.c b/block/qapi.c
>> index df73f5b..9051947 100644
>> --- a/block/qapi.c
>> +++ b/block/qapi.c
>> @@ -200,9 +200,15 @@ int bdrv_query_image_info(BlockDriverState *bs,
>>       return 0;
>>   }
>>
>> -BlockInfo *bdrv_query_info(BlockDriverState *bs)
>> +/* return 0 on success, and @p_info will be set only on success. */
>> +int bdrv_query_info(BlockDriverState *bs,
>> +                    BlockInfo **p_info,
>> +                    Error **errp)
>>   {
>>       BlockInfo *info = g_malloc0(sizeof(*info));
>> +    BlockDriverState *bs0;
>> +    ImageInfo **p_image_info;
>> +    int ret = 0;
>
> ret is never changed, so this function always returns 0. I would suggest
> to drop ret and make the function return type void.
>
   My bad, I forgot to set its value, the interface is intend to return
negative error number and errp both on fail.

>>       info->device = g_strdup(bs->device_name);
>>       info->type = g_strdup("unknown");
>>       info->locked = bdrv_dev_is_medium_locked(bs);
>> @@ -256,8 +262,29 @@ BlockInfo *bdrv_query_info(BlockDriverState *bs)
>>               info->inserted->iops_wr =
>>                              bs->io_limits.iops[BLOCK_IO_LIMIT_WRITE];
>>           }
>> +
>> +        bs0 = bs;
>> +        p_image_info = &info->inserted->image;
>> +        while (1) {
>> +            if (bdrv_query_image_info(bs0, p_image_info, errp)) {
>> +                goto err;
>> +            }
   Sorry ret is missing here, it should be:
             ret = bdrv_query_image_info(bs0, p_image_info, errp));
             if (ret) {
                 goto err;
             }
   I'll correct it.

>> +            if (bs0->drv && bs0->backing_hd) {
>> +                bs0 = bs0->backing_hd;
>> +                (*p_image_info)->has_backing_image = true;
>> +                p_image_info = &((*p_image_info)->backing_image);
>> +            } else {
>> +                break;
>> +            }
>> +        }
>>       }
>> -    return info;
>> +
>> +    *p_info = info;
>> +    return 0;
>> +
>> + err:
>> +    qapi_free_BlockInfo(info);
>> +    return ret;
>>   }
>>
>>   SnapshotInfoList *qmp_query_snapshots(Error **errp)
>> @@ -284,11 +311,17 @@ BlockInfoList *qmp_query_block(Error **errp)
>>
>>       while ((bs = bdrv_next(bs))) {
>>           BlockInfoList *info = g_malloc0(sizeof(*info));
>> -        info->value = bdrv_query_info(bs);
>> +        if (bdrv_query_info(bs, &info->value, errp)) {
>> +            goto err;
>> +        }
>
> Consequently, you've got the error handling wrong here, the if condition
> is never true. It should look more or less like this (the syntax details
> may be wrong):
>
> Error *local_err;
> bdrv_query_info(bs, &info->value, &local_err);
> if (error_is_set(local_err)) {
>      error_propagate(err, local_err);
>      goto err;
> }
>
> Kevin
>
Kevin Wolf - April 2, 2013, 8:09 a.m.
Am 29.03.2013 um 03:35 hat Wenchao Xia geschrieben:
> 于 2013-3-28 17:54, Kevin Wolf 写道:
> >Am 22.03.2013 um 15:19 hat Wenchao Xia geschrieben:
> >>   Now image info will be retrieved as an embbed json object inside
> >>BlockDeviceInfo, backing chain info and all related internal snapshot
> >>info can be got in the enhanced recursive structure of ImageInfo.
> >>
> >>Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
> >>---
> >>  block/qapi.c         |   39 ++++++++++++++++++++++++++--
> >>  include/block/qapi.h |    4 ++-
> >>  qapi-schema.json     |    5 +++-
> >>  qmp-commands.hx      |   67 +++++++++++++++++++++++++++++++++++++++++++++++++-
> >>  4 files changed, 109 insertions(+), 6 deletions(-)
> >>
> >>diff --git a/block/qapi.c b/block/qapi.c
> >>index df73f5b..9051947 100644
> >>--- a/block/qapi.c
> >>+++ b/block/qapi.c
> >>@@ -200,9 +200,15 @@ int bdrv_query_image_info(BlockDriverState *bs,
> >>      return 0;
> >>  }
> >>
> >>-BlockInfo *bdrv_query_info(BlockDriverState *bs)
> >>+/* return 0 on success, and @p_info will be set only on success. */
> >>+int bdrv_query_info(BlockDriverState *bs,
> >>+                    BlockInfo **p_info,
> >>+                    Error **errp)
> >>  {
> >>      BlockInfo *info = g_malloc0(sizeof(*info));
> >>+    BlockDriverState *bs0;
> >>+    ImageInfo **p_image_info;
> >>+    int ret = 0;
> >
> >ret is never changed, so this function always returns 0. I would suggest
> >to drop ret and make the function return type void.
> >
>   My bad, I forgot to set its value, the interface is intend to return
> negative error number and errp both on fail.

Why do you need two separate error reporting mechanisms? Shouldn't only
errp be enough?

Kevin
Wayne Xia - April 2, 2013, 8:54 a.m.
于 2013-4-2 16:09, Kevin Wolf 写道:
> Am 29.03.2013 um 03:35 hat Wenchao Xia geschrieben:
>> 于 2013-3-28 17:54, Kevin Wolf 写道:
>>> Am 22.03.2013 um 15:19 hat Wenchao Xia geschrieben:
>>>>    Now image info will be retrieved as an embbed json object inside
>>>> BlockDeviceInfo, backing chain info and all related internal snapshot
>>>> info can be got in the enhanced recursive structure of ImageInfo.
>>>>
>>>> Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
>>>> ---
>>>>   block/qapi.c         |   39 ++++++++++++++++++++++++++--
>>>>   include/block/qapi.h |    4 ++-
>>>>   qapi-schema.json     |    5 +++-
>>>>   qmp-commands.hx      |   67 +++++++++++++++++++++++++++++++++++++++++++++++++-
>>>>   4 files changed, 109 insertions(+), 6 deletions(-)
>>>>
>>>> diff --git a/block/qapi.c b/block/qapi.c
>>>> index df73f5b..9051947 100644
>>>> --- a/block/qapi.c
>>>> +++ b/block/qapi.c
>>>> @@ -200,9 +200,15 @@ int bdrv_query_image_info(BlockDriverState *bs,
>>>>       return 0;
>>>>   }
>>>>
>>>> -BlockInfo *bdrv_query_info(BlockDriverState *bs)
>>>> +/* return 0 on success, and @p_info will be set only on success. */
>>>> +int bdrv_query_info(BlockDriverState *bs,
>>>> +                    BlockInfo **p_info,
>>>> +                    Error **errp)
>>>>   {
>>>>       BlockInfo *info = g_malloc0(sizeof(*info));
>>>> +    BlockDriverState *bs0;
>>>> +    ImageInfo **p_image_info;
>>>> +    int ret = 0;
>>>
>>> ret is never changed, so this function always returns 0. I would suggest
>>> to drop ret and make the function return type void.
>>>
>>    My bad, I forgot to set its value, the interface is intend to return
>> negative error number and errp both on fail.
>
> Why do you need two separate error reporting mechanisms? Shouldn't only
> errp be enough?
>
> Kevin
>
   Returned value can tell caller what error it is, like -ENOMEDIUM.
Although it is not used by caller now, but I feel better to have it
just like

+int bdrv_query_snapshot_info_list(BlockDriverState *bs,
+                                  SnapshotInfoList **p_list,
+                                  Error **errp)

in patch 5/17.
Kevin Wolf - April 2, 2013, 9:30 a.m.
Am 02.04.2013 um 10:54 hat Wenchao Xia geschrieben:
> 于 2013-4-2 16:09, Kevin Wolf 写道:
> >Am 29.03.2013 um 03:35 hat Wenchao Xia geschrieben:
> >>于 2013-3-28 17:54, Kevin Wolf 写道:
> >>>Am 22.03.2013 um 15:19 hat Wenchao Xia geschrieben:
> >>>>   Now image info will be retrieved as an embbed json object inside
> >>>>BlockDeviceInfo, backing chain info and all related internal snapshot
> >>>>info can be got in the enhanced recursive structure of ImageInfo.
> >>>>
> >>>>Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
> >>>>---
> >>>>  block/qapi.c         |   39 ++++++++++++++++++++++++++--
> >>>>  include/block/qapi.h |    4 ++-
> >>>>  qapi-schema.json     |    5 +++-
> >>>>  qmp-commands.hx      |   67 +++++++++++++++++++++++++++++++++++++++++++++++++-
> >>>>  4 files changed, 109 insertions(+), 6 deletions(-)
> >>>>
> >>>>diff --git a/block/qapi.c b/block/qapi.c
> >>>>index df73f5b..9051947 100644
> >>>>--- a/block/qapi.c
> >>>>+++ b/block/qapi.c
> >>>>@@ -200,9 +200,15 @@ int bdrv_query_image_info(BlockDriverState *bs,
> >>>>      return 0;
> >>>>  }
> >>>>
> >>>>-BlockInfo *bdrv_query_info(BlockDriverState *bs)
> >>>>+/* return 0 on success, and @p_info will be set only on success. */
> >>>>+int bdrv_query_info(BlockDriverState *bs,
> >>>>+                    BlockInfo **p_info,
> >>>>+                    Error **errp)
> >>>>  {
> >>>>      BlockInfo *info = g_malloc0(sizeof(*info));
> >>>>+    BlockDriverState *bs0;
> >>>>+    ImageInfo **p_image_info;
> >>>>+    int ret = 0;
> >>>
> >>>ret is never changed, so this function always returns 0. I would suggest
> >>>to drop ret and make the function return type void.
> >>>
> >>   My bad, I forgot to set its value, the interface is intend to return
> >>negative error number and errp both on fail.
> >
> >Why do you need two separate error reporting mechanisms? Shouldn't only
> >errp be enough?
> >
> >Kevin
> >
>   Returned value can tell caller what error it is, like -ENOMEDIUM.
> Although it is not used by caller now, but I feel better to have it

No, let's remove it if there is no user. We can always add it back if we
ever need it. I doubt that we will.

Kevin

Patch

diff --git a/block/qapi.c b/block/qapi.c
index df73f5b..9051947 100644
--- a/block/qapi.c
+++ b/block/qapi.c
@@ -200,9 +200,15 @@  int bdrv_query_image_info(BlockDriverState *bs,
     return 0;
 }
 
-BlockInfo *bdrv_query_info(BlockDriverState *bs)
+/* return 0 on success, and @p_info will be set only on success. */
+int bdrv_query_info(BlockDriverState *bs,
+                    BlockInfo **p_info,
+                    Error **errp)
 {
     BlockInfo *info = g_malloc0(sizeof(*info));
+    BlockDriverState *bs0;
+    ImageInfo **p_image_info;
+    int ret = 0;
     info->device = g_strdup(bs->device_name);
     info->type = g_strdup("unknown");
     info->locked = bdrv_dev_is_medium_locked(bs);
@@ -256,8 +262,29 @@  BlockInfo *bdrv_query_info(BlockDriverState *bs)
             info->inserted->iops_wr =
                            bs->io_limits.iops[BLOCK_IO_LIMIT_WRITE];
         }
+
+        bs0 = bs;
+        p_image_info = &info->inserted->image;
+        while (1) {
+            if (bdrv_query_image_info(bs0, p_image_info, errp)) {
+                goto err;
+            }
+            if (bs0->drv && bs0->backing_hd) {
+                bs0 = bs0->backing_hd;
+                (*p_image_info)->has_backing_image = true;
+                p_image_info = &((*p_image_info)->backing_image);
+            } else {
+                break;
+            }
+        }
     }
-    return info;
+
+    *p_info = info;
+    return 0;
+
+ err:
+    qapi_free_BlockInfo(info);
+    return ret;
 }
 
 SnapshotInfoList *qmp_query_snapshots(Error **errp)
@@ -284,11 +311,17 @@  BlockInfoList *qmp_query_block(Error **errp)
 
     while ((bs = bdrv_next(bs))) {
         BlockInfoList *info = g_malloc0(sizeof(*info));
-        info->value = bdrv_query_info(bs);
+        if (bdrv_query_info(bs, &info->value, errp)) {
+            goto err;
+        }
 
         *p_next = info;
         p_next = &info->next;
     }
 
     return head;
+
+ err:
+    qapi_free_BlockInfoList(head);
+    return NULL;
 }
diff --git a/include/block/qapi.h b/include/block/qapi.h
index 0039a70..49f9a17 100644
--- a/include/block/qapi.h
+++ b/include/block/qapi.h
@@ -35,5 +35,7 @@  int bdrv_query_snapshot_info_list(BlockDriverState *bs,
 int bdrv_query_image_info(BlockDriverState *bs,
                           ImageInfo **p_info,
                           Error **errp);
-BlockInfo *bdrv_query_info(BlockDriverState *bs);
+int bdrv_query_info(BlockDriverState *bs,
+                    BlockInfo **p_info,
+                    Error **errp);
 #endif
diff --git a/qapi-schema.json b/qapi-schema.json
index b927c97..9a9e673 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -754,6 +754,8 @@ 
 #
 # @iops_wr: write I/O operations per second is specified
 #
+# @image: the info of image used (since: 1.5)
+#
 # Since: 0.14.0
 #
 # Notes: This interface is only found in @BlockInfo.
@@ -763,7 +765,8 @@ 
             '*backing_file': 'str', 'backing_file_depth': 'int',
             'encrypted': 'bool', 'encryption_key_missing': 'bool',
             'bps': 'int', 'bps_rd': 'int', 'bps_wr': 'int',
-            'iops': 'int', 'iops_rd': 'int', 'iops_wr': 'int'} }
+            'iops': 'int', 'iops_rd': 'int', 'iops_wr': 'int',
+            'image': 'ImageInfo' } }
 
 ##
 # @BlockDeviceIoStatus:
diff --git a/qmp-commands.hx b/qmp-commands.hx
index a4cd229..f977c6f 100644
--- a/qmp-commands.hx
+++ b/qmp-commands.hx
@@ -1681,6 +1681,47 @@  Each json-object contain the following:
          - "iops": limit total I/O operations per second (json-int)
          - "iops_rd": limit read operations per second (json-int)
          - "iops_wr": limit write operations per second (json-int)
+         - "image": the detail of the image, it is a json-object containing
+            the following:
+             - "filename": image file name (json-string)
+             - "format": image format (json-string)
+             - "virtual-size": image capacity in bytes (json-int)
+             - "dirty-flag": true if image is not cleanly closed, not present
+                             means clean (json-bool, optional)
+             - "actual-size": actual size on disk in bytes of the image, not
+                              present when image does not support thin
+                              provision (json-int, optional)
+             - "cluster-size": size of a cluster in bytes, not present if image
+                               format does not support it (json-int, optional)
+             - "encrypted": true if the image is encrypted, not present means
+                            false or the image format does not support
+                            encryption (json-bool, optional)
+             - "backing_file": backing file name, not present means no backing
+                               file is used or the image format does not
+                               support backing file chain
+                               (json-string, optional)
+             - "full-backing-filename": full path of the backing file, not
+                                        present if it equals backing_file or no
+                                        backing file is used
+                                        (json-string, optional)
+             - "backing-filename-format": the format of the backing file, not
+                                          present means unknown or no backing
+                                          file (json-string, optional)
+             - "snapshots": the internal snapshot info, it is an optional list
+                of json-object containing the following:
+                 - "id": unique snapshot id (json-string)
+                 - "name": snapshot name (json-string)
+                 - "vm-state-size": size of the VM state in bytes (json-int)
+                 - "date-sec": UTC date of the snapshot in seconds (json-int)
+                 - "date-nsec": fractional part in nanoseconds to be used with
+                                date-sec(json-int)
+                 - "vm-clock-sec": VM clock relative to boot in seconds
+                                   (json-int)
+                 - "vm-clock-nsec": fractional part in nanoseconds to be used
+                                    with vm-clock-sec (json-int)
+             - "backing-image": the detail of the backing image, it is an
+                                optional json-object only present when a
+                                backing image present for this image
 
 - "io-status": I/O operation status, only present if the device supports it
                and the VM is configured to stop on errors. It's always reset
@@ -1702,13 +1743,37 @@  Example:
                "drv":"qcow2",
                "encrypted":false,
                "file":"disks/test.img",
-               "backing_file_depth":0,
+               "backing_file_depth":1,
                "bps":1000000,
                "bps_rd":0,
                "bps_wr":0,
                "iops":1000000,
                "iops_rd":0,
                "iops_wr":0,
+               "image":{
+                  "filename":"disks/test.img",
+                  "format":"qcow2",
+                  "virtual-size":2048000,
+                  "backing_file":"base.img",
+                  "full-backing-filename":"disks/base.img",
+                  "backing-filename-format:"qcow2",
+                  "snapshots":[
+                     {
+                        "id": "1",
+                        "name": "snapshot1",
+                        "vm-state-size": 0,
+                        "date-sec": 10000200,
+                        "date-nsec": 12,
+                        "vm-clock-sec": 206,
+                        "vm-clock-nsec": 30
+                     }
+                  ],
+                  "backing-image":{
+                      "filename":"disks/base.img",
+                      "format":"qcow2",
+                      "virtual-size":2048000
+                  }
+               }
             },
             "type":"unknown"
          },