[Qemu-devel] [PATCH 0/6] block: Single-device query-block(-node) and cache info

[Qemu-devel] [PATCH 0/6] block: Single-device query-block(-node) and cache info

[Kevin Wolf]

This was supposed to be the first part of a series allowing to override
the cache mode for backing files, and more specifically to support the
test cases for that series.

However, this work turned out to be a bit more complicated than I had
hoped, and the query improvement is nice to have on its own, so I'm
sending this out as a standalone series.

Kevin Wolf (6):
  block/qapi: Add cache information to query-block
  block: Add optional device argument to query-block
  block: Introduce query-block-node
  block/hmp: Factor out print_block_info()
  block/hmp: Allow info = NULL in print_block_info()
  block: Allow node-name in HMP 'info block'

 block.c                    |  15 +++-
 block/qapi.c               |  25 +++++-
 blockdev.c                 |   8 +-
 hmp.c                      | 208 ++++++++++++++++++++++++++-------------------
 include/block/block.h      |   2 +-
 qapi/block-core.json       |  31 +++++--
 tests/qemu-iotests/051.out |   1 +
 tests/qemu-iotests/067.out |  10 +--
 8 files changed, 196 insertions(+), 104 deletions(-)

-- 
1.8.3.1

[Qemu-devel] [PATCH 1/6] block/qapi: Add cache information to query-block

[Kevin Wolf]

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 block/qapi.c               | 10 ++++++++++
 hmp.c                      |  8 ++++++++
 qapi/block-core.json       |  4 +++-
 tests/qemu-iotests/051.out |  1 +
 tests/qemu-iotests/067.out | 10 +++++-----
 5 files changed, 27 insertions(+), 6 deletions(-)

diff --git a/block/qapi.c b/block/qapi.c
index 9733ebd..6b43bc3 100644
--- a/block/qapi.c
+++ b/block/qapi.c
@@ -46,6 +46,16 @@ BlockDeviceInfo *bdrv_block_device_info(BlockDriverState *bs)
     info->encrypted              = bs->encrypted;
     info->encryption_key_missing = bdrv_key_required(bs);
 
+    info->cache = g_new(BlockdevCacheOptions, 1);
+    *info->cache = (BlockdevCacheOptions) {
+        .writeback      = bdrv_enable_write_cache(bs),
+        .has_writeback  = true,
+        .direct         = !!(bs->open_flags & BDRV_O_NOCACHE),
+        .has_direct     = true,
+        .no_flush       = !!(bs->open_flags & BDRV_O_NO_FLUSH),
+        .has_no_flush   = true,
+    };
+
     if (bs->node_name[0]) {
         info->has_node_name = true;
         info->node_name = g_strdup(bs->node_name);
diff --git a/hmp.c b/hmp.c
index 40a90da..c3846b8 100644
--- a/hmp.c
+++ b/hmp.c
@@ -294,6 +294,7 @@ void hmp_info_block(Monitor *mon, const QDict *qdict)
 {
     BlockInfoList *block_list, *info;
     ImageInfo *image_info;
+    BlockDeviceInfo *inserted;
     const char *device = qdict_get_try_str(qdict, "device");
     bool verbose = qdict_get_try_bool(qdict, "verbose", 0);
 
@@ -335,6 +336,13 @@ void hmp_info_block(Monitor *mon, const QDict *qdict)
             continue;
         }
 
+        inserted = info->value->inserted;
+
+        monitor_printf(mon, "    Cache mode:       %s%s%s\n",
+                       inserted->cache->writeback ? "writeback" : "writethrough",
+                       inserted->cache->direct ? ", direct" : "",
+                       inserted->cache->no_flush ? ", ignore flushes" : "");
+
         if (info->value->inserted->has_backing_file) {
             monitor_printf(mon,
                            "    Backing file:     %s "
diff --git a/qapi/block-core.json b/qapi/block-core.json
index 95dcd81..c53b587 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -238,6 +238,8 @@
 #
 # @iops_size: #optional an I/O size in bytes (Since 1.7)
 #
+# @cache: the cache mode used for the block device (since: 2.2)
+#
 # Since: 0.14.0
 #
 ##
@@ -252,7 +254,7 @@
             '*bps_max': 'int', '*bps_rd_max': 'int',
             '*bps_wr_max': 'int', '*iops_max': 'int',
             '*iops_rd_max': 'int', '*iops_wr_max': 'int',
-            '*iops_size': 'int' } }
+            '*iops_size': 'int', 'cache': 'BlockdevCacheOptions' } }
 
 ##
 # @BlockDeviceIoStatus:
diff --git a/tests/qemu-iotests/051.out b/tests/qemu-iotests/051.out
index a3f2820..fee597e 100644
--- a/tests/qemu-iotests/051.out
+++ b/tests/qemu-iotests/051.out
@@ -50,6 +50,7 @@ Testing: -drive file=TEST_DIR/t.qcow2,driver=qcow2,backing.file.filename=TEST_DI
 QEMU X.Y.Z monitor - type 'help' for more information
 (qemu) i^[[K^[[Din^[[K^[[D^[[Dinf^[[K^[[D^[[D^[[Dinfo^[[K^[[D^[[D^[[D^[[Dinfo ^[[K^[[D^[[D^[[D^[[D^[[Dinfo b^[[K^[[D^[[D^[[D^[[D^[[D^[[Dinfo bl^[[K^[[D^[[D^[[D^[[D^[[D^[[D^[[Dinfo blo^[[K^[[D^[[D^[[D^[[D^[[D^[[D^[[D^[[Dinfo bloc^[[K^[[D^[[D^[[D^[[D^[[D^[[D^[[D^[[D^[[Dinfo block^[[K
 ide0-hd0: TEST_DIR/t.qcow2 (qcow2)
+    Cache mode:       writeback
     Backing file:     TEST_DIR/t.qcow2.orig (chain depth: 1)
 (qemu) q^[[K^[[Dqu^[[K^[[D^[[Dqui^[[K^[[D^[[D^[[Dquit^[[K
 
diff --git a/tests/qemu-iotests/067.out b/tests/qemu-iotests/067.out
index 7e090b9..3774c95 100644
--- a/tests/qemu-iotests/067.out
+++ b/tests/qemu-iotests/067.out
@@ -6,7 +6,7 @@ Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=134217728
 Testing: -drive file=TEST_DIR/t.qcow2,format=qcow2,if=none,id=disk -device virtio-blk-pci,drive=disk,id=virtio0
 QMP_VERSION
 {"return": {}}
-{"return": [{"io-status": "ok", "device": "disk", "locked": false, "removable": false, "inserted": {"iops_rd": 0, "detect_zeroes": "off", "image": {"virtual-size": 134217728, "filename": "TEST_DIR/t.qcow2", "cluster-size": 65536, "format": "qcow2", "actual-size": SIZE, "format-specific": {"type": "qcow2", "data": {"compat": "1.1", "lazy-refcounts": false}}, "dirty-flag": false}, "iops_wr": 0, "ro": false, "backing_file_depth": 0, "drv": "qcow2", "iops": 0, "bps_wr": 0, "encrypted": false, "bps": 0, "bps_rd": 0, "file": "TEST_DIR/t.qcow2", "encryption_key_missing": false}, "type": "unknown"}, {"io-status": "ok", "device": "ide1-cd0", "locked": false, "removable": true, "tray_open": false, "type": "unknown"}, {"device": "floppy0", "locked": false, "removable": true, "tray_open": false, "type": "unknown"}, {"device": "sd0", "locked": false, "removable": true, "tray_open": false, "type": "unknown"}]}
+{"return": [{"io-status": "ok", "device": "disk", "locked": false, "removable": false, "inserted": {"iops_rd": 0, "detect_zeroes": "off", "image": {"virtual-size": 134217728, "filename": "TEST_DIR/t.qcow2", "cluster-size": 65536, "format": "qcow2", "actual-size": SIZE, "format-specific": {"type": "qcow2", "data": {"compat": "1.1", "lazy-refcounts": false}}, "dirty-flag": false}, "iops_wr": 0, "ro": false, "backing_file_depth": 0, "drv": "qcow2", "iops": 0, "bps_wr": 0, "encrypted": false, "bps": 0, "bps_rd": 0, "cache": {"no-flush": false, "direct": false, "writeback": true}, "file": "TEST_DIR/t.qcow2", "encryption_key_missing": false}, "type": "unknown"}, {"io-status": "ok", "device": "ide1-cd0", "locked": false, "removable": true, "tray_open": false, "type": "unknown"}, {"device": "floppy0", "locked": false, "removable": true, "tray_open": false, "type": "unknown"}, {"device": "sd0", "locked": false, "removable": true, "tray_open": false, "type": "unknown"}]}
 {"return": {}}
 {"return": {}}
 {"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": "DEVICE_DELETED", "data": {"path": "/machine/peripheral/virtio0/virtio-backend"}}
@@ -24,7 +24,7 @@ QMP_VERSION
 Testing: -drive file=TEST_DIR/t.qcow2,format=qcow2,if=none,id=disk
 QMP_VERSION
 {"return": {}}
-{"return": [{"device": "disk", "locked": false, "removable": true, "inserted": {"iops_rd": 0, "detect_zeroes": "off", "image": {"virtual-size": 134217728, "filename": "TEST_DIR/t.qcow2", "cluster-size": 65536, "format": "qcow2", "actual-size": SIZE, "format-specific": {"type": "qcow2", "data": {"compat": "1.1", "lazy-refcounts": false}}, "dirty-flag": false}, "iops_wr": 0, "ro": false, "backing_file_depth": 0, "drv": "qcow2", "iops": 0, "bps_wr": 0, "encrypted": false, "bps": 0, "bps_rd": 0, "file": "TEST_DIR/t.qcow2", "encryption_key_missing": false}, "tray_open": false, "type": "unknown"}, {"io-status": "ok", "device": "ide1-cd0", "locked": false, "removable": true, "tray_open": false, "type": "unknown"}, {"device": "floppy0", "locked": false, "removable": true, "tray_open": false, "type": "unknown"}, {"device": "sd0", "locked": false, "removable": true, "tray_open": false, "type": "unknown"}]}
+{"return": [{"device": "disk", "locked": false, "removable": true, "inserted": {"iops_rd": 0, "detect_zeroes": "off", "image": {"virtual-size": 134217728, "filename": "TEST_DIR/t.qcow2", "cluster-size": 65536, "format": "qcow2", "actual-size": SIZE, "format-specific": {"type": "qcow2", "data": {"compat": "1.1", "lazy-refcounts": false}}, "dirty-flag": false}, "iops_wr": 0, "ro": false, "backing_file_depth": 0, "drv": "qcow2", "iops": 0, "bps_wr": 0, "encrypted": false, "bps": 0, "bps_rd": 0, "cache": {"no-flush": false, "direct": false, "writeback": true}, "file": "TEST_DIR/t.qcow2", "encryption_key_missing": false}, "tray_open": false, "type": "unknown"}, {"io-status": "ok", "device": "ide1-cd0", "locked": false, "removable": true, "tray_open": false, "type": "unknown"}, {"device": "floppy0", "locked": false, "removable": true, "tray_open": false, "type": "unknown"}, {"device": "sd0", "locked": false, "removable": true, "tray_open": false, "type": "unknown"}]}
 {"return": {}}
 {"return": {}}
 {"return": {}}
@@ -44,7 +44,7 @@ Testing:
 QMP_VERSION
 {"return": {}}
 {"return": "OK\r\n"}
-{"return": [{"io-status": "ok", "device": "ide1-cd0", "locked": false, "removable": true, "tray_open": false, "type": "unknown"}, {"device": "floppy0", "locked": false, "removable": true, "tray_open": false, "type": "unknown"}, {"device": "sd0", "locked": false, "removable": true, "tray_open": false, "type": "unknown"}, {"device": "disk", "locked": false, "removable": true, "inserted": {"iops_rd": 0, "detect_zeroes": "off", "image": {"virtual-size": 134217728, "filename": "TEST_DIR/t.qcow2", "cluster-size": 65536, "format": "qcow2", "actual-size": SIZE, "format-specific": {"type": "qcow2", "data": {"compat": "1.1", "lazy-refcounts": false}}, "dirty-flag": false}, "iops_wr": 0, "ro": false, "backing_file_depth": 0, "drv": "qcow2", "iops": 0, "bps_wr": 0, "encrypted": false, "bps": 0, "bps_rd": 0, "file": "TEST_DIR/t.qcow2", "encryption_key_missing": false}, "tray_open": false, "type": "unknown"}]}
+{"return": [{"io-status": "ok", "device": "ide1-cd0", "locked": false, "removable": true, "tray_open": false, "type": "unknown"}, {"device": "floppy0", "locked": false, "removable": true, "tray_open": false, "type": "unknown"}, {"device": "sd0", "locked": false, "removable": true, "tray_open": false, "type": "unknown"}, {"device": "disk", "locked": false, "removable": true, "inserted": {"iops_rd": 0, "detect_zeroes": "off", "image": {"virtual-size": 134217728, "filename": "TEST_DIR/t.qcow2", "cluster-size": 65536, "format": "qcow2", "actual-size": SIZE, "format-specific": {"type": "qcow2", "data": {"compat": "1.1", "lazy-refcounts": false}}, "dirty-flag": false}, "iops_wr": 0, "ro": false, "backing_file_depth": 0, "drv": "qcow2", "iops": 0, "bps_wr": 0, "encrypted": false, "bps": 0, "bps_rd": 0, "cache": {"no-flush": false, "direct": false, "writeback": true}, "file": "TEST_DIR/t.qcow2", "encryption_key_missing": false}, "tray_open": false, "type": "unknown"}]}
 {"return": {}}
 {"return": {}}
 {"return": {}}
@@ -64,14 +64,14 @@ Testing:
 QMP_VERSION
 {"return": {}}
 {"return": {}}
-{"return": [{"io-status": "ok", "device": "ide1-cd0", "locked": false, "removable": true, "tray_open": false, "type": "unknown"}, {"device": "floppy0", "locked": false, "removable": true, "tray_open": false, "type": "unknown"}, {"device": "sd0", "locked": false, "removable": true, "tray_open": false, "type": "unknown"}, {"device": "disk", "locked": false, "removable": true, "inserted": {"iops_rd": 0, "detect_zeroes": "off", "image": {"virtual-size": 134217728, "filename": "TEST_DIR/t.qcow2", "cluster-size": 65536, "format": "qcow2", "actual-size": SIZE, "format-specific": {"type": "qcow2", "data": {"compat": "1.1", "lazy-refcounts": false}}, "dirty-flag": false}, "iops_wr": 0, "ro": false, "backing_file_depth": 0, "drv": "qcow2", "iops": 0, "bps_wr": 0, "encrypted": false, "bps": 0, "bps_rd": 0, "file": "TEST_DIR/t.qcow2", "encryption_key_missing": false}, "tray_open": false, "type": "unknown"}]}
+{"return": [{"io-status": "ok", "device": "ide1-cd0", "locked": false, "removable": true, "tray_open": false, "type": "unknown"}, {"device": "floppy0", "locked": false, "removable": true, "tray_open": false, "type": "unknown"}, {"device": "sd0", "locked": false, "removable": true, "tray_open": false, "type": "unknown"}, {"device": "disk", "locked": false, "removable": true, "inserted": {"iops_rd": 0, "detect_zeroes": "off", "image": {"virtual-size": 134217728, "filename": "TEST_DIR/t.qcow2", "cluster-size": 65536, "format": "qcow2", "actual-size": SIZE, "format-specific": {"type": "qcow2", "data": {"compat": "1.1", "lazy-refcounts": false}}, "dirty-flag": false}, "iops_wr": 0, "ro": false, "backing_file_depth": 0, "drv": "qcow2", "iops": 0, "bps_wr": 0, "encrypted": false, "bps": 0, "bps_rd": 0, "cache": {"no-flush": false, "direct": false, "writeback": true}, "file": "TEST_DIR/t.qcow2", "encryption_key_missing": false}, "tray_open": false, "type": "unknown"}]}
 {"return": {}}
 {"return": {}}
 {"return": {}}
 {"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": "DEVICE_DELETED", "data": {"path": "/machine/peripheral/virtio0/virtio-backend"}}
 {"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": "DEVICE_DELETED", "data": {"device": "virtio0", "path": "/machine/peripheral/virtio0"}}
 {"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": "RESET"}
-{"return": [{"io-status": "ok", "device": "ide1-cd0", "locked": false, "removable": true, "tray_open": false, "type": "unknown"}, {"device": "floppy0", "locked": false, "removable": true, "tray_open": false, "type": "unknown"}, {"device": "sd0", "locked": false, "removable": true, "tray_open": false, "type": "unknown"}, {"io-status": "ok", "device": "disk", "locked": false, "removable": true, "inserted": {"iops_rd": 0, "detect_zeroes": "off", "image": {"virtual-size": 134217728, "filename": "TEST_DIR/t.qcow2", "cluster-size": 65536, "format": "qcow2", "actual-size": SIZE, "format-specific": {"type": "qcow2", "data": {"compat": "1.1", "lazy-refcounts": false}}, "dirty-flag": false}, "iops_wr": 0, "ro": false, "backing_file_depth": 0, "drv": "qcow2", "iops": 0, "bps_wr": 0, "encrypted": false, "bps": 0, "bps_rd": 0, "file": "TEST_DIR/t.qcow2", "encryption_key_missing": false}, "tray_open": false, "type": "unknown"}]}
+{"return": [{"io-status": "ok", "device": "ide1-cd0", "locked": false, "removable": true, "tray_open": false, "type": "unknown"}, {"device": "floppy0", "locked": false, "removable": true, "tray_open": false, "type": "unknown"}, {"device": "sd0", "locked": false, "removable": true, "tray_open": false, "type": "unknown"}, {"io-status": "ok", "device": "disk", "locked": false, "removable": true, "inserted": {"iops_rd": 0, "detect_zeroes": "off", "image": {"virtual-size": 134217728, "filename": "TEST_DIR/t.qcow2", "cluster-size": 65536, "format": "qcow2", "actual-size": SIZE, "format-specific": {"type": "qcow2", "data": {"compat": "1.1", "lazy-refcounts": false}}, "dirty-flag": false}, "iops_wr": 0, "ro": false, "backing_file_depth": 0, "drv": "qcow2", "iops": 0, "bps_wr": 0, "encrypted": false, "bps": 0, "bps_rd": 0, "cache": {"no-flush": false, "direct": false, "writeback": true}, "file": "TEST_DIR/t.qcow2", "encryption_key_missing": false}, "tray_open": false, "type": "unkno!
 wn"}]}
 {"return": {}}
 {"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": "SHUTDOWN"}
 {"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": "DEVICE_TRAY_MOVED", "data": {"device": "ide1-cd0", "tray-open": true}}
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH 1/6] block/qapi: Add cache information to query-block

[Markus Armbruster]

Kevin Wolf <kwolf@redhat.com> writes:

> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  block/qapi.c               | 10 ++++++++++
>  hmp.c                      |  8 ++++++++
>  qapi/block-core.json       |  4 +++-
>  tests/qemu-iotests/051.out |  1 +
>  tests/qemu-iotests/067.out | 10 +++++-----
>  5 files changed, 27 insertions(+), 6 deletions(-)
>
> diff --git a/block/qapi.c b/block/qapi.c
> index 9733ebd..6b43bc3 100644
> --- a/block/qapi.c
> +++ b/block/qapi.c
> @@ -46,6 +46,16 @@ BlockDeviceInfo *bdrv_block_device_info(BlockDriverState *bs)
>      info->encrypted              = bs->encrypted;
>      info->encryption_key_missing = bdrv_key_required(bs);
>  
> +    info->cache = g_new(BlockdevCacheOptions, 1);
> +    *info->cache = (BlockdevCacheOptions) {
> +        .writeback      = bdrv_enable_write_cache(bs),
> +        .has_writeback  = true,
> +        .direct         = !!(bs->open_flags & BDRV_O_NOCACHE),
> +        .has_direct     = true,
> +        .no_flush       = !!(bs->open_flags & BDRV_O_NO_FLUSH),
> +        .has_no_flush   = true,
> +    };
> +

Awkward optionality.  I'm going to discuss this below, in the context of
the schema change.

>      if (bs->node_name[0]) {
>          info->has_node_name = true;
>          info->node_name = g_strdup(bs->node_name);
> diff --git a/hmp.c b/hmp.c
> index 40a90da..c3846b8 100644
> --- a/hmp.c
> +++ b/hmp.c
> @@ -294,6 +294,7 @@ void hmp_info_block(Monitor *mon, const QDict *qdict)
>  {
>      BlockInfoList *block_list, *info;
>      ImageInfo *image_info;
> +    BlockDeviceInfo *inserted;
>      const char *device = qdict_get_try_str(qdict, "device");
>      bool verbose = qdict_get_try_bool(qdict, "verbose", 0);
>  
> @@ -335,6 +336,13 @@ void hmp_info_block(Monitor *mon, const QDict *qdict)
>              continue;
>          }
>  
> +        inserted = info->value->inserted;
> +
> +        monitor_printf(mon, "    Cache mode:       %s%s%s\n",
> +                       inserted->cache->writeback ? "writeback" : "writethrough",
> +                       inserted->cache->direct ? ", direct" : "",
> +                       inserted->cache->no_flush ? ", ignore flushes" : "");
> +

If !inserted->cache->writeback && inserted->cache->direct, this prints
"    Cache mode:       , writeback".

>          if (info->value->inserted->has_backing_file) {
>              monitor_printf(mon,
>                             "    Backing file:     %s "
> diff --git a/qapi/block-core.json b/qapi/block-core.json
> index 95dcd81..c53b587 100644
> --- a/qapi/block-core.json
> +++ b/qapi/block-core.json
> @@ -238,6 +238,8 @@
>  #
>  # @iops_size: #optional an I/O size in bytes (Since 1.7)
>  #
> +# @cache: the cache mode used for the block device (since: 2.2)
> +#
>  # Since: 0.14.0
>  #
>  ##
> @@ -252,7 +254,7 @@
>              '*bps_max': 'int', '*bps_rd_max': 'int',
>              '*bps_wr_max': 'int', '*iops_max': 'int',
>              '*iops_rd_max': 'int', '*iops_wr_max': 'int',
> -            '*iops_size': 'int' } }
> +            '*iops_size': 'int', 'cache': 'BlockdevCacheOptions' } }
>  
>  ##
>  # @BlockDeviceIoStatus:

Before this patch, QAPI type BlockdevCacheOptions is used only as a
member of BlockdevOptionsBase.

BlockdevOptionsBase is a collection configuration settings.
Consequently, it's a complex type whose members are optional exactly
when the configuration setting is optional.  Makes sense.

Complication: a few options are wrapped in another complex type,
BlockdevCacheOptions.  It's optional, but that's not sufficient, it's
members are all optional, too.

BlockdevCacheOptions is the only complex member of BlockdevOptionsBase.
The others are all simple or enum.

In this patch, you reuse BlockdevCacheOptions for a purpose other than
configuration: *querying* configuration.  In the query result, neither
the complex type nor its members are optional.  The schema reflects the
former (the patch hunk has 'cache', not '*cache'), but not the latter.

Do we want the schema to reflect reality accurately?

If no, we should still document the places where it doesn't, like this
one.

If yes, how can we fix this particular inaccuracy?

The obvious solution is not to reuse BlockdevCacheOptions.  Create a
second type, identical except for its members aren't optional.

Another idea is to add means to the schema to declare a member "deep
optional": not just the member is optional, but if it's present, its
members are optional, too.  Begs the question whether its member's
member's are optional.  Hmm.

Yet another idea is to declare nesting configuration options stupid, and
just not do it, but it may be too late for that.

Other ideas?

[...]

Re: [Qemu-devel] [PATCH 1/6] block/qapi: Add cache information to query-block

[Markus Armbruster]

Markus Armbruster <armbru@redhat.com> writes:

> Kevin Wolf <kwolf@redhat.com> writes:
[...]
>> @@ -335,6 +336,13 @@ void hmp_info_block(Monitor *mon, const QDict *qdict)
>>              continue;
>>          }
>>  
>> +        inserted = info->value->inserted;
>> +
>> +        monitor_printf(mon, "    Cache mode:       %s%s%s\n",
>> +                       inserted->cache->writeback ? "writeback" : "writethrough",
>> +                       inserted->cache->direct ? ", direct" : "",
>> +                       inserted->cache->no_flush ? ", ignore flushes" : "");
>> +
>
> If !inserted->cache->writeback && inserted->cache->direct, this prints
> "    Cache mode:       , writeback".

Nevermind, I can't read.

[...]

Re: [Qemu-devel] [PATCH 1/6] block/qapi: Add cache information to query-block

[Kevin Wolf]

Am 18.09.2014 um 13:04 hat Markus Armbruster geschrieben:
> Before this patch, QAPI type BlockdevCacheOptions is used only as a
> member of BlockdevOptionsBase.
> 
> BlockdevOptionsBase is a collection configuration settings.
> Consequently, it's a complex type whose members are optional exactly
> when the configuration setting is optional.  Makes sense.
> 
> Complication: a few options are wrapped in another complex type,
> BlockdevCacheOptions.  It's optional, but that's not sufficient, it's
> members are all optional, too.
> 
> BlockdevCacheOptions is the only complex member of BlockdevOptionsBase.
> The others are all simple or enum.
> 
> In this patch, you reuse BlockdevCacheOptions for a purpose other than
> configuration: *querying* configuration.  In the query result, neither
> the complex type nor its members are optional.  The schema reflects the
> former (the patch hunk has 'cache', not '*cache'), but not the latter.
> 
> Do we want the schema to reflect reality accurately?
> 
> If no, we should still document the places where it doesn't, like this
> one.

That's a fair requirement. If anything is optional in a return value, we
should specify the conditions under which it is there or missing.
Including, of course, if it's always or never there.

> If yes, how can we fix this particular inaccuracy?
> 
> The obvious solution is not to reuse BlockdevCacheOptions.  Create a
> second type, identical except for its members aren't optional.

I can introduce a BlockdevCacheInfo instead. While I'm not completely
excited about having two structs for each option dict (one for
configuring, one for querying), there's precedence for this and it's at
least a small struct this time.

> Another idea is to add means to the schema to declare a member "deep
> optional": not just the member is optional, but if it's present, its
> members are optional, too.  Begs the question whether its member's
> member's are optional.  Hmm.

"deep optional" doesn't sound like it makes a lot of sense conceptually,
even if it might happen to be a hack that gets us the right result in
this one specific case.

> Yet another idea is to declare nesting configuration options stupid, and
> just not do it, but it may be too late for that.
> 
> Other ideas?

I think the choice is between adding BlockdevCacheInfo and changing
documentation while reusing BlockdevCacheOptions. Both are fine with me.
Which one do you prefer?

Kevin

Re: [Qemu-devel] [PATCH 1/6] block/qapi: Add cache information to query-block

[Markus Armbruster]

Kevin Wolf <kwolf@redhat.com> writes:

> Am 18.09.2014 um 13:04 hat Markus Armbruster geschrieben:
>> Before this patch, QAPI type BlockdevCacheOptions is used only as a
>> member of BlockdevOptionsBase.
>> 
>> BlockdevOptionsBase is a collection configuration settings.
>> Consequently, it's a complex type whose members are optional exactly
>> when the configuration setting is optional.  Makes sense.
>> 
>> Complication: a few options are wrapped in another complex type,
>> BlockdevCacheOptions.  It's optional, but that's not sufficient, it's
>> members are all optional, too.
>> 
>> BlockdevCacheOptions is the only complex member of BlockdevOptionsBase.
>> The others are all simple or enum.
>> 
>> In this patch, you reuse BlockdevCacheOptions for a purpose other than
>> configuration: *querying* configuration.  In the query result, neither
>> the complex type nor its members are optional.  The schema reflects the
>> former (the patch hunk has 'cache', not '*cache'), but not the latter.
>> 
>> Do we want the schema to reflect reality accurately?
>> 
>> If no, we should still document the places where it doesn't, like this
>> one.
>
> That's a fair requirement. If anything is optional in a return value, we
> should specify the conditions under which it is there or missing.
> Including, of course, if it's always or never there.

Writing documentation saying an optional member is in fact not optional
feels weird, and saying it's never there feels even weirder :)

>> If yes, how can we fix this particular inaccuracy?
>> 
>> The obvious solution is not to reuse BlockdevCacheOptions.  Create a
>> second type, identical except for its members aren't optional.
>
> I can introduce a BlockdevCacheInfo instead. While I'm not completely
> excited about having two structs for each option dict (one for
> configuring, one for querying), there's precedence for this and it's at
> least a small struct this time.

I'm not excited about the additional types, either.

>> Another idea is to add means to the schema to declare a member "deep
>> optional": not just the member is optional, but if it's present, its
>> members are optional, too.  Begs the question whether its member's
>> member's are optional.  Hmm.
>
> "deep optional" doesn't sound like it makes a lot of sense conceptually,
> even if it might happen to be a hack that gets us the right result in
> this one specific case.
>
>> Yet another idea is to declare nesting configuration options stupid, and
>> just not do it, but it may be too late for that.
>> 
>> Other ideas?
>
> I think the choice is between adding BlockdevCacheInfo and changing
> documentation while reusing BlockdevCacheOptions. Both are fine with me.
> Which one do you prefer?

I'm leaning towards adding BlockdevCacheInfo, because you say there's
precedence, and because I like the schema to be accurate more than I
dislike the additional type.

Re: [Qemu-devel] [PATCH 1/6] block/qapi: Add cache information to query-block

[Eric Blake]

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

On 09/18/2014 05:53 AM, Kevin Wolf wrote:
> Am 18.09.2014 um 13:04 hat Markus Armbruster geschrieben:
>> Before this patch, QAPI type BlockdevCacheOptions is used only as a
>> member of BlockdevOptionsBase.
>>
>> BlockdevOptionsBase is a collection configuration settings.
>> Consequently, it's a complex type whose members are optional exactly
>> when the configuration setting is optional.  Makes sense.
>>
>> Complication: a few options are wrapped in another complex type,
>> BlockdevCacheOptions.  It's optional, but that's not sufficient, it's
>> members are all optional, too.
>>

> I think the choice is between adding BlockdevCacheInfo and changing
> documentation while reusing BlockdevCacheOptions. Both are fine with me.
> Which one do you prefer?

Those two options sound the best to me as well; I'm fine with either,
although I have a slight preference for just documenting on
BlockdevCacheOptions that members are optional on input but will all be
present when generated as output.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 539 bytes --]

[Qemu-devel] [PATCH 2/6] block: Add optional device argument to query-block

[Kevin Wolf]

This allows querying one specific BlockDriverState instead of a list of
all drives.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 block/qapi.c         | 15 +++++++++++++--
 hmp.c                |  6 +++---
 qapi/block-core.json |  8 ++++++--
 3 files changed, 22 insertions(+), 7 deletions(-)

diff --git a/block/qapi.c b/block/qapi.c
index 6b43bc3..2a060d3 100644
--- a/block/qapi.c
+++ b/block/qapi.c
@@ -367,13 +367,24 @@ static BlockStats *bdrv_query_stats(const BlockDriverState *bs)
     return s;
 }
 
-BlockInfoList *qmp_query_block(Error **errp)
+BlockInfoList *qmp_query_block(bool has_device, const char *device,
+                               Error **errp)
 {
     BlockInfoList *head = NULL, **p_next = &head;
     BlockDriverState *bs = NULL;
     Error *local_err = NULL;
 
-     while ((bs = bdrv_next(bs))) {
+    if (has_device) {
+        bs = bdrv_find(device);
+        if (bs == NULL) {
+            error_setg(errp, "Device not found");
+            goto err;
+        }
+    } else {
+        bs = bdrv_next(NULL);
+    }
+
+    for (; bs; (bs = has_device ? NULL : bdrv_next(bs))) {
         BlockInfoList *info = g_malloc0(sizeof(*info));
         bdrv_query_info(bs, &info->value, &local_err);
         if (local_err) {
diff --git a/hmp.c b/hmp.c
index c3846b8..02eee80 100644
--- a/hmp.c
+++ b/hmp.c
@@ -298,7 +298,7 @@ void hmp_info_block(Monitor *mon, const QDict *qdict)
     const char *device = qdict_get_try_str(qdict, "device");
     bool verbose = qdict_get_try_bool(qdict, "verbose", 0);
 
-    block_list = qmp_query_block(NULL);
+    block_list = qmp_query_block(false, NULL, NULL);
 
     for (info = block_list; info; info = info->next) {
         if (device && strcmp(device, info->value->device)) {
@@ -847,7 +847,7 @@ void hmp_cont(Monitor *mon, const QDict *qdict)
     BlockInfoList *bdev_list, *bdev;
     Error *err = NULL;
 
-    bdev_list = qmp_query_block(NULL);
+    bdev_list = qmp_query_block(false, NULL, NULL);
     for (bdev = bdev_list; bdev; bdev = bdev->next) {
         if (key_is_missing(bdev->value)) {
             monitor_read_block_device_key(mon, bdev->value->device,
@@ -1588,7 +1588,7 @@ void hmp_nbd_server_start(Monitor *mon, const QDict *qdict)
     /* Then try adding all block devices.  If one fails, close all and
      * exit.
      */
-    block_list = qmp_query_block(NULL);
+    block_list = qmp_query_block(false, NULL, NULL);
 
     for (info = block_list; info; info = info->next) {
         if (!info->value->has_inserted) {
diff --git a/qapi/block-core.json b/qapi/block-core.json
index c53b587..ddc3fe0 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -354,13 +354,17 @@
 ##
 # @query-block:
 #
-# Get a list of BlockInfo for all virtual block devices.
+# Get a list of BlockInfo for all or one specific virtual block device.
+#
+# @device: #optional The id of the block device to inspect. (Since 2.2)
 #
 # Returns: a list of @BlockInfo describing each virtual block device
 #
 # Since: 0.14.0
 ##
-{ 'command': 'query-block', 'returns': ['BlockInfo'] }
+{ 'command': 'query-block',
+  'data': { '*device': 'str' },
+  'returns': ['BlockInfo'] }
 
 ##
 # @BlockDeviceStats:
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH 2/6] block: Add optional device argument to query-block

[Markus Armbruster]

Kevin Wolf <kwolf@redhat.com> writes:

> This allows querying one specific BlockDriverState instead of a list of
> all drives.

I've long resisted optional arguments to limit list-valued queries,
because I believe the additional interface complexity isn't worth the
additional utility.  But resistance might be futile.

We have one only query- with an optional limiting arguement[*]:
query-rx-filter.

If we go down this route, then I'd prefer to have such optional limiting
for all query- commands returning lists of elements describing some
aspect of a named object.  Preference != demand.

>
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  block/qapi.c         | 15 +++++++++++++--
>  hmp.c                |  6 +++---
>  qapi/block-core.json |  8 ++++++--
>  3 files changed, 22 insertions(+), 7 deletions(-)
>
> diff --git a/block/qapi.c b/block/qapi.c
> index 6b43bc3..2a060d3 100644
> --- a/block/qapi.c
> +++ b/block/qapi.c
> @@ -367,13 +367,24 @@ static BlockStats *bdrv_query_stats(const BlockDriverState *bs)
>      return s;
>  }
>  
> -BlockInfoList *qmp_query_block(Error **errp)
> +BlockInfoList *qmp_query_block(bool has_device, const char *device,
> +                               Error **errp)
>  {
>      BlockInfoList *head = NULL, **p_next = &head;
>      BlockDriverState *bs = NULL;
>      Error *local_err = NULL;
>  
> -     while ((bs = bdrv_next(bs))) {
> +    if (has_device) {
> +        bs = bdrv_find(device);
> +        if (bs == NULL) {
> +            error_setg(errp, "Device not found");
> +            goto err;
> +        }
> +    } else {
> +        bs = bdrv_next(NULL);
> +    }
> +
> +    for (; bs; (bs = has_device ? NULL : bdrv_next(bs))) {
>          BlockInfoList *info = g_malloc0(sizeof(*info));
>          bdrv_query_info(bs, &info->value, &local_err);
>          if (local_err) {

I'd find

    for (bs = bdrv_next(NULL); bs; bs == bdrv_next(bs)) {
        if (device && strcmp(device, bs->device)) {
            continue;
        }
    }

simpler and clearer.  It's how qmp_query_rx_filter() works.

> diff --git a/hmp.c b/hmp.c
> index c3846b8..02eee80 100644
> --- a/hmp.c
> +++ b/hmp.c
> @@ -298,7 +298,7 @@ void hmp_info_block(Monitor *mon, const QDict *qdict)
>      const char *device = qdict_get_try_str(qdict, "device");
>      bool verbose = qdict_get_try_bool(qdict, "verbose", 0);
>  
> -    block_list = qmp_query_block(NULL);
> +    block_list = qmp_query_block(false, NULL, NULL);
>  
>      for (info = block_list; info; info = info->next) {
>          if (device && strcmp(device, info->value->device)) {
> @@ -847,7 +847,7 @@ void hmp_cont(Monitor *mon, const QDict *qdict)
>      BlockInfoList *bdev_list, *bdev;
>      Error *err = NULL;
>  
> -    bdev_list = qmp_query_block(NULL);
> +    bdev_list = qmp_query_block(false, NULL, NULL);
>      for (bdev = bdev_list; bdev; bdev = bdev->next) {
>          if (key_is_missing(bdev->value)) {
>              monitor_read_block_device_key(mon, bdev->value->device,
> @@ -1588,7 +1588,7 @@ void hmp_nbd_server_start(Monitor *mon, const QDict *qdict)
>      /* Then try adding all block devices.  If one fails, close all and
>       * exit.
>       */
> -    block_list = qmp_query_block(NULL);
> +    block_list = qmp_query_block(false, NULL, NULL);
>  
>      for (info = block_list; info; info = info->next) {
>          if (!info->value->has_inserted) {
> diff --git a/qapi/block-core.json b/qapi/block-core.json
> index c53b587..ddc3fe0 100644
> --- a/qapi/block-core.json
> +++ b/qapi/block-core.json
> @@ -354,13 +354,17 @@
>  ##
>  # @query-block:
>  #
> -# Get a list of BlockInfo for all virtual block devices.
> +# Get a list of BlockInfo for all or one specific virtual block device.
> +#
> +# @device: #optional The id of the block device to inspect. (Since 2.2)
>  #
>  # Returns: a list of @BlockInfo describing each virtual block device
>  #
>  # Since: 0.14.0
>  ##

This is how we document query-rx-filter:

##
# @query-rx-filter:
#
# Return rx-filter information for all NICs (or for the given NIC).
#
# @name: #optional net client name
#
# Returns: list of @RxFilterInfo for all NICs (or for the given NIC).
#          Returns an error if the given @name doesn't exist, or given
#          NIC doesn't support rx-filter querying, or given net client
#          isn't a NIC.
#
# Since: 1.6
##

I find that slightly clearer.  Perhaps you'd like to steal from it.

> -{ 'command': 'query-block', 'returns': ['BlockInfo'] }
> +{ 'command': 'query-block',
> +  'data': { '*device': 'str' },
> +  'returns': ['BlockInfo'] }
>  
>  ##
>  # @BlockDeviceStats:


[*] Yes, I objected to the argument, but I didn't insist.  I won't
insist now.

[Qemu-devel] [PATCH 3/6] block: Introduce query-block-node

[Kevin Wolf]

This new command is a renamed version of query-named-block-nodes
with an additional argument that allows querying only one specific
BlockDriverState instead of getting a list for all of them.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 block.c               | 15 +++++++++++++--
 blockdev.c            |  8 +++++++-
 include/block/block.h |  2 +-
 qapi/block-core.json  | 19 ++++++++++++++++---
 4 files changed, 37 insertions(+), 7 deletions(-)

diff --git a/block.c b/block.c
index bcd952a..6c538d3 100644
--- a/block.c
+++ b/block.c
@@ -3823,13 +3823,24 @@ 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(const char *device, Error **errp)
 {
     BlockDeviceInfoList *list, *entry;
     BlockDriverState *bs;
+    Error *local_err = NULL;
+
+    if (device) {
+        bs = bdrv_lookup_bs(device, device, &local_err);
+        if (local_err) {
+            error_propagate(errp, local_err);
+            return NULL;
+        }
+    } else {
+        bs = QTAILQ_FIRST(&graph_bdrv_states);
+    }
 
     list = NULL;
-    QTAILQ_FOREACH(bs, &graph_bdrv_states, node_list) {
+    for (; bs; bs = device ? NULL : QTAILQ_NEXT(bs, node_list)) {
         entry = g_malloc0(sizeof(*entry));
         entry->value = bdrv_block_device_info(bs);
         entry->next = list;
diff --git a/blockdev.c b/blockdev.c
index b361fbb..a194d04 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -2127,9 +2127,15 @@ void qmp_drive_backup(const char *device, const char *target,
     }
 }
 
+BlockDeviceInfoList *qmp_query_block_node(bool has_device, const char *device,
+                                          Error **errp)
+{
+    return bdrv_named_nodes_list(has_device ? device : NULL, errp);
+}
+
 BlockDeviceInfoList *qmp_query_named_block_nodes(Error **errp)
 {
-    return bdrv_named_nodes_list();
+    return qmp_query_block_node(false, NULL, errp);
 }
 
 #define DEFAULT_MIRROR_BUF_SIZE   (10 << 20)
diff --git a/include/block/block.h b/include/block/block.h
index 07d6d8e..0caab0d 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -403,7 +403,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);
+BlockDeviceInfoList *bdrv_named_nodes_list(const char *device, Error **errp);
 BlockDriverState *bdrv_lookup_bs(const char *device,
                                  const char *node_name,
                                  Error **errp);
diff --git a/qapi/block-core.json b/qapi/block-core.json
index ddc3fe0..f170c7e 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -797,13 +797,26 @@
 ##
 # @query-named-block-nodes
 #
-# Get the named block driver list
+# Deprecated, may be removed in future versions. Use query-block-node instead.
+#
+# Since 2.0, until 2.1
+##
+{ 'command': 'query-named-block-nodes', 'returns': [ 'BlockDeviceInfo' ] }
+
+##
+# @query-block-node
+#
+# Gets information about one or all block device nodes.
+#
+# @device: #optional The id or node-name of the block device to inspect.
 #
 # Returns: the list of BlockDeviceInfo
 #
-# Since 2.0
+# Since 2.2
 ##
-{ 'command': 'query-named-block-nodes', 'returns': [ 'BlockDeviceInfo' ] }
+{ 'command': 'query-block-node',
+  'data': { '*device': 'str' },
+  'returns': [ 'BlockDeviceInfo' ] }
 
 ##
 # @drive-mirror
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH 3/6] block: Introduce query-block-node

[Markus Armbruster]

Kevin Wolf <kwolf@redhat.com> writes:

> This new command is a renamed version of query-named-block-nodes
> with an additional argument that allows querying only one specific
> BlockDriverState instead of getting a list for all of them.
>
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  block.c               | 15 +++++++++++++--
>  blockdev.c            |  8 +++++++-
>  include/block/block.h |  2 +-
>  qapi/block-core.json  | 19 ++++++++++++++++---
>  4 files changed, 37 insertions(+), 7 deletions(-)
>
> diff --git a/block.c b/block.c
> index bcd952a..6c538d3 100644
> --- a/block.c
> +++ b/block.c
> @@ -3823,13 +3823,24 @@ 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(const char *device, Error **errp)
>  {
>      BlockDeviceInfoList *list, *entry;
>      BlockDriverState *bs;
> +    Error *local_err = NULL;
> +
> +    if (device) {
> +        bs = bdrv_lookup_bs(device, device, &local_err);
> +        if (local_err) {
> +            error_propagate(errp, local_err);
> +            return NULL;
> +        }
> +    } else {
> +        bs = QTAILQ_FIRST(&graph_bdrv_states);
> +    }
>  
>      list = NULL;
> -    QTAILQ_FOREACH(bs, &graph_bdrv_states, node_list) {
> +    for (; bs; bs = device ? NULL : QTAILQ_NEXT(bs, node_list)) {
>          entry = g_malloc0(sizeof(*entry));
>          entry->value = bdrv_block_device_info(bs);
>          entry->next = list;

Same loop as in PATCH 2/6, same suggestion to simplify it.

> diff --git a/blockdev.c b/blockdev.c
> index b361fbb..a194d04 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -2127,9 +2127,15 @@ void qmp_drive_backup(const char *device, const char *target,
>      }
>  }
>  
> +BlockDeviceInfoList *qmp_query_block_node(bool has_device, const char *device,
> +                                          Error **errp)
> +{
> +    return bdrv_named_nodes_list(has_device ? device : NULL, errp);
> +}
> +
>  BlockDeviceInfoList *qmp_query_named_block_nodes(Error **errp)
>  {
> -    return bdrv_named_nodes_list();
> +    return qmp_query_block_node(false, NULL, errp);
>  }
>  
>  #define DEFAULT_MIRROR_BUF_SIZE   (10 << 20)
> diff --git a/include/block/block.h b/include/block/block.h
> index 07d6d8e..0caab0d 100644
> --- a/include/block/block.h
> +++ b/include/block/block.h
> @@ -403,7 +403,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);
> +BlockDeviceInfoList *bdrv_named_nodes_list(const char *device, Error **errp);
>  BlockDriverState *bdrv_lookup_bs(const char *device,
>                                   const char *node_name,
>                                   Error **errp);
> diff --git a/qapi/block-core.json b/qapi/block-core.json
> index ddc3fe0..f170c7e 100644
> --- a/qapi/block-core.json
> +++ b/qapi/block-core.json
> @@ -797,13 +797,26 @@
>  ##
>  # @query-named-block-nodes
>  #
> -# Get the named block driver list
> +# Deprecated, may be removed in future versions. Use query-block-node instead.
> +#
> +# Since 2.0, until 2.1

Suggest

 ##
 # @query-named-block-nodes
 #
 # Get the named block driver list
 #
 # Since 2.0
+# Deprecated since 2.1, use query-block-node instead

and a general understanding that "deprecated" implies "should not be
used anymore" and "may be removed in a future release".

> +##
> +{ 'command': 'query-named-block-nodes', 'returns': [ 'BlockDeviceInfo' ] }
> +
> +##
> +# @query-block-node
> +#
> +# Gets information about one or all block device nodes.
> +#
> +# @device: #optional The id or node-name of the block device to inspect.
>  #
>  # Returns: the list of BlockDeviceInfo
>  #
> -# Since 2.0
> +# Since 2.2
>  ##
> -{ 'command': 'query-named-block-nodes', 'returns': [ 'BlockDeviceInfo' ] }
> +{ 'command': 'query-block-node',
> +  'data': { '*device': 'str' },
> +  'returns': [ 'BlockDeviceInfo' ] }
>  
>  ##
>  # @drive-mirror

[Qemu-devel] [PATCH 4/6] block/hmp: Factor out print_block_info()

[Kevin Wolf]

The new function prints the info for a single BlockDriverState.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 hmp.c | 192 +++++++++++++++++++++++++++++++++---------------------------------
 1 file changed, 97 insertions(+), 95 deletions(-)

diff --git a/hmp.c b/hmp.c
index 02eee80..be55b9b 100644
--- a/hmp.c
+++ b/hmp.c
@@ -290,118 +290,120 @@ void hmp_info_cpus(Monitor *mon, const QDict *qdict)
     qapi_free_CpuInfoList(cpu_list);
 }
 
-void hmp_info_block(Monitor *mon, const QDict *qdict)
+static void print_block_info(Monitor *mon, BlockInfo *info,
+                             BlockDeviceInfo *inserted, bool verbose)
 {
-    BlockInfoList *block_list, *info;
     ImageInfo *image_info;
-    BlockDeviceInfo *inserted;
-    const char *device = qdict_get_try_str(qdict, "device");
-    bool verbose = qdict_get_try_bool(qdict, "verbose", 0);
-
-    block_list = qmp_query_block(false, NULL, NULL);
 
-    for (info = block_list; info; info = info->next) {
-        if (device && strcmp(device, info->value->device)) {
-            continue;
-        }
+    monitor_printf(mon, "%s", info->device);
+    if (inserted) {
+        monitor_printf(mon, ": %s (%s%s%s)\n",
+                       inserted->file,
+                       inserted->drv,
+                       inserted->ro ? ", read-only" : "",
+                       inserted->encrypted ? ", encrypted" : "");
+    } else {
+        monitor_printf(mon, ": [not inserted]\n");
+    }
 
-        if (info != block_list) {
-            monitor_printf(mon, "\n");
-        }
+    if (info->has_io_status && info->io_status != BLOCK_DEVICE_IO_STATUS_OK) {
+        monitor_printf(mon, "    I/O status:       %s\n",
+                       BlockDeviceIoStatus_lookup[info->io_status]);
+    }
 
-        monitor_printf(mon, "%s", info->value->device);
-        if (info->value->has_inserted) {
-            monitor_printf(mon, ": %s (%s%s%s)\n",
-                           info->value->inserted->file,
-                           info->value->inserted->drv,
-                           info->value->inserted->ro ? ", read-only" : "",
-                           info->value->inserted->encrypted ? ", encrypted" : "");
-        } else {
-            monitor_printf(mon, ": [not inserted]\n");
-        }
+    if (info->removable) {
+        monitor_printf(mon, "    Removable device: %slocked, tray %s\n",
+                       info->locked ? "" : "not ",
+                       info->tray_open ? "open" : "closed");
+    }
 
-        if (info->value->has_io_status && info->value->io_status != BLOCK_DEVICE_IO_STATUS_OK) {
-            monitor_printf(mon, "    I/O status:       %s\n",
-                           BlockDeviceIoStatus_lookup[info->value->io_status]);
-        }
 
-        if (info->value->removable) {
-            monitor_printf(mon, "    Removable device: %slocked, tray %s\n",
-                           info->value->locked ? "" : "not ",
-                           info->value->tray_open ? "open" : "closed");
-        }
+    if (!inserted) {
+        return;
+    }
 
+    monitor_printf(mon, "    Cache mode:       %s%s%s\n",
+                   inserted->cache->writeback ? "writeback" : "writethrough",
+                   inserted->cache->direct ? ", direct" : "",
+                   inserted->cache->no_flush ? ", ignore flushes" : "");
 
-        if (!info->value->has_inserted) {
-            continue;
+    if (inserted->has_backing_file) {
+        monitor_printf(mon,
+                       "    Backing file:     %s "
+                       "(chain depth: %" PRId64 ")\n",
+                       inserted->backing_file,
+                       inserted->backing_file_depth);
+    }
+
+    if (inserted->detect_zeroes != BLOCKDEV_DETECT_ZEROES_OPTIONS_OFF) {
+        monitor_printf(mon, "    Detect zeroes:    %s\n",
+                       BlockdevDetectZeroesOptions_lookup[inserted->detect_zeroes]);
+    }
+
+    if (inserted->bps  || inserted->bps_rd  || inserted->bps_wr  ||
+        inserted->iops || inserted->iops_rd || inserted->iops_wr)
+    {
+        monitor_printf(mon, "    I/O throttling:   bps=%" PRId64
+                        " bps_rd=%" PRId64  " bps_wr=%" PRId64
+                        " bps_max=%" PRId64
+                        " bps_rd_max=%" PRId64
+                        " bps_wr_max=%" PRId64
+                        " iops=%" PRId64 " iops_rd=%" PRId64
+                        " iops_wr=%" PRId64
+                        " iops_max=%" PRId64
+                        " iops_rd_max=%" PRId64
+                        " iops_wr_max=%" PRId64
+                        " iops_size=%" PRId64 "\n",
+                        inserted->bps,
+                        inserted->bps_rd,
+                        inserted->bps_wr,
+                        inserted->bps_max,
+                        inserted->bps_rd_max,
+                        inserted->bps_wr_max,
+                        inserted->iops,
+                        inserted->iops_rd,
+                        inserted->iops_wr,
+                        inserted->iops_max,
+                        inserted->iops_rd_max,
+                        inserted->iops_wr_max,
+                        inserted->iops_size);
+    }
+
+    if (verbose) {
+        monitor_printf(mon, "\nImages:\n");
+        image_info = inserted->image;
+        while (1) {
+                bdrv_image_info_dump((fprintf_function)monitor_printf,
+                                     mon, image_info);
+            if (image_info->has_backing_image) {
+                image_info = image_info->backing_image;
+            } else {
+                break;
+            }
         }
+    }
+}
 
-        inserted = info->value->inserted;
-
-        monitor_printf(mon, "    Cache mode:       %s%s%s\n",
-                       inserted->cache->writeback ? "writeback" : "writethrough",
-                       inserted->cache->direct ? ", direct" : "",
-                       inserted->cache->no_flush ? ", ignore flushes" : "");
+void hmp_info_block(Monitor *mon, const QDict *qdict)
+{
+    BlockInfoList *block_list, *info;
+    const char *device = qdict_get_try_str(qdict, "device");
+    bool verbose = qdict_get_try_bool(qdict, "verbose", 0);
 
-        if (info->value->inserted->has_backing_file) {
-            monitor_printf(mon,
-                           "    Backing file:     %s "
-                           "(chain depth: %" PRId64 ")\n",
-                           info->value->inserted->backing_file,
-                           info->value->inserted->backing_file_depth);
-        }
+    block_list = qmp_query_block(false, NULL, NULL);
 
-        if (info->value->inserted->detect_zeroes != BLOCKDEV_DETECT_ZEROES_OPTIONS_OFF) {
-            monitor_printf(mon, "    Detect zeroes:    %s\n",
-                           BlockdevDetectZeroesOptions_lookup[info->value->inserted->detect_zeroes]);
+    for (info = block_list; info; info = info->next) {
+        if (device && strcmp(device, info->value->device)) {
+            continue;
         }
 
-        if (info->value->inserted->bps
-            || info->value->inserted->bps_rd
-            || info->value->inserted->bps_wr
-            || info->value->inserted->iops
-            || info->value->inserted->iops_rd
-            || info->value->inserted->iops_wr)
-        {
-            monitor_printf(mon, "    I/O throttling:   bps=%" PRId64
-                            " bps_rd=%" PRId64  " bps_wr=%" PRId64
-                            " bps_max=%" PRId64
-                            " bps_rd_max=%" PRId64
-                            " bps_wr_max=%" PRId64
-                            " iops=%" PRId64 " iops_rd=%" PRId64
-                            " iops_wr=%" PRId64
-                            " iops_max=%" PRId64
-                            " iops_rd_max=%" PRId64
-                            " iops_wr_max=%" PRId64
-                            " iops_size=%" PRId64 "\n",
-                            info->value->inserted->bps,
-                            info->value->inserted->bps_rd,
-                            info->value->inserted->bps_wr,
-                            info->value->inserted->bps_max,
-                            info->value->inserted->bps_rd_max,
-                            info->value->inserted->bps_wr_max,
-                            info->value->inserted->iops,
-                            info->value->inserted->iops_rd,
-                            info->value->inserted->iops_wr,
-                            info->value->inserted->iops_max,
-                            info->value->inserted->iops_rd_max,
-                            info->value->inserted->iops_wr_max,
-                            info->value->inserted->iops_size);
+        if (info != block_list) {
+            monitor_printf(mon, "\n");
         }
 
-        if (verbose) {
-            monitor_printf(mon, "\nImages:\n");
-            image_info = info->value->inserted->image;
-            while (1) {
-                    bdrv_image_info_dump((fprintf_function)monitor_printf,
-                                         mon, image_info);
-                if (image_info->has_backing_image) {
-                    image_info = image_info->backing_image;
-                } else {
-                    break;
-                }
-            }
-        }
+        print_block_info(mon, info->value, info->value->has_inserted
+                                           ? info->value->inserted : NULL,
+                         verbose);
     }
 
     qapi_free_BlockInfoList(block_list);
-- 
1.8.3.1

[Qemu-devel] [PATCH 5/6] block/hmp: Allow info = NULL in print_block_info()

[Kevin Wolf]

This allows printing infos of BlockDriverStates that aren't at the root
of the graph (and logically implementing a BlockBackend).

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 hmp.c | 34 +++++++++++++++++++++++++---------
 1 file changed, 25 insertions(+), 9 deletions(-)

diff --git a/hmp.c b/hmp.c
index be55b9b..811ceba 100644
--- a/hmp.c
+++ b/hmp.c
@@ -295,7 +295,21 @@ static void print_block_info(Monitor *mon, BlockInfo *info,
 {
     ImageInfo *image_info;
 
-    monitor_printf(mon, "%s", info->device);
+    assert(!info || !info->has_inserted || info->inserted == inserted);
+
+    if (info) {
+        monitor_printf(mon, "%s", info->device);
+        if (inserted && inserted->has_node_name) {
+            monitor_printf(mon, " (%s)", inserted->node_name);
+        }
+    } else {
+        assert(inserted);
+        monitor_printf(mon, "%s",
+                       inserted->has_node_name
+                       ? inserted->node_name
+                       : "<anonymous>");
+    }
+
     if (inserted) {
         monitor_printf(mon, ": %s (%s%s%s)\n",
                        inserted->file,
@@ -306,15 +320,17 @@ static void print_block_info(Monitor *mon, BlockInfo *info,
         monitor_printf(mon, ": [not inserted]\n");
     }
 
-    if (info->has_io_status && info->io_status != BLOCK_DEVICE_IO_STATUS_OK) {
-        monitor_printf(mon, "    I/O status:       %s\n",
-                       BlockDeviceIoStatus_lookup[info->io_status]);
-    }
+    if (info) {
+        if (info->has_io_status && info->io_status != BLOCK_DEVICE_IO_STATUS_OK) {
+            monitor_printf(mon, "    I/O status:       %s\n",
+                           BlockDeviceIoStatus_lookup[info->io_status]);
+        }
 
-    if (info->removable) {
-        monitor_printf(mon, "    Removable device: %slocked, tray %s\n",
-                       info->locked ? "" : "not ",
-                       info->tray_open ? "open" : "closed");
+        if (info->removable) {
+            monitor_printf(mon, "    Removable device: %slocked, tray %s\n",
+                           info->locked ? "" : "not ",
+                           info->tray_open ? "open" : "closed");
+        }
     }
 
 
-- 
1.8.3.1

[Qemu-devel] [PATCH 6/6] block: Allow node-name in HMP 'info block'

[Kevin Wolf]

The optional parameter specifying a block device allows now to use a
node-name instead of a drive name (and therefore to inspect any node in
the graph).

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 hmp.c | 28 ++++++++++++++++++----------
 1 file changed, 18 insertions(+), 10 deletions(-)

diff --git a/hmp.c b/hmp.c
index 811ceba..945e5fa 100644
--- a/hmp.c
+++ b/hmp.c
@@ -403,26 +403,34 @@ static void print_block_info(Monitor *mon, BlockInfo *info,
 void hmp_info_block(Monitor *mon, const QDict *qdict)
 {
     BlockInfoList *block_list, *info;
+    BlockDeviceInfoList *blockdev_list, *blockdev;
     const char *device = qdict_get_try_str(qdict, "device");
     bool verbose = qdict_get_try_bool(qdict, "verbose", 0);
 
-    block_list = qmp_query_block(false, NULL, NULL);
+    block_list = qmp_query_block(!!device, device, NULL);
+    if (block_list) {
+        for (info = block_list; info; info = info->next) {
+            if (info != block_list) {
+                monitor_printf(mon, "\n");
+            }
 
-    for (info = block_list; info; info = info->next) {
-        if (device && strcmp(device, info->value->device)) {
-            continue;
+            print_block_info(mon, info->value, info->value->has_inserted
+                                               ? info->value->inserted : NULL,
+                             verbose);
         }
+        qapi_free_BlockInfoList(block_list);
+        return;
+    }
 
-        if (info != block_list) {
+    blockdev_list = qmp_query_block_node(!!device, device, NULL);
+    for (blockdev = blockdev_list; blockdev; blockdev = blockdev->next) {
+        if (blockdev != blockdev_list) {
             monitor_printf(mon, "\n");
         }
 
-        print_block_info(mon, info->value, info->value->has_inserted
-                                           ? info->value->inserted : NULL,
-                         verbose);
+        print_block_info(mon, NULL, blockdev->value, verbose);
     }
-
-    qapi_free_BlockInfoList(block_list);
+    qapi_free_BlockDeviceInfoList(blockdev_list);
 }
 
 void hmp_info_blockstats(Monitor *mon, const QDict *qdict)
-- 
1.8.3.1