Re: [Qemu-devel] [PATCH V10 11/17] qmp: add ImageInfo in BlockDeviceInfo used by query-block

[Wenchao Xia]

于 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 <xiaw...@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

--
Best Regards

Wenchao Xia