[Qemu-devel] [PATCH v2 for-2.9] block: Declare blockdev-add and blockdev-del supported

Markus Armbruster posted 1 patch 7 years ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/qemu tags/patchew/1490199642-5521-1-git-send-email-armbru@redhat.com
Test checkpatch passed
Test docker passed
Test s390x passed
blockdev.c             |  4 +--
qapi/block-core.json   | 69 ++++++++++++++++++--------------------------------
tests/qemu-iotests/139 |  8 +++---
tests/qemu-iotests/141 |  4 +--
tests/qemu-iotests/147 |  2 +-
5 files changed, 34 insertions(+), 53 deletions(-)
[Qemu-devel] [PATCH v2 for-2.9] block: Declare blockdev-add and blockdev-del supported
Posted by Markus Armbruster 7 years ago
It's been a long journey, but here we are.

The supported blockdev-add is not compatible to its experimental
predecessors; bump all Since: tags to 2.9.

x-blockdev-remove-medium, x-blockdev-insert-medium and
x-blockdev-change need a bit more work, so leave them alone for now.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
v2:
* Another "command is considered experimental" comment dropped [Max]
* More Since: tags updated to 2.9, commit message improved, R-bys dropped

 blockdev.c             |  4 +--
 qapi/block-core.json   | 69 ++++++++++++++++++--------------------------------
 tests/qemu-iotests/139 |  8 +++---
 tests/qemu-iotests/141 |  4 +--
 tests/qemu-iotests/147 |  2 +-
 5 files changed, 34 insertions(+), 53 deletions(-)

diff --git a/blockdev.c b/blockdev.c
index c5b2c2c..040c152 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -2835,7 +2835,7 @@ void hmp_drive_del(Monitor *mon, const QDict *qdict)
 
     bs = bdrv_find_node(id);
     if (bs) {
-        qmp_x_blockdev_del(id, &local_err);
+        qmp_blockdev_del(id, &local_err);
         if (local_err) {
             error_report_err(local_err);
         }
@@ -3900,7 +3900,7 @@ fail:
     visit_free(v);
 }
 
-void qmp_x_blockdev_del(const char *node_name, Error **errp)
+void qmp_blockdev_del(const char *node_name, Error **errp)
 {
     AioContext *aio_context;
     BlockDriverState *bs;
diff --git a/qapi/block-core.json b/qapi/block-core.json
index 0f132fc..bd39b3f 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -2108,18 +2108,7 @@
 #
 # Drivers that are supported in block device operations.
 #
-# @host_device: Since 2.1
-# @host_cdrom: Since 2.1
-# @gluster: Since 2.7
-# @nbd: Since 2.8
-# @nfs: Since 2.8
-# @replication: Since 2.8
-# @ssh: Since 2.8
-# @iscsi: Since 2.9
-# @rbd: Since 2.9
-# @sheepdog: Since 2.9
-#
-# Since: 2.0
+# Since: 2.9
 ##
 { 'enum': 'BlockdevDriver',
   'data': [ 'blkdebug', 'blkverify', 'bochs', 'cloop',
@@ -2137,7 +2126,7 @@
 # @filename:    path to the image file
 # @aio:         AIO backend (default: threads) (since: 2.8)
 #
-# Since: 1.7
+# Since: 2.9
 ##
 { 'struct': 'BlockdevOptionsFile',
   'data': { 'filename': 'str',
@@ -2153,7 +2142,7 @@
 #              requests. Default to zero which completes requests immediately.
 #              (Since 2.4)
 #
-# Since: 2.2
+# Since: 2.9
 ##
 { 'struct': 'BlockdevOptionsNull',
   'data': { '*size': 'int', '*latency-ns': 'uint64' } }
@@ -2173,7 +2162,7 @@
 #               (since 2.4)
 # @rw:          whether to allow write operations (default: false)
 #
-# Since: 1.7
+# Since: 2.9
 ##
 { 'struct': 'BlockdevOptionsVVFAT',
   'data': { 'dir': 'str', '*fat-type': 'int', '*floppy': 'bool',
@@ -2187,7 +2176,7 @@
 #
 # @file:        reference to or definition of the data source block device
 #
-# Since: 1.7
+# Since: 2.9
 ##
 { 'struct': 'BlockdevOptionsGenericFormat',
   'data': { 'file': 'BlockdevRef' } }
@@ -2201,7 +2190,7 @@
 #              the decryption key (since 2.6). Mandatory except when
 #              doing a metadata-only probe of the image.
 #
-# Since: 2.6
+# Since: 2.9
 ##
 { 'struct': 'BlockdevOptionsLUKS',
   'base': 'BlockdevOptionsGenericFormat',
@@ -2219,7 +2208,7 @@
 #               allowed to pass an empty string here in order to disable the
 #               default backing file.
 #
-# Since: 1.7
+# Since: 2.9
 ##
 { 'struct': 'BlockdevOptionsGenericCOWFormat',
   'base': 'BlockdevOptionsGenericFormat',
@@ -2320,7 +2309,7 @@
 #                         caches. The interval is in seconds. The default value
 #                         is 0 and it disables this feature (since 2.5)
 #
-# Since: 1.7
+# Since: 2.9
 ##
 { 'struct': 'BlockdevOptionsQcow2',
   'base': 'BlockdevOptionsGenericCOWFormat',
@@ -2347,7 +2336,7 @@
 #
 # TODO: Expose the host_key_check option in QMP
 #
-# Since: 2.8
+# Since: 2.9
 ##
 { 'struct': 'BlockdevOptionsSsh',
   'data': { 'server': 'InetSocketAddress',
@@ -2446,7 +2435,7 @@
 #
 # @set-state:       array of state-change descriptions
 #
-# Since: 2.0
+# Since: 2.9
 ##
 { 'struct': 'BlockdevOptionsBlkdebug',
   'data': { 'image': 'BlockdevRef',
@@ -2464,7 +2453,7 @@
 #
 # @raw:     raw image used for verification
 #
-# Since: 2.0
+# Since: 2.9
 ##
 { 'struct': 'BlockdevOptionsBlkverify',
   'data': { 'test': 'BlockdevRef',
@@ -2501,7 +2490,7 @@
 # @read-pattern: choose read pattern and set to quorum by default
 #                (Since 2.2)
 #
-# Since: 2.0
+# Since: 2.9
 ##
 { 'struct': 'BlockdevOptionsQuorum',
   'data': { '*blkverify': 'bool',
@@ -2526,7 +2515,7 @@
 #
 # @logfile:     libgfapi log file (default /dev/stderr) (Since 2.8)
 #
-# Since: 2.7
+# Since: 2.9
 ##
 { 'struct': 'BlockdevOptionsGluster',
   'data': { 'volume': 'str',
@@ -2700,7 +2689,7 @@
 #          node who owns the replication node chain. Must not be given in
 #          primary mode.
 #
-# Since: 2.8
+# Since: 2.9
 ##
 { 'struct': 'BlockdevOptionsReplication',
   'base': 'BlockdevOptionsGenericFormat',
@@ -2763,7 +2752,7 @@
 # @debug:                   set the NFS debug level (max 2) (defaults
 #                           to libnfs default)
 #
-# Since: 2.8
+# Since: 2.9
 ##
 { 'struct': 'BlockdevOptionsNfs',
   'data': { 'server': 'NFSServer',
@@ -2782,7 +2771,7 @@
 #
 # @filename:    path to the image file
 #
-# Since: 1.7
+# Since: 2.9
 ##
 { 'struct': 'BlockdevOptionsCurl',
   'data': { 'filename': 'str' } }
@@ -2798,7 +2787,7 @@
 #
 # @tls-creds:   TLS credentials ID
 #
-# Since: 2.8
+# Since: 2.9
 ##
 { 'struct': 'BlockdevOptionsNbd',
   'data': { 'server': 'SocketAddress',
@@ -2813,7 +2802,7 @@
 # @offset:      position where the block device starts
 # @size:        the assumed size of the device
 #
-# Since: 2.8
+# Since: 2.9
 ##
 { 'struct': 'BlockdevOptionsRaw',
   'base': 'BlockdevOptionsGenericFormat',
@@ -2837,7 +2826,7 @@
 #
 # Remaining options are determined by the block driver.
 #
-# Since: 1.7
+# Since: 2.9
 ##
 { 'union': 'BlockdevOptions',
   'base': { 'driver': 'BlockdevDriver',
@@ -2894,7 +2883,7 @@
 #                   empty string means that no block device should be
 #                   referenced.
 #
-# Since: 1.7
+# Since: 2.9
 ##
 { 'alternate': 'BlockdevRef',
   'data': { 'definition': 'BlockdevOptions',
@@ -2907,11 +2896,7 @@
 # BlockBackend will be created; otherwise, @node-name is mandatory at the top
 # level and no BlockBackend will be created.
 #
-# Note: This command is still a work in progress.  It doesn't support all
-# block drivers among other things.  Stay away from it unless you want
-# to help with its development.
-#
-# Since: 1.7
+# Since: 2.9
 #
 # Example:
 #
@@ -2957,7 +2942,7 @@
 { 'command': 'blockdev-add', 'data': 'BlockdevOptions', 'boxed': true }
 
 ##
-# @x-blockdev-del:
+# @blockdev-del:
 #
 # Deletes a block device that has been added using blockdev-add.
 # The command will fail if the node is attached to a device or is
@@ -2965,11 +2950,7 @@
 #
 # @node-name: Name of the graph node to delete.
 #
-# Note: This command is still a work in progress and is considered
-# experimental. Stay away from it unless you want to help with its
-# development.
-#
-# Since: 2.5
+# Since: 2.9
 #
 # Example:
 #
@@ -2985,13 +2966,13 @@
 #    }
 # <- { "return": {} }
 #
-# -> { "execute": "x-blockdev-del",
+# -> { "execute": "blockdev-del",
 #      "arguments": { "node-name": "node0" }
 #    }
 # <- { "return": {} }
 #
 ##
-{ 'command': 'x-blockdev-del', 'data': { 'node-name': 'str' } }
+{ 'command': 'blockdev-del', 'data': { 'node-name': 'str' } }
 
 ##
 # @blockdev-open-tray:
diff --git a/tests/qemu-iotests/139 b/tests/qemu-iotests/139
index 6d98e4f..175d8f0 100644
--- a/tests/qemu-iotests/139
+++ b/tests/qemu-iotests/139
@@ -1,6 +1,6 @@
 #!/usr/bin/env python
 #
-# Test cases for the QMP 'x-blockdev-del' command
+# Test cases for the QMP 'blockdev-del' command
 #
 # Copyright (C) 2015 Igalia, S.L.
 # Author: Alberto Garcia <berto@igalia.com>
@@ -79,7 +79,7 @@ class TestBlockdevDel(iotests.QMPTestCase):
     # Delete a BlockDriverState
     def delBlockDriverState(self, node, expect_error = False):
         self.checkBlockDriverState(node)
-        result = self.vm.qmp('x-blockdev-del', node_name = node)
+        result = self.vm.qmp('blockdev-del', node_name = node)
         if expect_error:
             self.assert_qmp(result, 'error/class', 'GenericError')
         else:
@@ -173,7 +173,7 @@ class TestBlockdevDel(iotests.QMPTestCase):
         self.wait_until_completed(id)
 
     # Add a BlkDebug node
-    # Note that the purpose of this is to test the x-blockdev-del
+    # Note that the purpose of this is to test the blockdev-del
     # sanity checks, not to create a usable blkdebug drive
     def addBlkDebug(self, debug, node):
         self.checkBlockDriverState(node, False)
@@ -191,7 +191,7 @@ class TestBlockdevDel(iotests.QMPTestCase):
         self.checkBlockDriverState(debug)
 
     # Add a BlkVerify node
-    # Note that the purpose of this is to test the x-blockdev-del
+    # Note that the purpose of this is to test the blockdev-del
     # sanity checks, not to create a usable blkverify drive
     def addBlkVerify(self, blkverify, test, raw):
         self.checkBlockDriverState(test, False)
diff --git a/tests/qemu-iotests/141 b/tests/qemu-iotests/141
index 6d8f0a1..27fb1cc 100755
--- a/tests/qemu-iotests/141
+++ b/tests/qemu-iotests/141
@@ -65,7 +65,7 @@ test_blockjob()
 
     # We want this to return an error because the block job is still running
     _send_qemu_cmd $QEMU_HANDLE \
-        "{'execute': 'x-blockdev-del',
+        "{'execute': 'blockdev-del',
           'arguments': {'node-name': 'drv0'}}" \
         'error' | _filter_generated_node_ids
 
@@ -75,7 +75,7 @@ test_blockjob()
         "$3"
 
     _send_qemu_cmd $QEMU_HANDLE \
-        "{'execute': 'x-blockdev-del',
+        "{'execute': 'blockdev-del',
           'arguments': {'node-name': 'drv0'}}" \
         'return'
 }
diff --git a/tests/qemu-iotests/147 b/tests/qemu-iotests/147
index 45469c9..cca75c5 100755
--- a/tests/qemu-iotests/147
+++ b/tests/qemu-iotests/147
@@ -57,7 +57,7 @@ class NBDBlockdevAddBase(iotests.QMPTestCase):
                                                     filename)
                 break
 
-        result = self.vm.qmp('x-blockdev-del', node_name='nbd-blockdev')
+        result = self.vm.qmp('blockdev-del', node_name='nbd-blockdev')
         self.assert_qmp(result, 'return', {})
 
 
-- 
2.7.4


Re: [Qemu-devel] [PATCH v2 for-2.9] block: Declare blockdev-add and blockdev-del supported
Posted by Max Reitz 7 years ago
On 22.03.2017 17:20, Markus Armbruster wrote:
> It's been a long journey, but here we are.
> 
> The supported blockdev-add is not compatible to its experimental
> predecessors; bump all Since: tags to 2.9.
> 
> x-blockdev-remove-medium, x-blockdev-insert-medium and
> x-blockdev-change need a bit more work, so leave them alone for now.
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
> v2:
> * Another "command is considered experimental" comment dropped [Max]
> * More Since: tags updated to 2.9, commit message improved, R-bys dropped

Do you want to do this recursively? There are things such as
BlkdebugSetStateOptions or ReplicationMode which are only used by the
corresponding BlockdevOptions*, so they too are basically only supported
as of 2.9.

I don't have a strong opinion, but I think I personally wouldn't bother
with any of these structures and just update the tag for blockdev-add
itself...

You being fine with me would be enough to convince me, so:

Reviewed-by: Max Reitz <mreitz@redhat.com>

> 
>  blockdev.c             |  4 +--
>  qapi/block-core.json   | 69 ++++++++++++++++++--------------------------------
>  tests/qemu-iotests/139 |  8 +++---
>  tests/qemu-iotests/141 |  4 +--
>  tests/qemu-iotests/147 |  2 +-
>  5 files changed, 34 insertions(+), 53 deletions(-)

Re: [Qemu-devel] [PATCH v2 for-2.9] block: Declare blockdev-add and blockdev-del supported
Posted by Eric Blake 7 years ago
On 03/22/2017 12:13 PM, Max Reitz wrote:
> On 22.03.2017 17:20, Markus Armbruster wrote:
>> It's been a long journey, but here we are.
>>
>> The supported blockdev-add is not compatible to its experimental
>> predecessors; bump all Since: tags to 2.9.
>>
>> x-blockdev-remove-medium, x-blockdev-insert-medium and
>> x-blockdev-change need a bit more work, so leave them alone for now.
>>
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> ---
>> v2:
>> * Another "command is considered experimental" comment dropped [Max]
>> * More Since: tags updated to 2.9, commit message improved, R-bys dropped
> 
> Do you want to do this recursively? There are things such as
> BlkdebugSetStateOptions or ReplicationMode which are only used by the
> corresponding BlockdevOptions*, so they too are basically only supported
> as of 2.9.
> 
> I don't have a strong opinion, but I think I personally wouldn't bother
> with any of these structures and just update the tag for blockdev-add
> itself...

I also don't have a strong opinion, and think it may actually be a bit
more time consuming to audit which types are recursively used only by
blockdev-add (and no where else) than to just leave them be.  But I'm
also okay if you like bumping the numbers to the point where the struct
was marked stable, and where we have to start thinking about back-compat
ramifications when modifying the struct.

> 
> You being fine with me would be enough to convince me, so:
> 
> Reviewed-by: Max Reitz <mreitz@redhat.com>

Likewise.

Reviewed-by: Eric Blake <eblake@redhat.com>

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