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

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

[Max Reitz]

v1 of the cover letter here (no change necessary there):
http://lists.nongnu.org/archive/html/qemu-block/2018-05/msg00243.html

v2: Fixed a leaked buffer in an error path in patch 1 [Eric]


git-backport-diff against v1:

Key:
[----] : patches are identical
[####] : number of functional differences between upstream/downstream patch
[down] : patch is downstream-only
The flags [FC] indicate (F)unctional and (C)ontextual differences, respectively

001/2:[0001] [FC] 'qemu-img: Resolve relative backing paths in rebase'
002/2:[----] [--] 'iotests: Add test for rebasing with relative paths'


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

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

-- 
2.14.3

[Qemu-devel] [PATCH v2 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 | 23 ++++++++++++++++++++++-
 1 file changed, 22 insertions(+), 1 deletion(-)

diff --git a/qemu-img.c b/qemu-img.c
index ea62d2d61e..55b146aa49 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,26 @@ 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;
+                g_free(out_real_path);
+                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 v2 1/2] qemu-img: Resolve relative backing paths in rebase

[Eric Blake]

On 05/09/2018 01:20 PM, 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

Interesting indentation in this paragraph. A maintainer could tweak it.

> 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 | 23 ++++++++++++++++++++++-
>   1 file changed, 22 insertions(+), 1 deletion(-)

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] [PATCH v2 1/2] qemu-img: Resolve relative backing paths in rebase

[Max Reitz]

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

On 2018-05-09 20:51, Eric Blake wrote:
> On 05/09/2018 01:20 PM, 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
> 
> Interesting indentation in this paragraph. A maintainer could tweak it.

Well, it was meant this way (because of the opening paranthesis), but I
could out-dent it.

Max

>> 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 | 23 ++++++++++++++++++++++-
>>   1 file changed, 22 insertions(+), 1 deletion(-)
> 
> Reviewed-by: Eric Blake <eblake@redhat.com>
> 



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

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

[Max Reitz]

Signed-off-by: Max Reitz <mreitz@redhat.com>
Reviewed-by: Eric Blake <eblake@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 v2 0/2] qemu-img: Resolve relative backing paths in rebase

[Max Reitz]

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

On 2018-05-09 20:20, Max Reitz wrote:
> v1 of the cover letter here (no change necessary there):
> http://lists.nongnu.org/archive/html/qemu-block/2018-05/msg00243.html
> 
> v2: Fixed a leaked buffer in an error path in patch 1 [Eric]
> 
> 
> git-backport-diff against v1:
> 
> Key:
> [----] : patches are identical
> [####] : number of functional differences between upstream/downstream patch
> [down] : patch is downstream-only
> The flags [FC] indicate (F)unctional and (C)ontextual differences, respectively
> 
> 001/2:[0001] [FC] 'qemu-img: Resolve relative backing paths in rebase'
> 002/2:[----] [--] 'iotests: Add test for rebasing with relative paths'
> 
> 
> Max Reitz (2):
>   qemu-img: Resolve relative backing paths in rebase
>   iotests: Add test for rebasing with relative paths
> 
>  qemu-img.c                 | 23 ++++++++++++-
>  tests/qemu-iotests/024     | 82 ++++++++++++++++++++++++++++++++++++++++++++--
>  tests/qemu-iotests/024.out | 30 +++++++++++++++++
>  3 files changed, 131 insertions(+), 4 deletions(-)

Applied to my block branch.

Max


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