Skip to contentSkip to conversation

block-commit fixes

PATCH Superseded Jeff Cody 2 participants 10 messages Oct 16, 2012 Block layer

v2 of 3 Superseded Latest is v3 →

lore
Share & tools
lore

Jeff Cody

author
#0 lore raw

v2 cover block-commit fixes

v1 -> v2 differences:

* Updated changes to be protocol friendly
* Updated block-commit tests to account for different error message wording
* Updated block-commit tests to add relative backing filename tests
* In bdrv_find_backing_image(), use g_malloc() instead of char arrays

Jeff Cody (4):
block: make bdrv_find_backing_image compare canonical filenames
block: in commit, determine base image from the top image
qemu-iotests: qemu-io tests update for block-commit (040)
qemu-iotests: add relative backing file tests for block-commit (040)

 block.c                    |  64 ++++++++++++++++++++++++---
 block/commit.c             |   9 ----
 blockdev.c                 |  21 ++++-----
 tests/qemu-iotests/040     | 106 ++++++++++++++++++++++++++++++++++++++++++++-
 tests/qemu-iotests/040.out |   4 +-
 5 files changed, 175 insertions(+), 29 deletions(-)
Signature · 1 line
1.7.11.4
author
#1 lore raw

PATCH v2 1/4 block: make bdrv_find_backing_image compare canonical filenames

Currently, bdrv_find_backing_image compares bs->backing_file with what is passed in as a backing_file name. Mismatches may occur, however, when bs->backing_file and backing_file are both not absolute or relative.

Use path_combine() to make sure any relative backing filenames are relative to the current image filename being searched, and then use realpath() to make all comparisons based on absolute filenames.

This also changes bdrv_find_backing_image to no longer be recursive, but iterative.

---
 block.c | 64 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++------
 1 file changed, 58 insertions(+), 6 deletions(-)
block.c +58 -6
index e95f613..165905a 100644
@@ -3123,22 +3123,74 @@ int bdrv_snapshot_load_tmp(BlockDriverState *bs,
     return -ENOTSUP;
 }
 
+/* backing_file can either be relative, or absolute, or a protocol.  If it is
+ * relative, it must be relative to the chain.  So, passing in bs->filename
+ * from a BDS as backing_file should not be done, as that may be relative to
+ * the CWD rather than the chain. */
 BlockDriverState *bdrv_find_backing_image(BlockDriverState *bs,
         const char *backing_file)
 {
-    if (!bs->drv) {
+    char *filename_full = NULL;
+    char *backing_file_full = NULL;
+    char *filename_tmp = NULL;
+    int is_protocol = 0;
+    BlockDriverState *curr_bs = NULL;
+    BlockDriverState *retval = NULL;
+
+    if (!bs || !bs->drv || !backing_file) {
         return NULL;
     }
 
-    if (bs->backing_hd) {
-        if (strcmp(bs->backing_file, backing_file) == 0) {
-            return bs->backing_hd;
+    filename_full     = g_malloc(sizeof(char) * PATH_MAX);
+    backing_file_full = g_malloc(sizeof(char) * PATH_MAX);
+    filename_tmp      = g_malloc(sizeof(char) * PATH_MAX);
+    if (!filename_full || !backing_file_full || !filename_tmp) {
+        goto error;
+    }
+
+    is_protocol = path_has_protocol(backing_file);
+
+    for (curr_bs = bs; curr_bs->backing_hd; curr_bs = curr_bs->backing_hd) {
+
+        /* If either of the filename paths is actually a protocol, then
+         * compare unmodified paths; otherwise make paths relative */
+        if (is_protocol || path_has_protocol(curr_bs->backing_file)) {
+            if (strcmp(backing_file, curr_bs->backing_file) == 0) {
+                retval = curr_bs->backing_hd;
+                break;
+            }
         } else {
-            return bdrv_find_backing_image(bs->backing_hd, backing_file);
+            /* If not an absolute filename path, make it relative to the current
+             * image's filename path */
+            path_combine(filename_tmp, PATH_MAX, curr_bs->filename,
+                         backing_file);
+
+            /* We are going to compare absolute pathnames */
+            if (!realpath(filename_tmp, filename_full)) {
+                continue;
+            }
+
+            /* We need to make sure the backing filename we are comparing against
+             * is relative to the current image filename (or absolute) */
+            path_combine(filename_tmp, PATH_MAX, curr_bs->filename,
+                         curr_bs->backing_file);
+
+            if (!realpath(filename_tmp, backing_file_full)) {
+                continue;
+            }
+
+            if (strcmp(backing_file_full, filename_full) == 0) {
+                retval = curr_bs->backing_hd;
+                break;
+            }
         }
     }
 
-    return NULL;
+error:
+    g_free(filename_full);
+    g_free(backing_file_full);
+    g_free(filename_tmp);
+    return retval;
 }
 
 int bdrv_get_backing_file_depth(BlockDriverState *bs)
Signature · 1 line
1.7.11.4
#2 lore raw

Re: [Qemu-devel] [PATCH v2 1/4] block: make bdrv_find_backing_image compare canonical filenames

Quoted 3 lines · Jeff Cody

Currently, bdrv_find_backing_image compares bs->backing_file with what is passed in as a backing_file name. Mismatches may occur, however, when bs->backing_file and backing_file are both not

Reads better as s/both not/not both/.

Quoted 14 lines

absolute or relative.

Use path_combine() to make sure any relative backing filenames are relative to the current image filename being searched, and then use realpath() to make all comparisons based on absolute filenames.

This also changes bdrv_find_backing_image to no longer be recursive, but iterative.

---
 block.c | 64 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++------
 1 file changed, 58 insertions(+), 6 deletions(-)

block.c int bdrv_snapshot_load_tmp(BlockDriverState *bs, · line 3136

Quoted 5 lines
block.c +4 -1
-    if (!bs->drv) {
+    char *filename_full = NULL;
+    char *backing_file_full = NULL;
+    char *filename_tmp = NULL;
+    int is_protocol = 0;

Any reason you didn't use bool here?

block.c int bdrv_snapshot_load_tmp(BlockDriverState *bs, · line 3144

Quoted 11 lines
block.c +5 -3
+    BlockDriverState *curr_bs = NULL;
+    BlockDriverState *retval = NULL;
+
+    if (!bs || !bs->drv || !backing_file) {
         return NULL;
     }
 
-    if (bs->backing_hd) {
-        if (strcmp(bs->backing_file, backing_file) == 0) {
-            return bs->backing_hd;
+    filename_full     = g_malloc(sizeof(char) * PATH_MAX);

sizeof(char) is guaranteed to be 1; this can be simplified to g_malloc(PATH_MAX).

block.c int bdrv_snapshot_load_tmp(BlockDriverState *bs, · line 3149

Quoted 5 lines
block.c +5 -0
+    backing_file_full = g_malloc(sizeof(char) * PATH_MAX);
+    filename_tmp      = g_malloc(sizeof(char) * PATH_MAX);
+    if (!filename_full || !backing_file_full || !filename_tmp) {
+        goto error;
+    }

Dead 'if', since g_malloc() is guaranteed to succeed.

block.c int bdrv_snapshot_load_tmp(BlockDriverState *bs, · line 3158

Quoted 9 lines
block.c +9 -0
+
+    is_protocol = path_has_protocol(backing_file);
+
+    for (curr_bs = bs; curr_bs->backing_hd; curr_bs = curr_bs->backing_hd) {
+
+        /* If either of the filename paths is actually a protocol, then
+         * compare unmodified paths; otherwise make paths relative */
+        if (is_protocol || path_has_protocol(curr_bs->backing_file)) {
+            if (strcmp(backing_file, curr_bs->backing_file) == 0) {

I guess we are guaranteed that if is_protocol and path_has_protocol() give different answers, then the strcmp() will fail?

Signature · 2 lines
Eric Blake   [email protected]    +1-919-301-3266
Libvirt virtualization library http://libvirt.org
author
#3 lore raw

Re: [Qemu-devel] [PATCH v2 1/4] block: make bdrv_find_backing_image compare canonical filenames

Quoted 21 lines · Eric Blake
Quoted 3 lines · Jeff Cody

Currently, bdrv_find_backing_image compares bs->backing_file with what is passed in as a backing_file name. Mismatches may occur, however, when bs->backing_file and backing_file are both not

Reads better as s/both not/not both/.

Quoted 13 lines

absolute or relative.

Use path_combine() to make sure any relative backing filenames are relative to the current image filename being searched, and then use realpath() to make all comparisons based on absolute filenames.

This also changes bdrv_find_backing_image to no longer be recursive, but iterative.

---
 block.c | 64 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++------
 1 file changed, 58 insertions(+), 6 deletions(-)
Quoted 8 lines

block.c int bdrv_snapshot_load_tmp(BlockDriverState *bs, · line 3136

Quoted 5 lines
block.c +4 -1
-    if (!bs->drv) {
+    char *filename_full = NULL;
+    char *backing_file_full = NULL;
+    char *filename_tmp = NULL;
+    int is_protocol = 0;

Any reason you didn't use bool here?

path_has_protocol() returns an int, so I preferred to just match that.

Quoted 15 lines

block.c int bdrv_snapshot_load_tmp(BlockDriverState *bs, · line 3144

Quoted 11 lines
block.c +5 -3
+    BlockDriverState *curr_bs = NULL;
+    BlockDriverState *retval = NULL;
+
+    if (!bs || !bs->drv || !backing_file) {
         return NULL;
     }
 
-    if (bs->backing_hd) {
-        if (strcmp(bs->backing_file, backing_file) == 0) {
-            return bs->backing_hd;
+    filename_full     = g_malloc(sizeof(char) * PATH_MAX);

sizeof(char) is guaranteed to be 1; this can be simplified to g_malloc(PATH_MAX).

Quoted 8 lines

block.c int bdrv_snapshot_load_tmp(BlockDriverState *bs, · line 3149

Quoted 5 lines
block.c +5 -0
+    backing_file_full = g_malloc(sizeof(char) * PATH_MAX);
+    filename_tmp      = g_malloc(sizeof(char) * PATH_MAX);
+    if (!filename_full || !backing_file_full || !filename_tmp) {
+        goto error;
+    }

Dead 'if', since g_malloc() is guaranteed to succeed.

I'll go ahead and simplify both of the above with the next spin.

Quoted 4 lines
Quoted 4 lines
block.c +4 -0
+
+    is_protocol = path_has_protocol(backing_file);
+
+    for (curr_bs = bs; curr_bs->backing_hd; curr_bs = curr_bs->backing_hd) {
Quoted 9 lines

block.c int bdrv_snapshot_load_tmp(BlockDriverState *bs, · line 3158

Quoted 5 lines
block.c +5 -0
+
+        /* If either of the filename paths is actually a protocol, then
+         * compare unmodified paths; otherwise make paths relative */
+        if (is_protocol || path_has_protocol(curr_bs->backing_file)) {
+            if (strcmp(backing_file, curr_bs->backing_file) == 0) {

I guess we are guaranteed that if is_protocol and path_has_protocol() give different answers, then the strcmp() will fail?

Yes, since is_protocol is determined via path_has_protocol() - if they are different, then by definition strcmp() should fail.

Jeff

author
#4 lore raw

PATCH v2 2/4 block: in commit, determine base image from the top image

In reply to Jeff Cody

This simplifies some code and error checking, and also fixes a bug.

bdrv_find_backing_image() should only be passed absolute filenames, or filenames relative to the chain. In the QMP message handler for block commit, when looking up the base do so from the determined top image, so we know it is reachable from top.

---
 block/commit.c |  9 ---------
 blockdev.c     | 21 +++++++++++----------
 2 files changed, 11 insertions(+), 19 deletions(-)
block/commit.c +0 -9
index 733c914..13d9e82 100644
@@ -211,15 +211,6 @@ void commit_start(BlockDriverState *bs, BlockDriverState *base,
         return;
     }
 
-    /* top and base may be valid, but let's make sure that base is reachable
-     * from top */
-    if (bdrv_find_backing_image(top, base->filename) != base) {
-        error_setg(errp,
-                   "Base (%s) is not reachable from top (%s)",
-                   base->filename, top->filename);
-        return;
-    }
-
     overlay_bs = bdrv_find_overlay(bs, top);
 
     if (overlay_bs == NULL) {
blockdev.c +11 -10
index 99828ad..7052287 100644
@@ -1157,16 +1157,6 @@ void qmp_block_commit(const char *device,
         error_set(errp, QERR_DEVICE_NOT_FOUND, device);
         return;
     }
-    if (base && has_base) {
-        base_bs = bdrv_find_backing_image(bs, base);
-    } else {
-        base_bs = bdrv_find_base(bs);
-    }
-
-    if (base_bs == NULL) {
-        error_set(errp, QERR_BASE_NOT_FOUND, base ? base : "NULL");
-        return;
-    }
 
     /* default top_bs is the active layer */
     top_bs = bs;
@@ -1182,6 +1172,17 @@ void qmp_block_commit(const char *device,
         return;
     }
 
+    if (base && has_base) {
+        base_bs = bdrv_find_backing_image(top_bs, base);
+    } else {
+        base_bs = bdrv_find_base(top_bs);
+    }
+
+    if (base_bs == NULL) {
+        error_set(errp, QERR_BASE_NOT_FOUND, base ? base : "NULL");
+        return;
+    }
+
     commit_start(bs, base_bs, top_bs, speed, on_error, block_job_cb, bs,
                 &local_err);
     if (local_err != NULL) {
Signature · 1 line
1.7.11.4
#5 lore raw

Re: [Qemu-devel] [PATCH v2 2/4] block: in commit, determine base image from the top image

Quoted 7 lines · Jeff Cody

This simplifies some code and error checking, and also fixes a bug.

bdrv_find_backing_image() should only be passed absolute filenames, or filenames relative to the chain. In the QMP message handler for block commit, when looking up the base do so from the determined top image, so we know it is reachable from top.

Quoted 5 lines
---
 block/commit.c |  9 ---------
 blockdev.c     | 21 +++++++++++----------
 2 files changed, 11 insertions(+), 19 deletions(-)

As long as you are touching this code:

blockdev.c void qmp_block_commit(const char *device, · line 1175

Quoted 5 lines
blockdev.c +1 -0
@@ -1182,6 +1172,17 @@ void qmp_block_commit(const char *device,
         return;
     }
 
+    if (base && has_base) {

please swap this to 'has_base && base' to avoid any potential of valgrind warnings about conditional jump based on uninitialized memory.

Also, I raised another bug[1] about a bad error message regarding top_bs, if the user passes a different spelling than the canonical name of the active image. Is that worth fixing in this series, or is it okay to leave it until you actually add support for committing the top image?

[1] https://lists.gnu.org/archive/html/qemu-devel/2012-10/msg02067.html

Signature · 2 lines
Eric Blake   [email protected]    +1-919-301-3266
Libvirt virtualization library http://libvirt.org
author
#6 lore raw

Re: [Qemu-devel] [PATCH v2 2/4] block: in commit, determine base image from the top image

Quoted 16 lines · Eric Blake
Quoted 7 lines · Jeff Cody

This simplifies some code and error checking, and also fixes a bug.

bdrv_find_backing_image() should only be passed absolute filenames, or filenames relative to the chain. In the QMP message handler for block commit, when looking up the base do so from the determined top image, so we know it is reachable from top.

Quoted 5 lines
---
 block/commit.c |  9 ---------
 blockdev.c     | 21 +++++++++++----------
 2 files changed, 11 insertions(+), 19 deletions(-)

As long as you are touching this code:

Quoted 9 lines

blockdev.c void qmp_block_commit(const char *device, · line 1175

Quoted 5 lines
blockdev.c +1 -0
@@ -1182,6 +1172,17 @@ void qmp_block_commit(const char *device,
         return;
     }
 
+    if (base && has_base) {

please swap this to 'has_base && base' to avoid any potential of valgrind warnings about conditional jump based on uninitialized memory.

OK.

Quoted 7 lines

Also, I raised another bug[1] about a bad error message regarding top_bs, if the user passes a different spelling than the canonical name of the active image. Is that worth fixing in this series, or is it okay to leave it until you actually add support for committing the top image?

[1] https://lists.gnu.org/archive/html/qemu-devel/2012-10/msg02067.html

Since this doesn't pose an actual issue now, I was planning on just addressing that with the stage-2 patches, since that will likely touch other related areas of the code anyway. If you have major heartburn over this, I'll change it now.

#7 lore raw

Re: [Qemu-devel] [PATCH v2 2/4] block: in commit, determine base image from the top image

Quoted 5 lines · Jeff Cody
Quoted 5 lines

Also, I raised another bug[1] about a bad error message regarding top_bs, if the user passes a different spelling than the canonical name of the active image. Is that worth fixing in this series, or is it okay to leave it until you actually add support for committing the top image?

Quoted 7 lines
Quoted 2 lines

[1] https://lists.gnu.org/archive/html/qemu-devel/2012-10/msg02067.html

Since this doesn't pose an actual issue now, I was planning on just addressing that with the stage-2 patches, since that will likely touch other related areas of the code anyway. If you have major heartburn over this, I'll change it now.

Nah, waiting is fine. As I said in the original bug report, it is only a poor quality error message at the moment, and fixing it may be easier once you have additional pieces in place for committing the active image. I just wanted to make sure the report wasn't lost.

Signature · 2 lines
Eric Blake   [email protected]    +1-919-301-3266
Libvirt virtualization library http://libvirt.org
author
#8 lore raw

PATCH v2 3/4 qemu-iotests: qemu-io tests update for block-commit (040)

In reply to Jeff Cody

Some of the error messages put out by block-commit have changed slightly, which causes 2 tests cases for block-commit to fail. This patch updates the test cases to look for the correct error output.

---
 tests/qemu-iotests/040 | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
tests/qemu-iotests/040 +2 -2
index 258e7ea..0fa6441 100755
@@ -111,7 +111,7 @@ class TestSingleDrive(ImageCommitTestCase):
         self.assert_no_active_commit()
         result = self.vm.qmp('block-commit', device='drive0', top='%s' % backing_img, base='%s' % backing_img)
         self.assert_qmp(result, 'error/class', 'GenericError')
-        self.assert_qmp(result, 'error/desc', 'Invalid files for merge: top and base are the same')
+        self.assert_qmp(result, 'error/desc', 'Base \'%s\' not found' % backing_img)
 
     def test_top_invalid(self):
         self.assert_no_active_commit()
@@ -135,7 +135,7 @@ class TestSingleDrive(ImageCommitTestCase):
         self.assert_no_active_commit()
         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 (%(1)s) is not reachable from top (%(2)s)' % {"1" : mid_img, "2" : backing_img})
+        self.assert_qmp(result, 'error/desc', 'Base \'%s\' not found' % mid_img)
 
     def test_top_omitted(self):
         self.assert_no_active_commit()
Signature · 1 line
1.7.11.4
author
#9 lore raw

PATCH v2 4/4 qemu-iotests: add relative backing file tests for block-commit (040)

In reply to Jeff Cody

The previous block commit used absolute filenames for all block-commit images and commands; this adds relative filenames for the same tests.

---
 tests/qemu-iotests/040     | 102 +++++++++++++++++++++++++++++++++++++++++++++
 tests/qemu-iotests/040.out |   4 +-
 2 files changed, 104 insertions(+), 2 deletions(-)
tests/qemu-iotests/040 +102 -0
index 0fa6441..aad535a 100755
@@ -26,6 +26,7 @@ import os
 import iotests
 from iotests import qemu_img, qemu_io
 import struct
+import errno
 
 backing_img = os.path.join(iotests.test_dir, 'backing.img')
 mid_img = os.path.join(iotests.test_dir, 'mid.img')
@@ -143,6 +144,107 @@ class TestSingleDrive(ImageCommitTestCase):
         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
+    test_len = 1 * 1024 * 256
+
+    dir1 = "dir1"
+    dir2 = "dir2/"
+    dir3 = "dir2/dir3/"
+
+    test_img = os.path.join(iotests.test_dir, dir3, 'test.img')
+    mid_img = "../mid.img"
+    backing_img = "../dir1/backing.img"
+
+    backing_img_abs = os.path.join(iotests.test_dir, dir1, 'backing.img')
+    mid_img_abs = os.path.join(iotests.test_dir, dir2, 'mid.img')
+
+    def setUp(self):
+        try:
+            os.mkdir(os.path.join(iotests.test_dir, self.dir1))
+            os.mkdir(os.path.join(iotests.test_dir, self.dir2))
+            os.mkdir(os.path.join(iotests.test_dir, self.dir3))
+        except OSError as exception:
+            if exception.errno != errno.EEXIST:
+                raise
+        self.create_image(self.backing_img_abs, TestRelativePaths.image_len)
+        qemu_img('create', '-f', iotests.imgfmt, '-o', 'backing_file=%s' % self.backing_img_abs, self.mid_img_abs)
+        qemu_img('create', '-f', iotests.imgfmt, '-o', 'backing_file=%s' % self.mid_img_abs, self.test_img)
+        qemu_img('rebase', '-u', '-b', self.backing_img, self.mid_img_abs)
+        qemu_img('rebase', '-u', '-b', self.mid_img, self.test_img)
+        qemu_io('-c', 'write -P 0xab 0 524288', self.backing_img_abs)
+        qemu_io('-c', 'write -P 0xef 524288 524288', self.mid_img_abs)
+        self.vm = iotests.VM().add_drive(self.test_img)
+        self.vm.launch()
+
+    def tearDown(self):
+        self.vm.shutdown()
+        os.remove(self.test_img)
+        os.remove(self.mid_img_abs)
+        os.remove(self.backing_img_abs)
+        try:
+            os.rmdir(os.path.join(iotests.test_dir, self.dir1))
+            os.rmdir(os.path.join(iotests.test_dir, self.dir3))
+            os.rmdir(os.path.join(iotests.test_dir, self.dir2))
+        except OSError as exception:
+            if exception.errno != errno.EEXIST and exception.errno != errno.ENOTEMPTY:
+                raise
+
+    def test_commit(self):
+        self.assert_no_active_commit()
+        result = self.vm.qmp('block-commit', device='drive0', top='%s' % self.mid_img)
+        self.assert_qmp(result, 'return', {})
+
+        completed = False
+        while not completed:
+            for event in self.vm.get_qmp_events(wait=True):
+                if event['event'] == 'BLOCK_JOB_COMPLETED':
+                    self.assert_qmp(event, 'data/type', 'commit')
+                    self.assert_qmp(event, 'data/device', 'drive0')
+                    self.assert_qmp(event, 'data/offset', self.image_len)
+                    self.assert_qmp(event, 'data/len', self.image_len)
+                    completed = True
+
+        self.assert_no_active_commit()
+        self.vm.shutdown()
+
+        self.assertEqual(-1, qemu_io('-c', 'read -P 0xab 0 524288', self.backing_img_abs).find("verification failed"))
+        self.assertEqual(-1, qemu_io('-c', 'read -P 0xef 524288 524288', self.backing_img_abs).find("verification failed"))
+
+    def test_device_not_found(self):
+        result = self.vm.qmp('block-commit', device='nonexistent', top='%s' % self.mid_img)
+        self.assert_qmp(result, 'error/class', 'DeviceNotFound')
+
+    def test_top_same_base(self):
+        self.assert_no_active_commit()
+        result = self.vm.qmp('block-commit', device='drive0', top='%s' % self.mid_img, base='%s' % self.mid_img)
+        self.assert_qmp(result, 'error/class', 'GenericError')
+        self.assert_qmp(result, 'error/desc', 'Base \'%s\' not found' % self.mid_img)
+
+    def test_top_invalid(self):
+        self.assert_no_active_commit()
+        result = self.vm.qmp('block-commit', device='drive0', top='badfile', base='%s' % self.backing_img)
+        self.assert_qmp(result, 'error/class', 'GenericError')
+        self.assert_qmp(result, 'error/desc', 'Top image file badfile not found')
+
+    def test_base_invalid(self):
+        self.assert_no_active_commit()
+        result = self.vm.qmp('block-commit', device='drive0', top='%s' % self.mid_img, base='badfile')
+        self.assert_qmp(result, 'error/class', 'GenericError')
+        self.assert_qmp(result, 'error/desc', 'Base \'badfile\' not found')
+
+    def test_top_is_active(self):
+        self.assert_no_active_commit()
+        result = self.vm.qmp('block-commit', device='drive0', top='%s' % self.test_img, base='%s' % self.backing_img)
+        self.assert_qmp(result, 'error/class', 'GenericError')
+        self.assert_qmp(result, 'error/desc', 'Top image as the active layer is currently unsupported')
+
+    def test_top_and_base_reversed(self):
+        self.assert_no_active_commit()
+        result = self.vm.qmp('block-commit', device='drive0', top='%s' % self.backing_img, base='%s' % self.mid_img)
+        self.assert_qmp(result, 'error/class', 'GenericError')
+        self.assert_qmp(result, 'error/desc', 'Base \'%s\' not found' % self.mid_img)
+
 
 class TestSetSpeed(ImageCommitTestCase):
     image_len = 80 * 1024 * 1024 # MB
tests/qemu-iotests/040.out +2 -2
index dae404e..b6f2576 100644
@@ -1,5 +1,5 @@
-.........
+................
 ----------------------------------------------------------------------
-Ran 9 tests
+Ran 16 tests
 
 OK
Signature · 1 line
1.7.11.4