* [Qemu-devel] [PATCH v2 1/4] block: make bdrv_find_backing_image compare canonical filenames
2012-10-16 14:44 [Qemu-devel] [PATCH v2 0/4] block-commit fixes Jeff Cody
@ 2012-10-16 14:44 ` Jeff Cody
2012-10-16 15:12 ` Eric Blake
2012-10-16 14:44 ` [Qemu-devel] [PATCH v2 2/4] block: in commit, determine base image from the top image Jeff Cody
` (2 subsequent siblings)
3 siblings, 1 reply; 10+ messages in thread
From: Jeff Cody @ 2012-10-16 14:44 UTC (permalink / raw)
To: qemu-devel; +Cc: kwolf
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.
Signed-off-by: Jeff Cody <jcody@redhat.com>
---
block.c | 64 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++------
1 file changed, 58 insertions(+), 6 deletions(-)
diff --git a/block.c b/block.c
index e95f613..165905a 100644
--- a/block.c
+++ b/block.c
@@ -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)
--
1.7.11.4
^ permalink raw reply related [flat|nested] 10+ messages in thread* [Qemu-devel] [PATCH v2 2/4] block: in commit, determine base image from the top image
2012-10-16 14:44 [Qemu-devel] [PATCH v2 0/4] block-commit fixes Jeff Cody
2012-10-16 14:44 ` [Qemu-devel] [PATCH v2 1/4] block: make bdrv_find_backing_image compare canonical filenames Jeff Cody
@ 2012-10-16 14:44 ` Jeff Cody
2012-10-16 15:22 ` Eric Blake
2012-10-16 14:44 ` [Qemu-devel] [PATCH v2 3/4] qemu-iotests: qemu-io tests update for block-commit (040) Jeff Cody
2012-10-16 14:44 ` [Qemu-devel] [PATCH v2 4/4] qemu-iotests: add relative backing file tests " Jeff Cody
3 siblings, 1 reply; 10+ messages in thread
From: Jeff Cody @ 2012-10-16 14:44 UTC (permalink / raw)
To: qemu-devel; +Cc: kwolf
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 <jcody@redhat.com>
---
block/commit.c | 9 ---------
blockdev.c | 21 +++++++++++----------
2 files changed, 11 insertions(+), 19 deletions(-)
diff --git a/block/commit.c b/block/commit.c
index 733c914..13d9e82 100644
--- a/block/commit.c
+++ b/block/commit.c
@@ -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) {
diff --git a/blockdev.c b/blockdev.c
index 99828ad..7052287 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -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) {
--
1.7.11.4
^ permalink raw reply related [flat|nested] 10+ messages in thread* [Qemu-devel] [PATCH v2 3/4] qemu-iotests: qemu-io tests update for block-commit (040)
2012-10-16 14:44 [Qemu-devel] [PATCH v2 0/4] block-commit fixes Jeff Cody
2012-10-16 14:44 ` [Qemu-devel] [PATCH v2 1/4] block: make bdrv_find_backing_image compare canonical filenames Jeff Cody
2012-10-16 14:44 ` [Qemu-devel] [PATCH v2 2/4] block: in commit, determine base image from the top image Jeff Cody
@ 2012-10-16 14:44 ` Jeff Cody
2012-10-16 14:44 ` [Qemu-devel] [PATCH v2 4/4] qemu-iotests: add relative backing file tests " Jeff Cody
3 siblings, 0 replies; 10+ messages in thread
From: Jeff Cody @ 2012-10-16 14:44 UTC (permalink / raw)
To: qemu-devel; +Cc: kwolf
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 <jcody@redhat.com>
---
tests/qemu-iotests/040 | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/tests/qemu-iotests/040 b/tests/qemu-iotests/040
index 258e7ea..0fa6441 100755
--- a/tests/qemu-iotests/040
+++ b/tests/qemu-iotests/040
@@ -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()
--
1.7.11.4
^ permalink raw reply related [flat|nested] 10+ messages in thread* [Qemu-devel] [PATCH v2 4/4] qemu-iotests: add relative backing file tests for block-commit (040)
2012-10-16 14:44 [Qemu-devel] [PATCH v2 0/4] block-commit fixes Jeff Cody
` (2 preceding siblings ...)
2012-10-16 14:44 ` [Qemu-devel] [PATCH v2 3/4] qemu-iotests: qemu-io tests update for block-commit (040) Jeff Cody
@ 2012-10-16 14:44 ` Jeff Cody
3 siblings, 0 replies; 10+ messages in thread
From: Jeff Cody @ 2012-10-16 14:44 UTC (permalink / raw)
To: qemu-devel; +Cc: kwolf
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 <jcody@redhat.com>
---
tests/qemu-iotests/040 | 102 +++++++++++++++++++++++++++++++++++++++++++++
tests/qemu-iotests/040.out | 4 +-
2 files changed, 104 insertions(+), 2 deletions(-)
diff --git a/tests/qemu-iotests/040 b/tests/qemu-iotests/040
index 0fa6441..aad535a 100755
--- a/tests/qemu-iotests/040
+++ b/tests/qemu-iotests/040
@@ -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
diff --git a/tests/qemu-iotests/040.out b/tests/qemu-iotests/040.out
index dae404e..b6f2576 100644
--- a/tests/qemu-iotests/040.out
+++ b/tests/qemu-iotests/040.out
@@ -1,5 +1,5 @@
-.........
+................
----------------------------------------------------------------------
-Ran 9 tests
+Ran 16 tests
OK
--
1.7.11.4
^ permalink raw reply related [flat|nested] 10+ messages in thread