All of lore.kernel.org
 help / color / mirror / Atom feed
* [Qemu-devel] [PATCH v2 0/3] block: Fix backing paths for filenames with colons
@ 2017-05-22 19:52 Max Reitz
  2017-05-22 19:52 ` [Qemu-devel] [PATCH v2 1/3] " Max Reitz
                   ` (3 more replies)
  0 siblings, 4 replies; 8+ messages in thread
From: Max Reitz @ 2017-05-22 19:52 UTC (permalink / raw)
  To: qemu-block; +Cc: qemu-devel, Max Reitz, Kevin Wolf, Eric Blake

There are (at least) two issues with filenames that contain colons when
trying to use relative backing filenames with them, for each of which
there is a patch in this series.

The first patch fixes an issue in the general block layer
(path_combine() does not have the same opinion on what constitutes a
protocol prefix as path_has_protocl()), the second fixes an issue in
file-* (when stripping off the optional "file:" prefix we should not
create a filename that seems to have another valid protocol prefix).

The third patch adds a test.


Bonus info: The following is something I did not change, although it is
weird:
$ mkdir foo && cd foo
$ ../qemu-img create -f qcow2 file:image:base.qcow2 64M
Formatting 'file:image:base.qcow2', fmt=qcow2 size=67108864
    encryption=off cluster_size=65536 lazy_refcounts=off
    refcount_bits=16
$ ../qemu-img create -f qcow2 -b file:image:base.qcow2 file:image:top.qcow2
Formatting 'file:image:top.qcow2', fmt=qcow2 size=67108864
    backing_file=file:image:base.qcow2 encryption=off
    cluster_size=65536 lazy_refcounts=off refcount_bits=16
$ cd ..
$ ./qemu-img info foo/image:top.qcow2
image: foo/image:top.qcow2
file format: qcow2
virtual size: 64M (67108864 bytes)
disk size: 196K
cluster_size: 65536
backing file: file:image:base.qcow2
Format specific information:
    compat: 1.1
    lazy refcounts: false
    refcount bits: 16
    corrupt: false
$ ./qemu-io foo/image:top.qcow2
can't open device foo/image:top.qcow2: Could not open backing file:
    Could not open './image:base.qcow2': No such file or directory

Patch 3 adds a comment to the test which explains this behavior in a bit
more detail, but the gist is this: qemu generally thinks that
protocol-prefixed filenames are absolute filenames; but from a
file-posix perspective, "file:foo" is still a relative filename.

I think the above behavior is what we want and that it's the best we can
do, but it still is weird, so I just wanted to mention it.


v2:
- Kept patch 1 unchanged
- Added patch 2
- Extended patch 3 (did NOT change the patch title)


git-backport-diff against v2:

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/3:[----] [--] 'block: Fix backing paths for filenames with colons'
002/3:[down] 'block/file-*: *_parse_filename() and colons'
003/3:[0036] [FC] 'iotests: Add test for colon handling'


Max Reitz (3):
  block: Fix backing paths for filenames with colons
  block/file-*: *_parse_filename() and colons
  iotests: Add test for colon handling

 include/block/block_int.h  |   3 ++
 block.c                    |  50 ++++++++++++++++++---
 block/file-posix.c         |  17 ++------
 block/file-win32.c         |  12 +-----
 tests/qemu-iotests/126     | 105 +++++++++++++++++++++++++++++++++++++++++++++
 tests/qemu-iotests/126.out |  23 ++++++++++
 tests/qemu-iotests/group   |   1 +
 7 files changed, 182 insertions(+), 29 deletions(-)
 create mode 100755 tests/qemu-iotests/126
 create mode 100644 tests/qemu-iotests/126.out

-- 
2.9.4

^ permalink raw reply	[flat|nested] 8+ messages in thread

* [Qemu-devel] [PATCH v2 1/3] block: Fix backing paths for filenames with colons
  2017-05-22 19:52 [Qemu-devel] [PATCH v2 0/3] block: Fix backing paths for filenames with colons Max Reitz
@ 2017-05-22 19:52 ` Max Reitz
  2017-05-22 19:52 ` [Qemu-devel] [PATCH v2 2/3] block/file-*: *_parse_filename() and colons Max Reitz
                   ` (2 subsequent siblings)
  3 siblings, 0 replies; 8+ messages in thread
From: Max Reitz @ 2017-05-22 19:52 UTC (permalink / raw)
  To: qemu-block; +Cc: qemu-devel, Max Reitz, Kevin Wolf, Eric Blake

path_combine() naturally tries to preserve a protocol prefix. However,
it recognizes such a prefix by scanning for the first colon; which is
different from what path_has_protocol() does: There only is a protocol
prefix if there is a colon before the first slash.

A protocol prefix that is not recognized by path_has_protocol() is none,
and should thus not be taken as one.

Case in point, before this patch:
$ ./qemu-img create -f qcow2 -b backing.qcow2 ./top:image.qcow2
qemu-img: ./top:image.qcow2: Could not open './top:backing.qcow2':
    No such file or directory

Afterwards:
$ ./qemu-img create -f qcow2 -b backing.qcow2 ./top:image.qcow2
qemu-img: ./top:image.qcow2: Could not open './backing.qcow2':
    No such file or directory

Reported-by: yangyang <yangyang@redhat.com>
Signed-off-by: Max Reitz <mreitz@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
---
 block.c | 15 ++++++++++-----
 1 file changed, 10 insertions(+), 5 deletions(-)

diff --git a/block.c b/block.c
index 50ba264..b72b872 100644
--- a/block.c
+++ b/block.c
@@ -163,11 +163,16 @@ void path_combine(char *dest, int dest_size,
     if (path_is_absolute(filename)) {
         pstrcpy(dest, dest_size, filename);
     } else {
-        p = strchr(base_path, ':');
-        if (p)
-            p++;
-        else
-            p = base_path;
+        const char *protocol_stripped = NULL;
+
+        if (path_has_protocol(base_path)) {
+            protocol_stripped = strchr(base_path, ':');
+            if (protocol_stripped) {
+                protocol_stripped++;
+            }
+        }
+        p = protocol_stripped ?: base_path;
+
         p1 = strrchr(base_path, '/');
 #ifdef _WIN32
         {
-- 
2.9.4

^ permalink raw reply related	[flat|nested] 8+ messages in thread

* [Qemu-devel] [PATCH v2 2/3] block/file-*: *_parse_filename() and colons
  2017-05-22 19:52 [Qemu-devel] [PATCH v2 0/3] block: Fix backing paths for filenames with colons Max Reitz
  2017-05-22 19:52 ` [Qemu-devel] [PATCH v2 1/3] " Max Reitz
@ 2017-05-22 19:52 ` Max Reitz
  2017-05-22 20:02   ` Eric Blake
  2017-05-22 19:52 ` [Qemu-devel] [PATCH v2 3/3] iotests: Add test for colon handling Max Reitz
  2017-05-26 17:30 ` [Qemu-devel] [PATCH v2 0/3] block: Fix backing paths for filenames with colons Max Reitz
  3 siblings, 1 reply; 8+ messages in thread
From: Max Reitz @ 2017-05-22 19:52 UTC (permalink / raw)
  To: qemu-block; +Cc: qemu-devel, Max Reitz, Kevin Wolf, Eric Blake

The file drivers' *_parse_filename() implementations just strip the
optional protocol prefix off the filename. However, for e.g.
"file:foo:bar", this would lead to "foo:bar" being stored as the BDS's
filename which looks like it should be managed using the "foo" protocol.
This is especially troublesome if you then try to resolve a backing
filename based on "foo:bar".

This issue can only occur if the stripped part is a relative filename
("file:/foo:bar" will be shortened to "/foo:bar" and having a slash
before the first colon means that "/foo" is not recognized as a protocol
part). Therefore, we can easily fix it by prepending "./" to such
filenames.

Before this patch:
$ ./qemu-img create -f qcow2 backing.qcow2 64M
Formatting 'backing.qcow2', fmt=qcow2 size=67108864 encryption=off
    cluster_size=65536 lazy_refcounts=off refcount_bits=16
$ ./qemu-img create -f qcow2 -b backing.qcow2 file:top:image.qcow2
Formatting 'file:top:image.qcow2', fmt=qcow2 size=67108864
    backing_file=backing.qcow2 encryption=off cluster_size=65536
    lazy_refcounts=off refcount_bits=16
$ ./qemu-io file:top:image.qcow2
can't open device file:top:image.qcow2: Could not open backing file:
    Unknown protocol 'top'

After this patch:
$ ./qemu-io file:top:image.qcow2
[no error]

Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 include/block/block_int.h |  3 +++
 block.c                   | 35 +++++++++++++++++++++++++++++++++++
 block/file-posix.c        | 17 +++--------------
 block/file-win32.c        | 12 ++----------
 4 files changed, 43 insertions(+), 24 deletions(-)

diff --git a/include/block/block_int.h b/include/block/block_int.h
index 8d3724c..e5eb473 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -682,6 +682,9 @@ int get_tmp_filename(char *filename, int size);
 BlockDriver *bdrv_probe_all(const uint8_t *buf, int buf_size,
                             const char *filename);
 
+void bdrv_parse_filename_strip_prefix(const char *filename, const char *prefix,
+                                      QDict *options);
+
 
 /**
  * bdrv_add_before_write_notifier:
diff --git a/block.c b/block.c
index b72b872..fa1d06d 100644
--- a/block.c
+++ b/block.c
@@ -197,6 +197,41 @@ void path_combine(char *dest, int dest_size,
     }
 }
 
+/*
+ * Helper function for bdrv_parse_filename() implementations to remove optional
+ * protocol prefixes (especially "file:") from a filename and for putting the
+ * stripped filename into the options QDict if there is such a prefix.
+ */
+void bdrv_parse_filename_strip_prefix(const char *filename, const char *prefix,
+                                      QDict *options)
+{
+    if (strstart(filename, prefix, &filename)) {
+        /* Stripping the explicit protocol prefix may result in a protocol
+         * prefix being (wrongly) detected (if the filename contains a colon) */
+        if (path_has_protocol(filename)) {
+            QString *fat_filename;
+
+            /* This means there is some colon before the first slash; therefore,
+             * this cannot be an absolute path */
+            assert(!path_is_absolute(filename));
+
+            /* And we can thus fix the protocol detection issue by prefixing it
+             * by "./" */
+            fat_filename = qstring_from_str("./");
+            qstring_append(fat_filename, filename);
+
+            assert(!path_has_protocol(qstring_get_str(fat_filename)));
+
+            qdict_put(options, "filename", fat_filename);
+        } else {
+            /* If no protocol prefix was detected, we can use the shortened
+             * filename as-is */
+            qdict_put_str(options, "filename", filename);
+        }
+    }
+}
+
+
 /* Returns whether the image file is opened as read-only. Note that this can
  * return false and writing to the image file is still not possible because the
  * image is inactivated. */
diff --git a/block/file-posix.c b/block/file-posix.c
index 4354d49..de2d3a2 100644
--- a/block/file-posix.c
+++ b/block/file-posix.c
@@ -381,12 +381,7 @@ static void raw_parse_flags(int bdrv_flags, int *open_flags)
 static void raw_parse_filename(const char *filename, QDict *options,
                                Error **errp)
 {
-    /* The filename does not have to be prefixed by the protocol name, since
-     * "file" is the default protocol; therefore, the return value of this
-     * function call can be ignored. */
-    strstart(filename, "file:", &filename);
-
-    qdict_put_str(options, "filename", filename);
+    bdrv_parse_filename_strip_prefix(filename, "file:", options);
 }
 
 static QemuOptsList raw_runtime_opts = {
@@ -2395,10 +2390,7 @@ static int check_hdev_writable(BDRVRawState *s)
 static void hdev_parse_filename(const char *filename, QDict *options,
                                 Error **errp)
 {
-    /* The prefix is optional, just as for "file". */
-    strstart(filename, "host_device:", &filename);
-
-    qdict_put_str(options, "filename", filename);
+    bdrv_parse_filename_strip_prefix(filename, "host_device:", options);
 }
 
 static bool hdev_is_sg(BlockDriverState *bs)
@@ -2697,10 +2689,7 @@ static BlockDriver bdrv_host_device = {
 static void cdrom_parse_filename(const char *filename, QDict *options,
                                  Error **errp)
 {
-    /* The prefix is optional, just as for "file". */
-    strstart(filename, "host_cdrom:", &filename);
-
-    qdict_put_str(options, "filename", filename);
+    bdrv_parse_filename_strip_prefix(filename, "host_cdrom:", options);
 }
 #endif
 
diff --git a/block/file-win32.c b/block/file-win32.c
index 8f14f0b..ef2910b 100644
--- a/block/file-win32.c
+++ b/block/file-win32.c
@@ -276,12 +276,7 @@ static void raw_parse_flags(int flags, bool use_aio, int *access_flags,
 static void raw_parse_filename(const char *filename, QDict *options,
                                Error **errp)
 {
-    /* The filename does not have to be prefixed by the protocol name, since
-     * "file" is the default protocol; therefore, the return value of this
-     * function call can be ignored. */
-    strstart(filename, "file:", &filename);
-
-    qdict_put_str(options, "filename", filename);
+    bdrv_parse_filename_strip_prefix(filename, "file:", options);
 }
 
 static QemuOptsList raw_runtime_opts = {
@@ -671,10 +666,7 @@ static int hdev_probe_device(const char *filename)
 static void hdev_parse_filename(const char *filename, QDict *options,
                                 Error **errp)
 {
-    /* The prefix is optional, just as for "file". */
-    strstart(filename, "host_device:", &filename);
-
-    qdict_put_str(options, "filename", filename);
+    bdrv_parse_filename_strip_prefix(filename, "host_device:", options);
 }
 
 static int hdev_open(BlockDriverState *bs, QDict *options, int flags,
-- 
2.9.4

^ permalink raw reply related	[flat|nested] 8+ messages in thread

* [Qemu-devel] [PATCH v2 3/3] iotests: Add test for colon handling
  2017-05-22 19:52 [Qemu-devel] [PATCH v2 0/3] block: Fix backing paths for filenames with colons Max Reitz
  2017-05-22 19:52 ` [Qemu-devel] [PATCH v2 1/3] " Max Reitz
  2017-05-22 19:52 ` [Qemu-devel] [PATCH v2 2/3] block/file-*: *_parse_filename() and colons Max Reitz
@ 2017-05-22 19:52 ` Max Reitz
  2017-05-22 20:06   ` Eric Blake
  2017-05-26 17:30 ` [Qemu-devel] [PATCH v2 0/3] block: Fix backing paths for filenames with colons Max Reitz
  3 siblings, 1 reply; 8+ messages in thread
From: Max Reitz @ 2017-05-22 19:52 UTC (permalink / raw)
  To: qemu-block; +Cc: qemu-devel, Max Reitz, Kevin Wolf, Eric Blake

Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 tests/qemu-iotests/126     | 105 +++++++++++++++++++++++++++++++++++++++++++++
 tests/qemu-iotests/126.out |  23 ++++++++++
 tests/qemu-iotests/group   |   1 +
 3 files changed, 129 insertions(+)
 create mode 100755 tests/qemu-iotests/126
 create mode 100644 tests/qemu-iotests/126.out

diff --git a/tests/qemu-iotests/126 b/tests/qemu-iotests/126
new file mode 100755
index 0000000..3a2a43a
--- /dev/null
+++ b/tests/qemu-iotests/126
@@ -0,0 +1,105 @@
+#!/bin/bash
+#
+# Tests handling of colons in filenames (which may be confused with protocol
+# prefixes)
+#
+# Copyright (C) 2017 Red Hat, Inc.
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 2 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+#
+
+# creator
+owner=mreitz@redhat.com
+
+seq="$(basename $0)"
+echo "QA output created by $seq"
+
+here="$PWD"
+status=1	# failure is the default!
+
+# get standard environment, filters and checks
+. ./common.rc
+. ./common.filter
+
+# Needs backing file support
+_supported_fmt qcow qcow2 qed vmdk
+# This is the default protocol (and we want to test the difference between
+# colons which separate a protocol prefix from the rest and colons which are
+# just part of the filename, so we cannot test protocols which require a prefix)
+_supported_proto file
+_supported_os Linux
+
+echo
+echo '=== Testing plain files ==='
+echo
+
+# A colon after a slash is not a protocol prefix separator
+TEST_IMG="$TEST_DIR/a:b.$IMGFMT" _make_test_img 64M
+_rm_test_img "$TEST_DIR/a:b.$IMGFMT"
+
+# But if you want to be really sure, you can do this
+TEST_IMG="file:$TEST_DIR/a:b.$IMGFMT" _make_test_img 64M
+_rm_test_img "$TEST_DIR/a:b.$IMGFMT"
+
+
+echo
+echo '=== Testing relative backing filename resolution ==='
+echo
+
+BASE_IMG="$TEST_DIR/image:base.$IMGFMT"
+TOP_IMG="$TEST_DIR/image:top.$IMGFMT"
+
+TEST_IMG=$BASE_IMG _make_test_img 64M
+TEST_IMG=$TOP_IMG _make_test_img -b ./image:base.$IMGFMT
+
+# The default cluster size depends on the image format
+TEST_IMG=$TOP_IMG _img_info | grep -v 'cluster_size'
+
+_rm_test_img "$BASE_IMG"
+_rm_test_img "$TOP_IMG"
+
+
+# Do another test where we access both top and base without any slash in them
+echo
+pushd "$TEST_DIR" >/dev/null
+
+BASE_IMG="base.$IMGFMT"
+TOP_IMG="file:image:top.$IMGFMT"
+
+TEST_IMG=$BASE_IMG _make_test_img 64M
+TEST_IMG=$TOP_IMG _make_test_img -b "$BASE_IMG"
+
+TEST_IMG=$TOP_IMG _img_info | grep -v 'cluster_size'
+
+_rm_test_img "$BASE_IMG"
+_rm_test_img "image:top.$IMGFMT"
+
+popd >/dev/null
+
+# Note that we could also do the same test with BASE_IMG=file:image:base.$IMGFMT
+# -- but behavior for that case is a bit strange. Protocol-prefixed paths are
+# in a sense always absolute paths, so such paths will never be combined with
+# the path of the overlay. But since "image:base.$IMGFMT" is actually a
+# relative path, it will always be evaluated relative to qemu's CWD (but not
+# relative to the overlay!). While this is more or less intended, it is still
+# pretty strange and thus not something that is tested here.
+# (The root of the issue is to use a relative path with a protocol prefix. This
+#  may always give you weird results because in one sense, qemu considers such
+#  paths absolute, whereas in another, they are still relative.)
+
+
+# success, all done
+echo '*** done'
+rm -f $seq.full
+status=0
diff --git a/tests/qemu-iotests/126.out b/tests/qemu-iotests/126.out
new file mode 100644
index 0000000..50d7308
--- /dev/null
+++ b/tests/qemu-iotests/126.out
@@ -0,0 +1,23 @@
+QA output created by 126
+
+=== Testing plain files ===
+
+Formatting 'TEST_DIR/a:b.IMGFMT', fmt=IMGFMT size=67108864
+Formatting 'TEST_DIR/a:b.IMGFMT', fmt=IMGFMT size=67108864
+
+=== Testing relative backing filename resolution ===
+
+Formatting 'TEST_DIR/image:base.IMGFMT', fmt=IMGFMT size=67108864
+Formatting 'TEST_DIR/image:top.IMGFMT', fmt=IMGFMT size=67108864 backing_file=./image:base.IMGFMT
+image: TEST_DIR/image:top.IMGFMT
+file format: IMGFMT
+virtual size: 64M (67108864 bytes)
+backing file: ./image:base.IMGFMT (actual path: TEST_DIR/./image:base.IMGFMT)
+
+Formatting 'base.IMGFMT', fmt=IMGFMT size=67108864
+Formatting 'file:image:top.IMGFMT', fmt=IMGFMT size=67108864 backing_file=base.IMGFMT
+image: ./image:top.IMGFMT
+file format: IMGFMT
+virtual size: 64M (67108864 bytes)
+backing file: base.IMGFMT (actual path: ./base.IMGFMT)
+*** done
diff --git a/tests/qemu-iotests/group b/tests/qemu-iotests/group
index 5c8ea0f..30717cb 100644
--- a/tests/qemu-iotests/group
+++ b/tests/qemu-iotests/group
@@ -130,6 +130,7 @@
 122 rw auto
 123 rw auto quick
 124 rw auto backing
+126 rw auto backing
 128 rw auto quick
 129 rw auto quick
 130 rw auto quick
-- 
2.9.4

^ permalink raw reply related	[flat|nested] 8+ messages in thread

* Re: [Qemu-devel] [PATCH v2 2/3] block/file-*: *_parse_filename() and colons
  2017-05-22 19:52 ` [Qemu-devel] [PATCH v2 2/3] block/file-*: *_parse_filename() and colons Max Reitz
@ 2017-05-22 20:02   ` Eric Blake
  0 siblings, 0 replies; 8+ messages in thread
From: Eric Blake @ 2017-05-22 20:02 UTC (permalink / raw)
  To: Max Reitz, qemu-block; +Cc: qemu-devel, Kevin Wolf

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

On 05/22/2017 02:52 PM, Max Reitz wrote:
> The file drivers' *_parse_filename() implementations just strip the
> optional protocol prefix off the filename. However, for e.g.
> "file:foo:bar", this would lead to "foo:bar" being stored as the BDS's
> filename which looks like it should be managed using the "foo" protocol.
> This is especially troublesome if you then try to resolve a backing
> filename based on "foo:bar".
> 
> This issue can only occur if the stripped part is a relative filename
> ("file:/foo:bar" will be shortened to "/foo:bar" and having a slash
> before the first colon means that "/foo" is not recognized as a protocol
> part). Therefore, we can easily fix it by prepending "./" to such
> filenames.

Slick.

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

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


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

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [Qemu-devel] [PATCH v2 3/3] iotests: Add test for colon handling
  2017-05-22 19:52 ` [Qemu-devel] [PATCH v2 3/3] iotests: Add test for colon handling Max Reitz
@ 2017-05-22 20:06   ` Eric Blake
  2017-05-22 20:17     ` Max Reitz
  0 siblings, 1 reply; 8+ messages in thread
From: Eric Blake @ 2017-05-22 20:06 UTC (permalink / raw)
  To: Max Reitz, qemu-block; +Cc: qemu-devel, Kevin Wolf

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

On 05/22/2017 02:52 PM, Max Reitz wrote:
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
>  tests/qemu-iotests/126     | 105 +++++++++++++++++++++++++++++++++++++++++++++
>  tests/qemu-iotests/126.out |  23 ++++++++++
>  tests/qemu-iotests/group   |   1 +
>  3 files changed, 129 insertions(+)
>  create mode 100755 tests/qemu-iotests/126
>  create mode 100644 tests/qemu-iotests/126.out
> 

> +# Note that we could also do the same test with BASE_IMG=file:image:base.$IMGFMT
> +# -- but behavior for that case is a bit strange. Protocol-prefixed paths are
> +# in a sense always absolute paths, so such paths will never be combined with
> +# the path of the overlay. But since "image:base.$IMGFMT" is actually a
> +# relative path, it will always be evaluated relative to qemu's CWD (but not
> +# relative to the overlay!). While this is more or less intended, it is still
> +# pretty strange and thus not something that is tested here.
> +# (The root of the issue is to use a relative path with a protocol prefix. This

s/to/the/

> +#  may always give you weird results because in one sense, qemu considers such
> +#  paths absolute, whereas in another, they are still relative.)

Should we tighten qemu to forbid the use of a protocol prefix with a
non-absolute path?  But that can be a subsequent patch, I don't see it
as a reason to hold up this one.

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

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


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

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [Qemu-devel] [PATCH v2 3/3] iotests: Add test for colon handling
  2017-05-22 20:06   ` Eric Blake
@ 2017-05-22 20:17     ` Max Reitz
  0 siblings, 0 replies; 8+ messages in thread
From: Max Reitz @ 2017-05-22 20:17 UTC (permalink / raw)
  To: Eric Blake, qemu-block; +Cc: qemu-devel, Kevin Wolf

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

On 2017-05-22 22:06, Eric Blake wrote:
> On 05/22/2017 02:52 PM, Max Reitz wrote:
>> Signed-off-by: Max Reitz <mreitz@redhat.com>
>> ---
>>  tests/qemu-iotests/126     | 105 +++++++++++++++++++++++++++++++++++++++++++++
>>  tests/qemu-iotests/126.out |  23 ++++++++++
>>  tests/qemu-iotests/group   |   1 +
>>  3 files changed, 129 insertions(+)
>>  create mode 100755 tests/qemu-iotests/126
>>  create mode 100644 tests/qemu-iotests/126.out
>>
> 
>> +# Note that we could also do the same test with BASE_IMG=file:image:base.$IMGFMT
>> +# -- but behavior for that case is a bit strange. Protocol-prefixed paths are
>> +# in a sense always absolute paths, so such paths will never be combined with
>> +# the path of the overlay. But since "image:base.$IMGFMT" is actually a
>> +# relative path, it will always be evaluated relative to qemu's CWD (but not
>> +# relative to the overlay!). While this is more or less intended, it is still
>> +# pretty strange and thus not something that is tested here.
>> +# (The root of the issue is to use a relative path with a protocol prefix. This
> 
> s/to/the/

Then it will also have to be s/use a/use of a/.

>> +#  may always give you weird results because in one sense, qemu considers such
>> +#  paths absolute, whereas in another, they are still relative.)
> 
> Should we tighten qemu to forbid the use of a protocol prefix with a
> non-absolute path?  But that can be a subsequent patch, I don't see it
> as a reason to hold up this one.

Hm. I'd rather not do this. It could be considered a bug fix (and it
would make patch 2 obsolete, so it would definitely have a use there),
but it would break compatibility.

The whole filename handling in qemu is a mess, and that's mostly because
users expect it to be a mess. It would be great if we didn't have to
handle filenames at all, or at least only absolute filenames, but that
would not be something that users like. And having little weird things
like these in some corner cases (it's not like many people are using an
explicit "file:" prefix anyway) is better than not supporting relative
filenames at all...

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

Thanks!

Max


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

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [Qemu-devel] [PATCH v2 0/3] block: Fix backing paths for filenames with colons
  2017-05-22 19:52 [Qemu-devel] [PATCH v2 0/3] block: Fix backing paths for filenames with colons Max Reitz
                   ` (2 preceding siblings ...)
  2017-05-22 19:52 ` [Qemu-devel] [PATCH v2 3/3] iotests: Add test for colon handling Max Reitz
@ 2017-05-26 17:30 ` Max Reitz
  3 siblings, 0 replies; 8+ messages in thread
From: Max Reitz @ 2017-05-26 17:30 UTC (permalink / raw)
  To: qemu-block; +Cc: qemu-devel, Kevin Wolf, Eric Blake

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

On 2017-05-22 21:52, Max Reitz wrote:
> There are (at least) two issues with filenames that contain colons when
> trying to use relative backing filenames with them, for each of which
> there is a patch in this series.
> 
> The first patch fixes an issue in the general block layer
> (path_combine() does not have the same opinion on what constitutes a
> protocol prefix as path_has_protocl()), the second fixes an issue in
> file-* (when stripping off the optional "file:" prefix we should not
> create a filename that seems to have another valid protocol prefix).
> 
> The third patch adds a test.

Applied to my block tree, with the spelling in patch 3 fixed as
indicated by Eric.

Max


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

^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, other threads:[~2017-05-26 17:30 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-05-22 19:52 [Qemu-devel] [PATCH v2 0/3] block: Fix backing paths for filenames with colons Max Reitz
2017-05-22 19:52 ` [Qemu-devel] [PATCH v2 1/3] " Max Reitz
2017-05-22 19:52 ` [Qemu-devel] [PATCH v2 2/3] block/file-*: *_parse_filename() and colons Max Reitz
2017-05-22 20:02   ` Eric Blake
2017-05-22 19:52 ` [Qemu-devel] [PATCH v2 3/3] iotests: Add test for colon handling Max Reitz
2017-05-22 20:06   ` Eric Blake
2017-05-22 20:17     ` Max Reitz
2017-05-26 17:30 ` [Qemu-devel] [PATCH v2 0/3] block: Fix backing paths for filenames with colons Max Reitz

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.