[Qemu-devel] [PATCH 0/2] qemu-img: Resolve relative backing paths in rebase

[Qemu-devel] [PATCH 0/2] qemu-img: Resolve relative backing paths in rebase

[Max Reitz]

The issue addressed in this series affects relative backing paths given
to qemu-img rebase.  For the rebasing operation itself, qemu-img uses
the target backing path as-is, that is, relative to its working
directory.  But after rebasing, the path is again written as-is into the
overlay, so it is now relative to the overlay.

Simple test case:
$ mkdir /tmp/foo && cd /tmp
$ qemu-img create -f qcow2 foo/base-old.qcow2 64M
Formatting 'foo/base-old.qcow2', fmt=qcow2 size=67108864 cluster_size=65536 lazy_refcounts=off refcount_bits=16
$ qemu-img create -f qcow2 foo/base-new.qcow2 64M
Formatting 'foo/base-new.qcow2', fmt=qcow2 size=67108864 cluster_size=65536 lazy_refcounts=off refcount_bits=16
$ qemu-img create -f qcow2 -b base-old.qcow2 foo/overlay.qcow2 64M
Formatting 'foo/overlay.qcow2', fmt=qcow2 size=67108864 backing_file=base-old.qcow2 cluster_size=65536 lazy_refcounts=off refcount_bits=16
$ qemu-img rebase -b base-new.qcow2 foo/overlay.qcow2
qemu-img: Could not open new backing file 'base-new.qcow2': Could not open 'base-new.qcow2': No such file or directory

Oops.  Now one may be tempted to try:

$ qemu-img rebase -b foo/base-new.qcow2 foo/overlay.qcow2
$ echo $?
0

However:

$ qemu-img info foo/overlay.qcow2
[...]
backing file: foo/base-new.qcow2 (actual path: foo/foo/base-new.qcow2)
[...]
$ qemu-io -c close foo/overlay.qcow2
can't open device foo/overlay.qcow2: Could not open backing file: Could not open 'foo/foo/base-new.qcow2': No such file or directory

Oops.

So the only way to do it before this series is:

$ qemu-img rebase -b foo/base-new.qcow2 foo/overlay.qcow2
$ qemu-img rebase -u -b base-new.qcow2 foo/overlay.qcow2

That does not seem right to me.

So this series makes qemu-img rebase always interpret the target backing
path relatively to the overlay image, because that is how we always
interpret relative backing paths.


Max Reitz (2):
  qemu-img: Resolve relative backing paths in rebase
  iotests: Add test for rebasing with relative paths

 qemu-img.c                 | 22 ++++++++++++-
 tests/qemu-iotests/024     | 82 ++++++++++++++++++++++++++++++++++++++++++++--
 tests/qemu-iotests/024.out | 30 +++++++++++++++++
 3 files changed, 130 insertions(+), 4 deletions(-)

-- 
2.14.3

[Qemu-devel] [PATCH 1/2] qemu-img: Resolve relative backing paths in rebase

[Max Reitz]

Currently, rebase interprets a relative path for the new backing image
as follows:
(1) Open the new backing image with the given relative path (thus relative to
    qemu-img's working directory).
(2) Write it directly into the overlay's backing path field (thus
    relative to the overlay).

If the overlay is not in qemu-img's working directory, both will be
different interpretations, which may either lead to an error somewhere
(either rebase fails because it cannot open the new backing image, or
 your overlay becomes unusable because its backing path does not point
 to a file), or, even worse, it may result in your rebase being
performed for a different backing file than what your overlay will point
to after the rebase.

Fix this by interpreting the target backing path as relative to the
overlay, like qemu-img does everywhere else.

Buglink: https://bugzilla.redhat.com/show_bug.cgi?id=1569835
Cc: qemu-stable@nongnu.org
Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 qemu-img.c | 22 +++++++++++++++++++++-
 1 file changed, 21 insertions(+), 1 deletion(-)

diff --git a/qemu-img.c b/qemu-img.c
index ea62d2d61e..50540519e3 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -3191,6 +3191,9 @@ static int img_rebase(int argc, char **argv)
         }
 
         if (out_baseimg[0]) {
+            const char *overlay_filename;
+            char *out_real_path;
+
             options = qdict_new();
             if (out_basefmt) {
                 qdict_put_str(options, "driver", out_basefmt);
@@ -3199,8 +3202,25 @@ static int img_rebase(int argc, char **argv)
                 qdict_put_bool(options, BDRV_OPT_FORCE_SHARE, true);
             }
 
-            blk_new_backing = blk_new_open(out_baseimg, NULL,
+            overlay_filename = bs->exact_filename[0] ? bs->exact_filename
+                                                     : bs->filename;
+            out_real_path = g_malloc(PATH_MAX);
+
+            bdrv_get_full_backing_filename_from_filename(overlay_filename,
+                                                         out_baseimg,
+                                                         out_real_path,
+                                                         PATH_MAX,
+                                                         &local_err);
+            if (local_err) {
+                error_reportf_err(local_err,
+                                  "Could not resolve backing filename: ");
+                ret = -1;
+                goto out;
+            }
+
+            blk_new_backing = blk_new_open(out_real_path, NULL,
                                            options, src_flags, &local_err);
+            g_free(out_real_path);
             if (!blk_new_backing) {
                 error_reportf_err(local_err,
                                   "Could not open new backing file '%s': ",
-- 
2.14.3

Re: [Qemu-devel] [PATCH 1/2] qemu-img: Resolve relative backing paths in rebase

[Eric Blake]

On 05/09/2018 10:49 AM, Max Reitz wrote:
> Currently, rebase interprets a relative path for the new backing image
> as follows:
> (1) Open the new backing image with the given relative path (thus relative to
>      qemu-img's working directory).
> (2) Write it directly into the overlay's backing path field (thus
>      relative to the overlay).
> 
> If the overlay is not in qemu-img's working directory, both will be
> different interpretations, which may either lead to an error somewhere
> (either rebase fails because it cannot open the new backing image, or
>   your overlay becomes unusable because its backing path does not point
>   to a file), or, even worse, it may result in your rebase being
> performed for a different backing file than what your overlay will point
> to after the rebase.
> 
> Fix this by interpreting the target backing path as relative to the
> overlay, like qemu-img does everywhere else.
> 
> Buglink: https://bugzilla.redhat.com/show_bug.cgi?id=1569835
> Cc: qemu-stable@nongnu.org
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---

> -            blk_new_backing = blk_new_open(out_baseimg, NULL,
> +            overlay_filename = bs->exact_filename[0] ? bs->exact_filename
> +                                                     : bs->filename;
> +            out_real_path = g_malloc(PATH_MAX);
> +
> +            bdrv_get_full_backing_filename_from_filename(overlay_filename,
> +                                                         out_baseimg,
> +                                                         out_real_path,
> +                                                         PATH_MAX,
> +                                                         &local_err);
> +            if (local_err) {
> +                error_reportf_err(local_err,
> +                                  "Could not resolve backing filename: ");
> +                ret = -1;
> +                goto out;

Leaks out_real_path.

> +            }
> +
> +            blk_new_backing = blk_new_open(out_real_path, NULL,
>                                              options, src_flags, &local_err);
> +            g_free(out_real_path);

Otherwise looks good.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

Re: [Qemu-devel] [Qemu-block] [PATCH 1/2] qemu-img: Resolve relative backing paths in rebase

[Max Reitz]

[-- Attachment #1: Type: text/plain, Size: 3096 bytes --]

On 2018-05-09 19:20, Eric Blake wrote:
> On 05/09/2018 10:49 AM, Max Reitz wrote:
>> Currently, rebase interprets a relative path for the new backing image
>> as follows:
>> (1) Open the new backing image with the given relative path (thus
>> relative to
>>      qemu-img's working directory).
>> (2) Write it directly into the overlay's backing path field (thus
>>      relative to the overlay).
>>
>> If the overlay is not in qemu-img's working directory, both will be
>> different interpretations, which may either lead to an error somewhere
>> (either rebase fails because it cannot open the new backing image, or
>>   your overlay becomes unusable because its backing path does not point
>>   to a file), or, even worse, it may result in your rebase being
>> performed for a different backing file than what your overlay will point
>> to after the rebase.
>>
>> Fix this by interpreting the target backing path as relative to the
>> overlay, like qemu-img does everywhere else.
>>
>> Buglink: https://bugzilla.redhat.com/show_bug.cgi?id=1569835
>> Cc: qemu-stable@nongnu.org
>> Signed-off-by: Max Reitz <mreitz@redhat.com>
>> ---
> 
>> -            blk_new_backing = blk_new_open(out_baseimg, NULL,
>> +            overlay_filename = bs->exact_filename[0] ?
>> bs->exact_filename
>> +                                                     : bs->filename;
>> +            out_real_path = g_malloc(PATH_MAX);
>> +
>> +           
>> bdrv_get_full_backing_filename_from_filename(overlay_filename,
>> +                                                         out_baseimg,
>> +                                                         out_real_path,
>> +                                                         PATH_MAX,
>> +                                                         &local_err);
>> +            if (local_err) {
>> +                error_reportf_err(local_err,
>> +                                  "Could not resolve backing
>> filename: ");
>> +                ret = -1;
>> +                goto out;
> 
> Leaks out_real_path.

Well, “leaks” is relative, considering that out: will effectively exit
the process immediately, but I can’t claim it was on purpose. O:-)

(I should have trusted my gut feeling that the single g_free() can’t
possibly be enough...)

Max

>> +            }
>> +
>> +            blk_new_backing = blk_new_open(out_real_path, NULL,
>>                                              options, src_flags,
>> &local_err);
>> +            g_free(out_real_path);
> 
> Otherwise looks good.
> 



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[Qemu-devel] [PATCH 2/2] iotests: Add test for rebasing with relative paths

[Max Reitz]

Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 tests/qemu-iotests/024     | 82 ++++++++++++++++++++++++++++++++++++++++++++--
 tests/qemu-iotests/024.out | 30 +++++++++++++++++
 2 files changed, 109 insertions(+), 3 deletions(-)

diff --git a/tests/qemu-iotests/024 b/tests/qemu-iotests/024
index e0d77ce2f5..4071ed6093 100755
--- a/tests/qemu-iotests/024
+++ b/tests/qemu-iotests/024
@@ -29,9 +29,14 @@ status=1	# failure is the default!
 
 _cleanup()
 {
-	_cleanup_test_img
-	rm -f "$TEST_DIR/t.$IMGFMT.base_old"
-	rm -f "$TEST_DIR/t.$IMGFMT.base_new"
+    _cleanup_test_img
+    rm -f "$TEST_DIR/t.$IMGFMT.base_old"
+    rm -f "$TEST_DIR/t.$IMGFMT.base_new"
+
+    rm -f "$TEST_DIR/subdir/t.$IMGFMT"
+    rm -f "$TEST_DIR/subdir/t.$IMGFMT.base_old"
+    rm -f "$TEST_DIR/subdir/t.$IMGFMT.base_new"
+    rmdir "$TEST_DIR/subdir" 2> /dev/null
 }
 trap "_cleanup; exit \$status" 0 1 2 3 15
 
@@ -123,6 +128,77 @@ io_pattern readv $((13 * CLUSTER_SIZE)) $CLUSTER_SIZE 0 1 0x00
 io_pattern readv $((14 * CLUSTER_SIZE)) $CLUSTER_SIZE 0 1 0x11
 io_pattern readv $((15 * CLUSTER_SIZE)) $CLUSTER_SIZE 0 1 0x00
 
+echo
+echo "=== Test rebase in a subdirectory of the working directory ==="
+echo
+
+# Clean up the old images beforehand so they do not interfere with
+# this test
+_cleanup
+
+mkdir "$TEST_DIR/subdir"
+
+# Relative to the overlay
+BASE_OLD_OREL="t.$IMGFMT.base_old"
+BASE_NEW_OREL="t.$IMGFMT.base_new"
+
+# Relative to $TEST_DIR (which is going to be our working directory)
+OVERLAY_WREL="subdir/t.$IMGFMT"
+
+BASE_OLD="$TEST_DIR/subdir/$BASE_OLD_OREL"
+BASE_NEW="$TEST_DIR/subdir/$BASE_NEW_OREL"
+OVERLAY="$TEST_DIR/$OVERLAY_WREL"
+
+# Test done here:
+#
+# Backing (old): 11 11 -- 11
+# Backing (new): -- 22 22 11
+# Overlay:       -- -- -- --
+#
+# Rebasing works, we have verified that above.  Here, we just want to
+# see that rebasing is done for the correct target backing file.
+
+TEST_IMG=$BASE_OLD _make_test_img 1M
+TEST_IMG=$BASE_NEW _make_test_img 1M
+TEST_IMG=$OVERLAY _make_test_img -b "$BASE_OLD_OREL" 1M
+
+echo
+
+$QEMU_IO "$BASE_OLD" \
+    -c "write -P 0x11 $((0 * CLUSTER_SIZE)) $((2 * CLUSTER_SIZE))" \
+    -c "write -P 0x11 $((3 * CLUSTER_SIZE)) $((1 * CLUSTER_SIZE))" \
+    | _filter_qemu_io
+
+$QEMU_IO "$BASE_NEW" \
+    -c "write -P 0x22 $((1 * CLUSTER_SIZE)) $((2 * CLUSTER_SIZE))" \
+    -c "write -P 0x11 $((3 * CLUSTER_SIZE)) $((1 * CLUSTER_SIZE))" \
+    | _filter_qemu_io
+
+echo
+
+pushd "$TEST_DIR" >/dev/null
+$QEMU_IMG rebase -f "$IMGFMT" -b "$BASE_NEW_OREL" "$OVERLAY_WREL"
+popd >/dev/null
+
+# Verify the backing path is correct
+TEST_IMG=$OVERLAY _img_info | grep '^backing file'
+
+echo
+
+# Verify the data is correct
+$QEMU_IO "$OVERLAY" \
+    -c "read -P 0x11 $((0 * CLUSTER_SIZE)) $CLUSTER_SIZE" \
+    -c "read -P 0x11 $((1 * CLUSTER_SIZE)) $CLUSTER_SIZE" \
+    -c "read -P 0x00 $((2 * CLUSTER_SIZE)) $CLUSTER_SIZE" \
+    -c "read -P 0x11 $((3 * CLUSTER_SIZE)) $CLUSTER_SIZE" \
+    | _filter_qemu_io
+
+echo
+
+# Verify that cluster #3 is not allocated (because it is the same in
+# $BASE_OLD and $BASE_NEW)
+$QEMU_IMG map "$OVERLAY" | _filter_qemu_img_map
+
 
 # success, all done
 echo "*** done"
diff --git a/tests/qemu-iotests/024.out b/tests/qemu-iotests/024.out
index 33cfaf5cfc..024dc786b3 100644
--- a/tests/qemu-iotests/024.out
+++ b/tests/qemu-iotests/024.out
@@ -141,4 +141,34 @@ read 65536/65536 bytes at offset 917504
 === IO: pattern 0x00
 read 65536/65536 bytes at offset 983040
 64 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+
+=== Test rebase in a subdirectory of the working directory ===
+
+Formatting 'TEST_DIR/subdir/t.IMGFMT.base_old', fmt=IMGFMT size=1048576
+Formatting 'TEST_DIR/subdir/t.IMGFMT.base_new', fmt=IMGFMT size=1048576
+Formatting 'TEST_DIR/subdir/t.IMGFMT', fmt=IMGFMT size=1048576 backing_file=t.IMGFMT.base_old
+
+wrote 131072/131072 bytes at offset 0
+128 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+wrote 65536/65536 bytes at offset 196608
+64 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+wrote 131072/131072 bytes at offset 65536
+128 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+wrote 65536/65536 bytes at offset 196608
+64 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+
+backing file: t.IMGFMT.base_new (actual path: TEST_DIR/subdir/t.IMGFMT.base_new)
+
+read 65536/65536 bytes at offset 0
+64 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+read 65536/65536 bytes at offset 65536
+64 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+read 65536/65536 bytes at offset 131072
+64 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+read 65536/65536 bytes at offset 196608
+64 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+
+Offset          Length          File
+0               0x30000         TEST_DIR/subdir/t.IMGFMT
+0x30000         0x10000         TEST_DIR/subdir/t.IMGFMT.base_new
 *** done
-- 
2.14.3

Re: [Qemu-devel] [PATCH 2/2] iotests: Add test for rebasing with relative paths

[Eric Blake]

On 05/09/2018 10:49 AM, Max Reitz wrote:
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
>   tests/qemu-iotests/024     | 82 ++++++++++++++++++++++++++++++++++++++++++++--
>   tests/qemu-iotests/024.out | 30 +++++++++++++++++
>   2 files changed, 109 insertions(+), 3 deletions(-)
> 
> diff --git a/tests/qemu-iotests/024 b/tests/qemu-iotests/024
> index e0d77ce2f5..4071ed6093 100755
> --- a/tests/qemu-iotests/024
> +++ b/tests/qemu-iotests/024
> @@ -29,9 +29,14 @@ status=1	# failure is the default!
>   
>   _cleanup()
>   {
> -	_cleanup_test_img
> -	rm -f "$TEST_DIR/t.$IMGFMT.base_old"
> -	rm -f "$TEST_DIR/t.$IMGFMT.base_new"
> +    _cleanup_test_img
> +    rm -f "$TEST_DIR/t.$IMGFMT.base_old"
> +    rm -f "$TEST_DIR/t.$IMGFMT.base_new"
> +
> +    rm -f "$TEST_DIR/subdir/t.$IMGFMT"
> +    rm -f "$TEST_DIR/subdir/t.$IMGFMT.base_old"
> +    rm -f "$TEST_DIR/subdir/t.$IMGFMT.base_new"
> +    rmdir "$TEST_DIR/subdir" 2> /dev/null

Any easier to just 'rm -rf "$TEST_DIR/subdir"' instead of calling out 
individual files?  And was Jeff ever going to finish the iotests 
cleanups that took care of cleanup globally by running tests in a 
per-test directory in preparation for running tests in parallel?

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

Re: [Qemu-devel] [Qemu-block] [PATCH 2/2] iotests: Add test for rebasing with relative paths

[Max Reitz]

[-- Attachment #1: Type: text/plain, Size: 1751 bytes --]

On 2018-05-09 19:24, Eric Blake wrote:
> On 05/09/2018 10:49 AM, Max Reitz wrote:
>> Signed-off-by: Max Reitz <mreitz@redhat.com>
>> ---
>>   tests/qemu-iotests/024     | 82
>> ++++++++++++++++++++++++++++++++++++++++++++--
>>   tests/qemu-iotests/024.out | 30 +++++++++++++++++
>>   2 files changed, 109 insertions(+), 3 deletions(-)
>>
>> diff --git a/tests/qemu-iotests/024 b/tests/qemu-iotests/024
>> index e0d77ce2f5..4071ed6093 100755
>> --- a/tests/qemu-iotests/024
>> +++ b/tests/qemu-iotests/024
>> @@ -29,9 +29,14 @@ status=1    # failure is the default!
>>     _cleanup()
>>   {
>> -    _cleanup_test_img
>> -    rm -f "$TEST_DIR/t.$IMGFMT.base_old"
>> -    rm -f "$TEST_DIR/t.$IMGFMT.base_new"
>> +    _cleanup_test_img
>> +    rm -f "$TEST_DIR/t.$IMGFMT.base_old"
>> +    rm -f "$TEST_DIR/t.$IMGFMT.base_new"
>> +
>> +    rm -f "$TEST_DIR/subdir/t.$IMGFMT"
>> +    rm -f "$TEST_DIR/subdir/t.$IMGFMT.base_old"
>> +    rm -f "$TEST_DIR/subdir/t.$IMGFMT.base_new"
>> +    rmdir "$TEST_DIR/subdir" 2> /dev/null
> 
> Any easier to just 'rm -rf "$TEST_DIR/subdir"' instead of calling out
> individual files?

Sure it would be easier, but I decided against it on the basis that I
personally don’t like to call rm -rf on cleanup.

(Because if I mess something up, I might hurt somebody really bad, and
that somebody might be myself.)

> And was Jeff ever going to finish the iotests
> cleanups that took care of cleanup globally by running tests in a
> per-test directory in preparation for running tests in parallel?

If that was a question to me, I’m afraid I don’t know the status of that...

Max

> Reviewed-by: Eric Blake <eblake@redhat.com>



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]