diff mbox

[v4,04/10] block: make 'top' argument to block-commit optional

Message ID fe437da04cdcbbdde4e940000129bb07d8bf9393.1401886089.git.jcody@redhat.com
State New
Headers show

Commit Message

Jeff Cody June 4, 2014, 1:51 p.m. UTC
Now that active layer block-commit is supported, the 'top' argument
no longer needs to be mandatory.

Change it to optional, with the default being the active layer in the
device chain.

Reviewed-by: Eric Blake <eblake@redhat.com>
Reviewed-by: Benoit Canet <benoit@irqsave.net>
Signed-off-by: Jeff Cody <jcody@redhat.com>
---
 blockdev.c             |  5 +++--
 qapi-schema.json       |  7 ++++---
 qmp-commands.hx        |  5 +++--
 tests/qemu-iotests/040 | 28 ++++++++++++++++++----------
 4 files changed, 28 insertions(+), 17 deletions(-)

Comments

Eric Blake June 4, 2014, 8:59 p.m. UTC | #1
On 06/04/2014 07:51 AM, Jeff Cody wrote:
> Now that active layer block-commit is supported, the 'top' argument
> no longer needs to be mandatory.
> 
> Change it to optional, with the default being the active layer in the
> device chain.
> 
> Reviewed-by: Eric Blake <eblake@redhat.com>
> Reviewed-by: Benoit Canet <benoit@irqsave.net>
> Signed-off-by: Jeff Cody <jcody@redhat.com>
> ---

Unrelated to my review, but I wish we had done a better job at making
the qemu 2.0 addition of active commit introspectible.  Had we made
_this_ patch at the same time, introspection would be possible by
creating a dummy blockdev, then attempting a block-commit that omits the
'top' argument (since the error message for a missing required argument
[old qemu] is different than the message for a blockdev that can't be
committed [new qemu]).  But since this change is in a different release
than where we supported active commit, I'm stuck coming up with some
reliable way to do a probe of whether active commit is supported so that
libvirt knows whether to expose active commit on to the end user.
Jeff Cody June 5, 2014, 12:26 a.m. UTC | #2
On Wed, Jun 04, 2014 at 02:59:44PM -0600, Eric Blake wrote:
> On 06/04/2014 07:51 AM, Jeff Cody wrote:
> > Now that active layer block-commit is supported, the 'top' argument
> > no longer needs to be mandatory.
> > 
> > Change it to optional, with the default being the active layer in the
> > device chain.
> > 
> > Reviewed-by: Eric Blake <eblake@redhat.com>
> > Reviewed-by: Benoit Canet <benoit@irqsave.net>
> > Signed-off-by: Jeff Cody <jcody@redhat.com>
> > ---
> 
> Unrelated to my review, but I wish we had done a better job at making
> the qemu 2.0 addition of active commit introspectible.  Had we made
> _this_ patch at the same time, introspection would be possible by
> creating a dummy blockdev, then attempting a block-commit that omits the
> 'top' argument (since the error message for a missing required argument
> [old qemu] is different than the message for a blockdev that can't be
> committed [new qemu]).  But since this change is in a different release
> than where we supported active commit, I'm stuck coming up with some
> reliable way to do a probe of whether active commit is supported so that
> libvirt knows whether to expose active commit on to the end user.
>

Well, I think what we _really_ need is true QAPI introspection.

Unfortunately, libvirt can't rely on the QEMU version to determine API
levels. This leaves it needing to do various QAPI contortions to
figure out what is supported and what is not supported, which seems
clunky and probably not sustainable long-term.

But I agree with the gist of what you saying, I think: libvirt and
QEMU should make sure to stay in the loop with each other on
command discoverability.

With that said, 2.1 soft freeze is not that far off; what other QAPI
commands might we be saying this about once 2.1 is out?


-Jeff
diff mbox

Patch

diff --git a/blockdev.c b/blockdev.c
index 9b5261b..58b526a 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -1914,7 +1914,8 @@  void qmp_block_stream(const char *device, bool has_base,
 }
 
 void qmp_block_commit(const char *device,
-                      bool has_base, const char *base, const char *top,
+                      bool has_base, const char *base,
+                      bool has_top, const char *top,
                       bool has_speed, int64_t speed,
                       Error **errp)
 {
@@ -1946,7 +1947,7 @@  void qmp_block_commit(const char *device,
     /* default top_bs is the active layer */
     top_bs = bs;
 
-    if (top) {
+    if (has_top && top) {
         if (strcmp(bs->filename, top) != 0) {
             top_bs = bdrv_find_backing_image(bs, top);
         }
diff --git a/qapi-schema.json b/qapi-schema.json
index 7bc33ea..97cf053 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -2102,8 +2102,9 @@ 
 # @base:   #optional The file name of the backing image to write data into.
 #                    If not specified, this is the deepest backing image
 #
-# @top:              The file name of the backing image within the image chain,
-#                    which contains the topmost data to be committed down.
+# @top:    #optional The file name of the backing image within the image chain,
+#                    which contains the topmost data to be committed down. If
+#                    not specified, this is the active layer.
 #
 #                    If top == base, that is an error.
 #                    If top == active, the job will not be completed by itself,
@@ -2131,7 +2132,7 @@ 
 #
 ##
 { 'command': 'block-commit',
-  'data': { 'device': 'str', '*base': 'str', 'top': 'str',
+  'data': { 'device': 'str', '*base': 'str', '*top': 'str',
             '*speed': 'int' } }
 
 ##
diff --git a/qmp-commands.hx b/qmp-commands.hx
index d8aa4ed..6b67d2f 100644
--- a/qmp-commands.hx
+++ b/qmp-commands.hx
@@ -985,7 +985,7 @@  EQMP
 
     {
         .name       = "block-commit",
-        .args_type  = "device:B,base:s?,top:s,speed:o?",
+        .args_type  = "device:B,base:s?,top:s?,speed:o?",
         .mhandler.cmd_new = qmp_marshal_input_block_commit,
     },
 
@@ -1003,7 +1003,8 @@  Arguments:
           If not specified, this is the deepest backing image
           (json-string, optional)
 - "top":  The file name of the backing image within the image chain,
-          which contains the topmost data to be committed down.
+          which contains the topmost data to be committed down. If
+          not specified, this is the active layer. (json-string, optional)
 
           If top == base, that is an error.
           If top == active, the job will not be completed by itself,
diff --git a/tests/qemu-iotests/040 b/tests/qemu-iotests/040
index 734b6a6..803b0c7 100755
--- a/tests/qemu-iotests/040
+++ b/tests/qemu-iotests/040
@@ -35,11 +35,7 @@  test_img = os.path.join(iotests.test_dir, 'test.img')
 class ImageCommitTestCase(iotests.QMPTestCase):
     '''Abstract base class for image commit test cases'''
 
-    def run_commit_test(self, top, base):
-        self.assert_no_active_block_jobs()
-        result = self.vm.qmp('block-commit', device='drive0', top=top, base=base)
-        self.assert_qmp(result, 'return', {})
-
+    def wait_for_complete(self):
         completed = False
         while not completed:
             for event in self.vm.get_qmp_events(wait=True):
@@ -58,6 +54,18 @@  class ImageCommitTestCase(iotests.QMPTestCase):
         self.assert_no_active_block_jobs()
         self.vm.shutdown()
 
+    def run_commit_test(self, top, base):
+        self.assert_no_active_block_jobs()
+        result = self.vm.qmp('block-commit', device='drive0', top=top, base=base)
+        self.assert_qmp(result, 'return', {})
+        self.wait_for_complete()
+
+    def run_default_commit_test(self):
+        self.assert_no_active_block_jobs()
+        result = self.vm.qmp('block-commit', device='drive0')
+        self.assert_qmp(result, 'return', {})
+        self.wait_for_complete()
+
 class TestSingleDrive(ImageCommitTestCase):
     image_len = 1 * 1024 * 1024
     test_len = 1 * 1024 * 256
@@ -109,17 +117,17 @@  class TestSingleDrive(ImageCommitTestCase):
         self.assertEqual(-1, qemu_io('-c', 'read -P 0xab 0 524288', backing_img).find("verification failed"))
         self.assertEqual(-1, qemu_io('-c', 'read -P 0xef 524288 524288', backing_img).find("verification failed"))
 
+    def test_top_is_default_active(self):
+        self.run_default_commit_test()
+        self.assertEqual(-1, qemu_io('-c', 'read -P 0xab 0 524288', backing_img).find("verification failed"))
+        self.assertEqual(-1, qemu_io('-c', 'read -P 0xef 524288 524288', backing_img).find("verification failed"))
+
     def test_top_and_base_reversed(self):
         self.assert_no_active_block_jobs()
         result = self.vm.qmp('block-commit', device='drive0', top='%s' % backing_img, base='%s' % mid_img)
         self.assert_qmp(result, 'error/class', 'GenericError')
         self.assert_qmp(result, 'error/desc', 'Base \'%s\' not found' % mid_img)
 
-    def test_top_omitted(self):
-        self.assert_no_active_block_jobs()
-        result = self.vm.qmp('block-commit', device='drive0')
-        self.assert_qmp(result, 'error/class', 'GenericError')
-        self.assert_qmp(result, 'error/desc', "Parameter 'top' is missing")
 
 class TestRelativePaths(ImageCommitTestCase):
     image_len = 1 * 1024 * 1024