block-commit fixes
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
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
@@ -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
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_imagecomparesbs->backing_filewith what is passed in as abacking_filename. Mismatches may occur, however, whenbs->backing_fileandbacking_fileare 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 userealpath()to make all comparisons based on absolute filenames.This also changes
bdrv_find_backing_imageto no longer be recursive, but iterative.Signed-off-by Jeff Cody <[email protected]>--- 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
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_imagecomparesbs->backing_filewith what is passed in as abacking_filename. Mismatches may occur, however, whenbs->backing_fileandbacking_fileare both notReads 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 userealpath()to make all comparisons based on absolute filenames.This also changes
bdrv_find_backing_imageto no longer be recursive, but iterative.Signed-off-by Jeff Cody <[email protected]>--- 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_protocolandpath_has_protocol()give different answers, then thestrcmp()will fail?
Yes, since is_protocol is determined via path_has_protocol() - if they are different, then by definition strcmp() should fail.
Jeff
PATCH v2 2/4 block: in commit, determine base image from the top image
In reply to Jeff CodyThis 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
@@ -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
@@ -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
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
Signed-off-by Jeff Cody <[email protected]>--- 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
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
Signed-off-by Jeff Cody <[email protected]>--- 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.
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
PATCH v2 3/4 qemu-iotests: qemu-io tests update for block-commit (040)
In reply to Jeff CodySome 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
@@ -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
PATCH v2 4/4 qemu-iotests: add relative backing file tests for block-commit (040)
In reply to Jeff CodyThe 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
@@ -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
@@ -1,5 +1,5 @@ -......... +................ ---------------------------------------------------------------------- -Ran 9 tests +Ran 16 tests OK
Signature · 1 line
1.7.11.4