block-commit fixes

PATCH In review Jeff Cody 3 participants 7 messages Oct 16, 2012 – Oct 17, 2012 Block layer

lore

Share & tools

lore

Jeff Cody

author

2012-10-16 19:49 UTC #0 lore raw

v3 cover block-commit fixes

v2 -> v3 differences:

* Simplified code in patch 2, minor commit rewording
* Made conditional check more valgrind-friendly

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                    |  60 ++++++++++++++++++++++---
 block/commit.c             |   9 ----
 blockdev.c                 |  21 ++++-----
 tests/qemu-iotests/040     | 106 ++++++++++++++++++++++++++++++++++++++++++++-
 tests/qemu-iotests/040.out |   4 +-
 5 files changed, 171 insertions(+), 29 deletions(-)

Signature · 1 line

1.7.11.4

Jeff Cody

author

2012-10-16 19:49 UTC #1 lore raw

PATCH v3 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 not both 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.

If either backing_file or bs->backing_file is determine to be a protocol, then no filename normalization is performed.

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

Signed-off-by Jeff Cody <[email protected]>

---
 block.c | 60 ++++++++++++++++++++++++++++++++++++++++++++++++++++++------
 1 file changed, 54 insertions(+), 6 deletions(-)

block.c +54 -6

index e95f613..5e7fc9e 100644
@@ -3123,22 +3123,70 @@ 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(PATH_MAX);
+    backing_file_full = g_malloc(PATH_MAX);
+    filename_tmp      = g_malloc(PATH_MAX);
+
+    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;
+    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

Jeff Cody

author

2012-10-16 19:49 UTC #2 lore raw

PATCH v3 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.

Signed-off-by Jeff Cody <[email protected]>

---
 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..46e4bbd 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 (has_base && 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

Jeff Cody

author

2012-10-16 19:49 UTC #3 lore raw

PATCH v3 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.

Signed-off-by Jeff Cody <[email protected]>

---
 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

Jeff Cody

author

2012-10-16 19:49 UTC #4 lore raw

PATCH v3 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.

Signed-off-by Jeff Cody <[email protected]>

---
 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

Eric Blake

Reviewed-by on the v3 series

2012-10-16 20:33 UTC #5 lore raw

In reply to Jeff Cody

Quoted 14 lines · Jeff Cody

v2 -> v3 differences:
* Simplified code in patch 2, minor commit rewording
* Made conditional check more valgrind-friendly
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

Quoted 5 lines

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)

Should patch 3/4 be squashed into 2/4 to avoid 'git bisect' failures when running the testsuite? Other than that, series:

Reviewed-by Eric Blake <[email protected]>

Signature · 2 lines

Eric Blake   [email protected]    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

Kevin Wolf

2012-10-17 09:46 UTC #6 lore raw

Am 16.10.2012 22:33, schrieb Eric Blake:

Quoted 15 lines · Jeff Cody

Quoted 14 lines
v2 -> v3 differences:
* Simplified code in patch 2, minor commit rewording
* Made conditional check more valgrind-friendly
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

Quoted 8 lines

Quoted 5 lines
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)
Should patch 3/4 be squashed into 2/4 to avoid 'git bisect' failures when running the testsuite? Other than that, series:

Yes, I agree. Applied all and squashed 3/4 into 2/4.

Kevin