[Qemu-devel] [PATCH] qmp: fill in the image field in BlockDeviceInfo

[Qemu-devel] [PATCH] qmp: fill in the image field in BlockDeviceInfo

[Alberto Garcia]

The image field in BlockDeviceInfo is supposed to contain an ImageInfo
object. However that is being filled in by bdrv_query_info(), not by
bdrv_block_device_info(), which is where BlockDeviceInfo is actually
created.

Anyone calling bdrv_block_device_info() directly will get a null image
field. As a consequence of this, the HMP command 'info block -n -v'
crashes QEMU.

This patch moves the code that fills in that field from
bdrv_query_info() to bdrv_block_device_info().

Signed-off-by: Alberto Garcia <berto@igalia.com>
---
 block.c               |  9 +++++++--
 block/qapi.c          | 46 +++++++++++++++++++++++++---------------------
 blockdev.c            |  2 +-
 include/block/block.h |  2 +-
 include/block/qapi.h  |  2 +-
 5 files changed, 35 insertions(+), 26 deletions(-)

diff --git a/block.c b/block.c
index f2f8ae7..9db2ad9 100644
--- a/block.c
+++ b/block.c
@@ -3870,15 +3870,20 @@ BlockDriverState *bdrv_find_node(const char *node_name)
 }
 
 /* Put this QMP function here so it can access the static graph_bdrv_states. */
-BlockDeviceInfoList *bdrv_named_nodes_list(void)
+BlockDeviceInfoList *bdrv_named_nodes_list(Error **errp)
 {
     BlockDeviceInfoList *list, *entry;
     BlockDriverState *bs;
 
     list = NULL;
     QTAILQ_FOREACH(bs, &graph_bdrv_states, node_list) {
+        BlockDeviceInfo *info = bdrv_block_device_info(bs, errp);
+        if (!info) {
+            qapi_free_BlockDeviceInfoList(list);
+            return NULL;
+        }
         entry = g_malloc0(sizeof(*entry));
-        entry->value = bdrv_block_device_info(bs);
+        entry->value = info;
         entry->next = list;
         list = entry;
     }
diff --git a/block/qapi.c b/block/qapi.c
index 8a19aed..063dd1b 100644
--- a/block/qapi.c
+++ b/block/qapi.c
@@ -31,8 +31,10 @@
 #include "qapi/qmp/types.h"
 #include "sysemu/block-backend.h"
 
-BlockDeviceInfo *bdrv_block_device_info(BlockDriverState *bs)
+BlockDeviceInfo *bdrv_block_device_info(BlockDriverState *bs, Error **errp)
 {
+    ImageInfo **p_image_info;
+    BlockDriverState *bs0;
     BlockDeviceInfo *info = g_malloc0(sizeof(*info));
 
     info->file                   = g_strdup(bs->filename);
@@ -92,6 +94,25 @@ BlockDeviceInfo *bdrv_block_device_info(BlockDriverState *bs)
 
     info->write_threshold = bdrv_write_threshold_get(bs);
 
+    bs0 = bs;
+    p_image_info = &info->image;
+    while (1) {
+        Error *local_err = NULL;
+        bdrv_query_image_info(bs0, p_image_info, &local_err);
+        if (local_err) {
+            error_propagate(errp, local_err);
+            qapi_free_BlockDeviceInfo(info);
+            return NULL;
+        }
+        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;
 }
 
@@ -264,9 +285,6 @@ static void bdrv_query_info(BlockBackend *blk, BlockInfo **p_info,
 {
     BlockInfo *info = g_malloc0(sizeof(*info));
     BlockDriverState *bs = blk_bs(blk);
-    BlockDriverState *bs0;
-    ImageInfo **p_image_info;
-    Error *local_err = NULL;
     info->device = g_strdup(blk_name(blk));
     info->type = g_strdup("unknown");
     info->locked = blk_dev_is_medium_locked(blk);
@@ -289,23 +307,9 @@ static void bdrv_query_info(BlockBackend *blk, BlockInfo **p_info,
 
     if (bs->drv) {
         info->has_inserted = true;
-        info->inserted = bdrv_block_device_info(bs);
-
-        bs0 = bs;
-        p_image_info = &info->inserted->image;
-        while (1) {
-            bdrv_query_image_info(bs0, p_image_info, &local_err);
-            if (local_err) {
-                error_propagate(errp, local_err);
-                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;
-            }
+        info->inserted = bdrv_block_device_info(bs, errp);
+        if (info->inserted == NULL) {
+            goto err;
         }
     }
 
diff --git a/blockdev.c b/blockdev.c
index fbb3a79..65e9bb6 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -2391,7 +2391,7 @@ out:
 
 BlockDeviceInfoList *qmp_query_named_block_nodes(Error **errp)
 {
-    return bdrv_named_nodes_list();
+    return bdrv_named_nodes_list(errp);
 }
 
 void qmp_blockdev_backup(const char *device, const char *target,
diff --git a/include/block/block.h b/include/block/block.h
index 4c57d63..40131b2 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -382,7 +382,7 @@ void bdrv_lock_medium(BlockDriverState *bs, bool locked);
 void bdrv_eject(BlockDriverState *bs, bool eject_flag);
 const char *bdrv_get_format_name(BlockDriverState *bs);
 BlockDriverState *bdrv_find_node(const char *node_name);
-BlockDeviceInfoList *bdrv_named_nodes_list(void);
+BlockDeviceInfoList *bdrv_named_nodes_list(Error **errp);
 BlockDriverState *bdrv_lookup_bs(const char *device,
                                  const char *node_name,
                                  Error **errp);
diff --git a/include/block/qapi.h b/include/block/qapi.h
index 168d788..327549d 100644
--- a/include/block/qapi.h
+++ b/include/block/qapi.h
@@ -29,7 +29,7 @@
 #include "block/block.h"
 #include "block/snapshot.h"
 
-BlockDeviceInfo *bdrv_block_device_info(BlockDriverState *bs);
+BlockDeviceInfo *bdrv_block_device_info(BlockDriverState *bs, Error **errp);
 int bdrv_query_snapshot_info_list(BlockDriverState *bs,
                                   SnapshotInfoList **p_list,
                                   Error **errp);
-- 
2.1.4

Re: [Qemu-devel] [PATCH] qmp: fill in the image field in BlockDeviceInfo

[Alberto Garcia]

On Fri 17 Apr 2015 01:52:43 PM CEST, Alberto Garcia <berto@igalia.com> wrote:

> Anyone calling bdrv_block_device_info() directly will get a null image
> field. As a consequence of this, the HMP command 'info block -n -v'
> crashes QEMU.
>
> This patch moves the code that fills in that field from
> bdrv_query_info() to bdrv_block_device_info().

And in case this change is too big/risky for 2.3, there's also the
simple workaround for the crash:

--- a/hmp.c
+++ b/hmp.c
@@ -391,7 +391,7 @@ static void print_block_info(Monitor *mon, BlockInfo *info,
                         inserted->iops_size);
     }
 
-    if (verbose) {
+    if (verbose && inserted->image) {
         monitor_printf(mon, "\nImages:\n");
         image_info = inserted->image;
         while (1) {

Berto

Re: [Qemu-devel] [Qemu-block] [PATCH] qmp: fill in the image field in BlockDeviceInfo

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 1033 bytes --]

On Fri, Apr 17, 2015 at 02:52:43PM +0300, Alberto Garcia wrote:
> The image field in BlockDeviceInfo is supposed to contain an ImageInfo
> object. However that is being filled in by bdrv_query_info(), not by
> bdrv_block_device_info(), which is where BlockDeviceInfo is actually
> created.
> 
> Anyone calling bdrv_block_device_info() directly will get a null image
> field. As a consequence of this, the HMP command 'info block -n -v'
> crashes QEMU.
> 
> This patch moves the code that fills in that field from
> bdrv_query_info() to bdrv_block_device_info().
> 
> Signed-off-by: Alberto Garcia <berto@igalia.com>
> ---
>  block.c               |  9 +++++++--
>  block/qapi.c          | 46 +++++++++++++++++++++++++---------------------
>  blockdev.c            |  2 +-
>  include/block/block.h |  2 +-
>  include/block/qapi.h  |  2 +-
>  5 files changed, 35 insertions(+), 26 deletions(-)

For the record, the following patch has been merged instead:
  [PATCH for-2.3] hmp: fix crash in 'info block -n -v'

[-- Attachment #2: Type: application/pgp-signature, Size: 473 bytes --]

Re: [Qemu-devel] [Qemu-block] [PATCH] qmp: fill in the image field in BlockDeviceInfo

[Kevin Wolf]

[-- Attachment #1: Type: text/plain, Size: 1337 bytes --]

Am 21.04.2015 um 15:28 hat Stefan Hajnoczi geschrieben:
> On Fri, Apr 17, 2015 at 02:52:43PM +0300, Alberto Garcia wrote:
> > The image field in BlockDeviceInfo is supposed to contain an ImageInfo
> > object. However that is being filled in by bdrv_query_info(), not by
> > bdrv_block_device_info(), which is where BlockDeviceInfo is actually
> > created.
> > 
> > Anyone calling bdrv_block_device_info() directly will get a null image
> > field. As a consequence of this, the HMP command 'info block -n -v'
> > crashes QEMU.
> > 
> > This patch moves the code that fills in that field from
> > bdrv_query_info() to bdrv_block_device_info().
> > 
> > Signed-off-by: Alberto Garcia <berto@igalia.com>
> > ---
> >  block.c               |  9 +++++++--
> >  block/qapi.c          | 46 +++++++++++++++++++++++++---------------------
> >  blockdev.c            |  2 +-
> >  include/block/block.h |  2 +-
> >  include/block/qapi.h  |  2 +-
> >  5 files changed, 35 insertions(+), 26 deletions(-)
> 
> For the record, the following patch has been merged instead:
>   [PATCH for-2.3] hmp: fix crash in 'info block -n -v'

That was just a minimal stopgap solution that could still be applied
after -rc3. We should revert it in block-next and apply this one as a
replacement (after proper review, of course).

Kevin

[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]

Re: [Qemu-devel] [Qemu-block] [PATCH] qmp: fill in the image field in BlockDeviceInfo

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 1081 bytes --]

On Fri, Apr 17, 2015 at 02:52:43PM +0300, Alberto Garcia wrote:
> The image field in BlockDeviceInfo is supposed to contain an ImageInfo
> object. However that is being filled in by bdrv_query_info(), not by
> bdrv_block_device_info(), which is where BlockDeviceInfo is actually
> created.
> 
> Anyone calling bdrv_block_device_info() directly will get a null image
> field. As a consequence of this, the HMP command 'info block -n -v'
> crashes QEMU.
> 
> This patch moves the code that fills in that field from
> bdrv_query_info() to bdrv_block_device_info().
> 
> Signed-off-by: Alberto Garcia <berto@igalia.com>
> ---
>  block.c               |  9 +++++++--
>  block/qapi.c          | 46 +++++++++++++++++++++++++---------------------
>  blockdev.c            |  2 +-
>  include/block/block.h |  2 +-
>  include/block/qapi.h  |  2 +-
>  5 files changed, 35 insertions(+), 26 deletions(-)

Reverted the QEMU 2.3 fix and replaced it with this commit.

Thanks, applied to my block-next tree:
https://github.com/stefanha/qemu/commits/block-next

Stefan

[-- Attachment #2: Type: application/pgp-signature, Size: 473 bytes --]