Re: [Qemu-devel] [PATCH V5 3/7] qmp: Add a command to list the named BlockDriverState nodes.

[Fam Zheng]

I think it's worth to at least mention the name of the command in the commit
message.

On Thu, 12/12 16:33, Benoît Canet wrote:
> Signed-off-by: Benoit Canet <address@hidden>
> ---
>  block.c               |  18 +++++++++
>  block/qapi.c          | 109 
> +++++++++++++++++++++++++-------------------------
>  blockdev.c            |   5 +++
>  include/block/block.h |   1 +
>  include/block/qapi.h  |   1 +
>  qapi-schema.json      |  16 +++++++-
>  qmp-commands.hx       |  61 ++++++++++++++++++++++++++++
>  7 files changed, 155 insertions(+), 56 deletions(-)
> 
> diff --git a/block.c b/block.c
> index 1c57f0d..78d13e5 100644
> --- a/block.c
> +++ b/block.c
> @@ -32,6 +32,7 @@
>  #include "sysemu/sysemu.h"
>  #include "qemu/notify.h"
>  #include "block/coroutine.h"
> +#include "block/qapi.h"
>  #include "qmp-commands.h"
>  #include "qemu/timer.h"
>  
> @@ -3189,6 +3190,23 @@ BlockDriverState *bdrv_find_node(const char *node_name)
>      return NULL;
>  }
>  
> +/* Put this QMP function here so it can access the static graph_bdrv_states. 
> */
> +BlockDeviceInfoList *bdrv_named_nodes_list(void)
> +{
> +    BlockDeviceInfoList *list, *entry;
> +    BlockDriverState *bs;
> +
> +    list = NULL;
> +    QTAILQ_FOREACH(bs, &graph_bdrv_states, node_list) {
> +        entry = g_malloc0(sizeof(*entry));
> +        entry->value = bdrv_block_device_info(bs);
> +        entry->next = list;
> +        list = entry;
> +    }
> +
> +    return list;
> +}
> +
>  BlockDriverState *bdrv_next(BlockDriverState *bs)
>  {
>      if (!bs) {
> diff --git a/block/qapi.c b/block/qapi.c
> index a32cb79..556f7fb 100644
> --- a/block/qapi.c
> +++ b/block/qapi.c
> @@ -29,6 +29,60 @@
>  #include "qapi/qmp-output-visitor.h"
>  #include "qapi/qmp/types.h"
>  
> +BlockDeviceInfo *bdrv_block_device_info(BlockDriverState *bs)
> +{
> +    BlockDeviceInfo *info = g_malloc0(sizeof(*info));
> +
> +    info->file                   = g_strdup(bs->filename);
> +    info->ro                     = bs->read_only;
> +    info->drv                    = g_strdup(bs->drv->format_name);
> +    info->encrypted              = bs->encrypted;
> +    info->encryption_key_missing = bdrv_key_required(bs);
> +
> +    if (bs->node_name[0]) {
> +        info->has_node_name = true;
> +        info->node_name = g_strdup(bs->node_name);
> +    }
> +
> +    if (bs->backing_file[0]) {
> +        info->has_backing_file = true;
> +        info->backing_file = g_strdup(bs->backing_file);
> +    }
> +
> +    info->backing_file_depth = bdrv_get_backing_file_depth(bs);
> +
> +    if (bs->io_limits_enabled) {
> +        ThrottleConfig cfg;
> +        throttle_get_config(&bs->throttle_state, &cfg);
> +        info->bps     = cfg.buckets[THROTTLE_BPS_TOTAL].avg;
> +        info->bps_rd  = cfg.buckets[THROTTLE_BPS_READ].avg;
> +        info->bps_wr  = cfg.buckets[THROTTLE_BPS_WRITE].avg;
> +
> +        info->iops    = cfg.buckets[THROTTLE_OPS_TOTAL].avg;
> +        info->iops_rd = cfg.buckets[THROTTLE_OPS_READ].avg;
> +        info->iops_wr = cfg.buckets[THROTTLE_OPS_WRITE].avg;
> +
> +        info->has_bps_max     = cfg.buckets[THROTTLE_BPS_TOTAL].max;
> +        info->bps_max         = cfg.buckets[THROTTLE_BPS_TOTAL].max;
> +        info->has_bps_rd_max  = cfg.buckets[THROTTLE_BPS_READ].max;
> +        info->bps_rd_max      = cfg.buckets[THROTTLE_BPS_READ].max;
> +        info->has_bps_wr_max  = cfg.buckets[THROTTLE_BPS_WRITE].max;
> +        info->bps_wr_max      = cfg.buckets[THROTTLE_BPS_WRITE].max;
> +
> +        info->has_iops_max    = cfg.buckets[THROTTLE_OPS_TOTAL].max;
> +        info->iops_max        = cfg.buckets[THROTTLE_OPS_TOTAL].max;
> +        info->has_iops_rd_max = cfg.buckets[THROTTLE_OPS_READ].max;
> +        info->iops_rd_max     = cfg.buckets[THROTTLE_OPS_READ].max;
> +        info->has_iops_wr_max = cfg.buckets[THROTTLE_OPS_WRITE].max;
> +        info->iops_wr_max     = cfg.buckets[THROTTLE_OPS_WRITE].max;
> +
> +        info->has_iops_size = cfg.op_size;
> +        info->iops_size = cfg.op_size;
> +    }
> +
> +    return info;
> +}
> +
>  /*
>   * Returns 0 on success, with *p_list either set to describe snapshot
>   * information, or NULL because there are no snapshots.  Returns -errno on
> @@ -211,60 +265,7 @@ void bdrv_query_info(BlockDriverState *bs,
>  
>      if (bs->drv) {
>          info->has_inserted = true;
> -        info->inserted = g_malloc0(sizeof(*info->inserted));
> -        info->inserted->file = g_strdup(bs->filename);
> -        info->inserted->ro = bs->read_only;
> -        info->inserted->drv = g_strdup(bs->drv->format_name);
> -        info->inserted->encrypted = bs->encrypted;
> -        info->inserted->encryption_key_missing = bdrv_key_required(bs);
> -
> -        if (bs->backing_file[0]) {
> -            info->inserted->has_backing_file = true;
> -            info->inserted->backing_file = g_strdup(bs->backing_file);
> -        }
> -
> -        info->inserted->backing_file_depth = bdrv_get_backing_file_depth(bs);
> -
> -        if (bs->io_limits_enabled) {
> -            ThrottleConfig cfg;
> -            throttle_get_config(&bs->throttle_state, &cfg);
> -            info->inserted->bps     = cfg.buckets[THROTTLE_BPS_TOTAL].avg;
> -            info->inserted->bps_rd  = cfg.buckets[THROTTLE_BPS_READ].avg;
> -            info->inserted->bps_wr  = cfg.buckets[THROTTLE_BPS_WRITE].avg;
> -
> -            info->inserted->iops    = cfg.buckets[THROTTLE_OPS_TOTAL].avg;
> -            info->inserted->iops_rd = cfg.buckets[THROTTLE_OPS_READ].avg;
> -            info->inserted->iops_wr = cfg.buckets[THROTTLE_OPS_WRITE].avg;
> -
> -            info->inserted->has_bps_max     =
> -                cfg.buckets[THROTTLE_BPS_TOTAL].max;
> -            info->inserted->bps_max         =
> -                cfg.buckets[THROTTLE_BPS_TOTAL].max;
> -            info->inserted->has_bps_rd_max  =
> -                cfg.buckets[THROTTLE_BPS_READ].max;
> -            info->inserted->bps_rd_max      =
> -                cfg.buckets[THROTTLE_BPS_READ].max;
> -            info->inserted->has_bps_wr_max  =
> -                cfg.buckets[THROTTLE_BPS_WRITE].max;
> -            info->inserted->bps_wr_max      =
> -                cfg.buckets[THROTTLE_BPS_WRITE].max;
> -
> -            info->inserted->has_iops_max    =
> -                cfg.buckets[THROTTLE_OPS_TOTAL].max;
> -            info->inserted->iops_max        =
> -                cfg.buckets[THROTTLE_OPS_TOTAL].max;
> -            info->inserted->has_iops_rd_max =
> -                cfg.buckets[THROTTLE_OPS_READ].max;
> -            info->inserted->iops_rd_max     =
> -                cfg.buckets[THROTTLE_OPS_READ].max;
> -            info->inserted->has_iops_wr_max =
> -                cfg.buckets[THROTTLE_OPS_WRITE].max;
> -            info->inserted->iops_wr_max     =
> -                cfg.buckets[THROTTLE_OPS_WRITE].max;
> -
> -            info->inserted->has_iops_size = cfg.op_size;
> -            info->inserted->iops_size = cfg.op_size;
> -        }
> +        info->inserted = bdrv_block_device_info(bs);
>  
>          bs0 = bs;
>          p_image_info = &info->inserted->image;
> diff --git a/blockdev.c b/blockdev.c
> index 44755e1..204ab40 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -1940,6 +1940,11 @@ void qmp_drive_backup(const char *device, const char 
> *target,
>      }
>  }
>  
> +BlockDeviceInfoList *qmp_query_named_block_nodes(Error **errp)
> +{
> +    return bdrv_named_nodes_list();
> +}
> +
>  #define DEFAULT_MIRROR_BUF_SIZE   (10 << 20)
>  
>  void qmp_drive_mirror(const char *device, const char *target,
> diff --git a/include/block/block.h b/include/block/block.h
> index 834abf9..8c10123 100644
> --- a/include/block/block.h
> +++ b/include/block/block.h
> @@ -375,6 +375,7 @@ void bdrv_eject(BlockDriverState *bs, bool eject_flag);
>  const char *bdrv_get_format_name(BlockDriverState *bs);
>  BlockDriverState *bdrv_find(const char *name);
>  BlockDriverState *bdrv_find_node(const char *node_name);
> +BlockDeviceInfoList *bdrv_named_nodes_list(void);
>  BlockDriverState *bdrv_next(BlockDriverState *bs);
>  void bdrv_iterate(void (*it)(void *opaque, BlockDriverState *bs),
>                    void *opaque);
> diff --git a/include/block/qapi.h b/include/block/qapi.h
> index 9518ee4..e92c00d 100644
> --- a/include/block/qapi.h
> +++ b/include/block/qapi.h
> @@ -29,6 +29,7 @@
>  #include "block/block.h"
>  #include "block/snapshot.h"
>  
> +BlockDeviceInfo *bdrv_block_device_info(BlockDriverState *bs);
>  int bdrv_query_snapshot_info_list(BlockDriverState *bs,
>                                    SnapshotInfoList **p_list,
>                                    Error **errp);
> diff --git a/qapi-schema.json b/qapi-schema.json
> index c3c939c..0dadd5d 100644
> --- a/qapi-schema.json
> +++ b/qapi-schema.json
> @@ -810,6 +810,8 @@
>  #
>  # @file: the filename of the backing device
>  #
> +# @node-name: #optional the name of the block driver node (Since 2.0)
> +#
>  # @ro: true if the backing device was open read-only
>  #
>  # @drv: the name of the block format used to open the backing device. As of
> @@ -857,10 +859,9 @@
>  #
>  # Since: 0.14.0
>  #
> -# Notes: This interface is only found in @BlockInfo.
>  ##
>  { 'type': 'BlockDeviceInfo',
> -  'data': { 'file': 'str', 'ro': 'bool', 'drv': 'str',
> +  'data': { 'file': 'str', '*node-name': 'str', 'ro': 'bool', 'drv': 'str',
>              '*backing_file': 'str', 'backing_file_depth': 'int',
>              'encrypted': 'bool', 'encryption_key_missing': 'bool',
>              'bps': 'int', 'bps_rd': 'int', 'bps_wr': 'int',
> @@ -2008,6 +2009,17 @@
>  { 'command': 'drive-backup', 'data': 'DriveBackup' }
>  
>  ##
> +# @query-named-block-nodes
> +#
> +# Get the named block driver list
> +#
> +# Returns: the list of BlockDeviceInfo
> +#
> +# Since 2.0
> +##
> +{ 'command': 'query-named-block-nodes', 'returns': [ 'BlockDeviceInfo' ] }
> +
> +##
>  # @drive-mirror
>  #
>  # Start mirroring a block device's writes to a new destination.
> diff --git a/qmp-commands.hx b/qmp-commands.hx
> index fba15cd..d644fe9 100644
> --- a/qmp-commands.hx
> +++ b/qmp-commands.hx
> @@ -3295,3 +3295,64 @@ Example (2):
>  <- { "return": {} }
>  
>  EQMP
> +
> +    {
> +        .name       = "query-named-block-nodes",
> +        .args_type  = "",
> +        .mhandler.cmd_new = qmp_marshal_input_query_named_block_nodes,
> +    },
> +
> +SQMP
> address@hidden
> +------------------------
> +
> +Return a list of BlockDeviceInfo for each named block driver node

How about "Return a list of BlockDeviceInfo for all the named block driver 
nodes"

But nothing to stop adding

Reviewed-by: Fam Zheng <address@hidden>

> +
> +Example:
> +
> +-> { "execute": "query-named-block-nodes" }
> +<- { "return": [ { "ro":false,
> +                   "drv":"qcow2",
> +                   "encrypted":false,
> +                   "file":"disks/test.qcow2",
> +                   "node-name": "my-node",
> +                   "backing_file_depth":1,
> +                   "bps":1000000,
> +                   "bps_rd":0,
> +                   "bps_wr":0,
> +                   "iops":1000000,
> +                   "iops_rd":0,
> +                   "iops_wr":0,
> +                   "bps_max": 8000000,
> +                   "bps_rd_max": 0,
> +                   "bps_wr_max": 0,
> +                   "iops_max": 0,
> +                   "iops_rd_max": 0,
> +                   "iops_wr_max": 0,
> +                   "iops_size": 0,
> +                   "image":{
> +                      "filename":"disks/test.qcow2",
> +                      "format":"qcow2",
> +                      "virtual-size":2048000,
> +                      "backing_file":"base.qcow2",
> +                      "full-backing-filename":"disks/base.qcow2",
> +                      "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.qcow2",
> +                          "format":"qcow2",
> +                          "virtual-size":2048000
> +                      }
> +                   } } ] }
> +
> +EQMP
> -- 
> 1.8.3.2
> 
>