Message ID | 1429271563-3765-1-git-send-email-berto@igalia.com |
---|---|
State | New |
Headers | show |
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'
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
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
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);
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(-)