diff mbox

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

Message ID baf3987c4d0e0611d8f545c45f9e132b4ddf15d5.1403041699.git.jcody@redhat.com
State New
Headers show

Commit Message

Jeff Cody June 17, 2014, 9:53 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             | 16 ++++++++++++++--
 qapi/block-core.json   |  7 ++++---
 qmp-commands.hx        |  5 +++--
 tests/qemu-iotests/040 | 28 ++++++++++++++++++----------
 4 files changed, 39 insertions(+), 17 deletions(-)

Comments

Eric Blake June 17, 2014, 10:25 p.m. UTC | #1
On 06/17/2014 03:53 PM, 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>
> ---

As mentioned elsewhere, this is the patch that changed from v5.  But so
there's no confusion, I've gone back over this one, and my R-b still stands.

> +    /* Important Note:
> +     *  libvirt relies on the DeviceNotFound error class in order to probe for
> +     *  live commit feature versions; for this to work, we must make sure to
> +     *  perform the device lookup before any generic errors that may occur in a
> +     *  scenario in which all optional arguments are omitted. */
>      bs = bdrv_find(device);
>      if (!bs) {
>          error_set(errp, QERR_DEVICE_NOT_FOUND, device);

Thanks for adding the comment :)
Stefan Hajnoczi June 19, 2014, 6:40 a.m. UTC | #2
On Tue, Jun 17, 2014 at 05:53:52PM -0400, 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             | 16 ++++++++++++++--
>  qapi/block-core.json   |  7 ++++---
>  qmp-commands.hx        |  5 +++--
>  tests/qemu-iotests/040 | 28 ++++++++++++++++++----------
>  4 files changed, 39 insertions(+), 17 deletions(-)

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
Eric Blake June 19, 2014, 4:56 p.m. UTC | #3
On 06/17/2014 04:25 PM, Eric Blake wrote:
> On 06/17/2014 03:53 PM, 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>
>> ---
> 
> As mentioned elsewhere, this is the patch that changed from v5.  But so
> there's no confusion, I've gone back over this one, and my R-b still stands.
> 
>> +    /* Important Note:
>> +     *  libvirt relies on the DeviceNotFound error class in order to probe for
>> +     *  live commit feature versions; for this to work, we must make sure to
>> +     *  perform the device lookup before any generic errors that may occur in a
>> +     *  scenario in which all optional arguments are omitted. */
>>      bs = bdrv_find(device);
>>      if (!bs) {
>>          error_set(errp, QERR_DEVICE_NOT_FOUND, device);
> 
> Thanks for adding the comment :)
> 

Should we split this into a different series, since it is
non-controversial (unrelated to the rest of the series' impact on
child-node op-blockers), and useful for libvirt to turn on active
commit?  The rest of the series lets libvirt turn on relative backing
chain work, but we might as well get the easy part into the tree now.
diff mbox

Patch

diff --git a/blockdev.c b/blockdev.c
index 9b0f8ac..a9a3b2f 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -1910,7 +1910,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)
 {
@@ -1929,6 +1930,11 @@  void qmp_block_commit(const char *device,
     /* drain all i/o before commits */
     bdrv_drain_all();
 
+    /* Important Note:
+     *  libvirt relies on the DeviceNotFound error class in order to probe for
+     *  live commit feature versions; for this to work, we must make sure to
+     *  perform the device lookup before any generic errors that may occur in a
+     *  scenario in which all optional arguments are omitted. */
     bs = bdrv_find(device);
     if (!bs) {
         error_set(errp, QERR_DEVICE_NOT_FOUND, device);
@@ -1942,7 +1948,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);
         }
@@ -1964,6 +1970,12 @@  void qmp_block_commit(const char *device,
         return;
     }
 
+    /* Do not allow attempts to commit an image into itself */
+    if (top_bs == base_bs) {
+        error_setg(errp, "cannot commit an image into itself");
+        return;
+    }
+
     if (top_bs == bs) {
         commit_active_start(bs, base_bs, speed, on_error, block_job_cb,
                             bs, &local_err);
diff --git a/qapi/block-core.json b/qapi/block-core.json
index 7215e48..6ddce4f 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -690,8 +690,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,
@@ -719,7 +720,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 d6bb0f4..4a8e71a 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