diff mbox

[v2,06/11] block: make 'top' argument to block-commit optional

Message ID d108a0d64f35f0b0f1197871632d01d9ee2c32db.1401200582.git.jcody@redhat.com
State New
Headers show

Commit Message

Jeff Cody May 27, 2014, 2:28 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             |  3 ++-
 qapi-schema.json       |  7 ++++---
 qmp-commands.hx        |  5 +++--
 tests/qemu-iotests/040 | 28 ++++++++++++++++++----------
 4 files changed, 27 insertions(+), 16 deletions(-)

Comments

Eric Blake May 27, 2014, 5:20 p.m. UTC | #1 
On 05/27/2014 08:28 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>
> ---
>  blockdev.c             |  3 ++-
>  qapi-schema.json       |  7 ++++---
>  qmp-commands.hx        |  5 +++--
>  tests/qemu-iotests/040 | 28 ++++++++++++++++++----------
>  4 files changed, 27 insertions(+), 16 deletions(-)
> 
> diff --git a/blockdev.c b/blockdev.c
> index 9a9bdec..b37ace7 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -1912,7 +1912,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)
>  {

Hmm.  Later in this function we have:

    if (top) {
        if (strcmp(bs->filename, top) != 0) {
            top_bs = bdrv_find_backing_image(bs, top);
        }
    }

which is not ideal; it means we are DEPENDING on qapi to NULL-initialize
a pointer when has_top is false.

Although we have (finally!) made that guarantee (see commit fc13d937), I
worry that backporting this patch but not that one may cause a use of
undefined memory, unless you add an explicit:

if (!has_top) {
    top = NULL;
}

In other words, I'm a bit reluctant to use default initialization values
without documenting and testing that we can rely on them.

On the other hand, prior to this commit, the 'if (top)' condition was
dead code (since QMP doesn't allow "arguments":{"top":null}, there was
previously no way for a caller to pass a NULL 'top' parameter - so the
fact that the code was already checking for a NULL top implies that we
had planned ages ago to make top a conditional parameter).  So on that
grounds, we are merely doing what we'd been planning all along.  I can
live with keeping my R-b without a respin, even though it feels a bit
dirty to not be checking has_top.
Eric Blake May 28, 2014, 12:35 p.m. UTC | #2 
On 05/27/2014 08:28 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>
> ---
>  blockdev.c             |  3 ++-
>  qapi-schema.json       |  7 ++++---
>  qmp-commands.hx        |  5 +++--
>  tests/qemu-iotests/040 | 28 ++++++++++++++++++----------
>  4 files changed, 27 insertions(+), 16 deletions(-)

Hmm, the HMP counterpart of 'commit' is not very full-featured compared
to QMP. I think that's a stronger argument for rebasing this series to
not even bother with HMP.  Just focus on the QMP interface, and save HMP
changes for a future day (if ever) when someone needs it.
diff mbox

Patch

diff --git a/blockdev.c b/blockdev.c
index 9a9bdec..b37ace7 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -1912,7 +1912,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)
 {
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