[Qemu-devel] [PATCH v4 0/2] iotests: Add test for colon handling

[Qemu-devel] [PATCH v4 0/2] iotests: Add test for colon handling

[Max Reitz]

v2 of v3 of "iotests: Add test for colon handling"; thus, basically, v4.

This adds an iotest for the original series "block: Fix backing paths
for filenames with colons" and fixes common.config so it works if you
have specified the qemu binaries through relative paths. As a bonus, it
makes symlinked binaries work for out-of-tree builds.


v4: Use `type -p` instead of `which`, and use -- before the realpath
    FILE argument [Eric]


git-backport-diff against v3:

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:[0010] [FC] 'iotests: Use absolute paths for executables'
002/2:[----] [--] 'iotests: Add test for colon handling'


Max Reitz (2):
  iotests: Use absolute paths for executables
  iotests: Add test for colon handling

 tests/qemu-iotests/126           | 105 +++++++++++++++++++++++++++++++++++++++
 tests/qemu-iotests/126.out       |  23 +++++++++
 tests/qemu-iotests/common.config |   6 +++
 tests/qemu-iotests/group         |   1 +
 4 files changed, 135 insertions(+)
 create mode 100755 tests/qemu-iotests/126
 create mode 100644 tests/qemu-iotests/126.out

-- 
2.9.4

[Qemu-devel] [PATCH v4 1/2] iotests: Use absolute paths for executables

[Max Reitz]

A user may specify a relative path for accessing qemu, qemu-img, etc.
through environment variables ($QEMU_PROG and friends) or a symlink.

If a test decides to change its working directory, relative paths will
cease to work, however. Work around this by making all of the paths to
programs that should undergo testing absolute. Besides "realpath", we
also have to use "which" to support programs in $PATH.

As a side effect, this fixes specifying these programs as symlinks for
out-of-tree builds: Before, you would have to create two symlinks, one
in the build and one in the source tree (the first one for common.config
to find, the second one for the iotest to use). Now it is sufficient to
create one in the build tree because common.config will resolve it.

Reported-by: Kevin Wolf <kwolf@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 tests/qemu-iotests/common.config | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/tests/qemu-iotests/common.config b/tests/qemu-iotests/common.config
index d1b45f5..c1dc425 100644
--- a/tests/qemu-iotests/common.config
+++ b/tests/qemu-iotests/common.config
@@ -103,6 +103,12 @@ if [ -z "$QEMU_VXHS_PROG" ]; then
     export QEMU_VXHS_PROG="`set_prog_path qnio_server`"
 fi
 
+export QEMU_PROG=$(realpath -- "$(type -p "$QEMU_PROG")")
+export QEMU_IMG_PROG=$(realpath -- "$(type -p "$QEMU_IMG_PROG")")
+export QEMU_IO_PROG=$(realpath -- "$(type -p "$QEMU_IO_PROG")")
+export QEMU_NBD_PROG=$(realpath -- "$(type -p "$QEMU_NBD_PROG")")
+export QEMU_VXHS_PROG=$(realpath -- "$(type -p "$QEMU_VXHS_PROG")")
+
 _qemu_wrapper()
 {
     (
-- 
2.9.4

Re: [Qemu-devel] [PATCH v4 1/2] iotests: Use absolute paths for executables

[Eric Blake]

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

On 06/21/2017 08:30 AM, Max Reitz wrote:
> A user may specify a relative path for accessing qemu, qemu-img, etc.
> through environment variables ($QEMU_PROG and friends) or a symlink.
> 
> If a test decides to change its working directory, relative paths will
> cease to work, however. Work around this by making all of the paths to
> programs that should undergo testing absolute. Besides "realpath", we
> also have to use "which" to support programs in $PATH.

Is this sentence stale...

> 
> As a side effect, this fixes specifying these programs as symlinks for
> out-of-tree builds: Before, you would have to create two symlinks, one
> in the build and one in the source tree (the first one for common.config
> to find, the second one for the iotest to use). Now it is sufficient to
> create one in the build tree because common.config will resolve it.
> 
> Reported-by: Kevin Wolf <kwolf@redhat.com>
> Reviewed-by: Eric Blake <eblake@redhat.com>
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
>  tests/qemu-iotests/common.config | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/tests/qemu-iotests/common.config b/tests/qemu-iotests/common.config
> index d1b45f5..c1dc425 100644
> --- a/tests/qemu-iotests/common.config
> +++ b/tests/qemu-iotests/common.config
> @@ -103,6 +103,12 @@ if [ -z "$QEMU_VXHS_PROG" ]; then
>      export QEMU_VXHS_PROG="`set_prog_path qnio_server`"
>  fi
>  
> +export QEMU_PROG=$(realpath -- "$(type -p "$QEMU_PROG")")

...now that you updated per my review to favor 'type' over 'which'?
Otherwise, the R-b stands.

-- 
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 --]

Re: [Qemu-devel] [PATCH v4 1/2] iotests: Use absolute paths for executables

[Max Reitz]

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

On 2017-06-29 20:52, Eric Blake wrote:
> On 06/21/2017 08:30 AM, Max Reitz wrote:
>> A user may specify a relative path for accessing qemu, qemu-img, etc.
>> through environment variables ($QEMU_PROG and friends) or a symlink.
>>
>> If a test decides to change its working directory, relative paths will
>> cease to work, however. Work around this by making all of the paths to
>> programs that should undergo testing absolute. Besides "realpath", we
>> also have to use "which" to support programs in $PATH.
> 
> Is this sentence stale...

Gnarf. Sure is, yes. Thanks for spotting!

>> As a side effect, this fixes specifying these programs as symlinks for
>> out-of-tree builds: Before, you would have to create two symlinks, one
>> in the build and one in the source tree (the first one for common.config
>> to find, the second one for the iotest to use). Now it is sufficient to
>> create one in the build tree because common.config will resolve it.
>>
>> Reported-by: Kevin Wolf <kwolf@redhat.com>
>> Reviewed-by: Eric Blake <eblake@redhat.com>
>> Signed-off-by: Max Reitz <mreitz@redhat.com>
>> ---
>>  tests/qemu-iotests/common.config | 6 ++++++
>>  1 file changed, 6 insertions(+)
>>
>> diff --git a/tests/qemu-iotests/common.config b/tests/qemu-iotests/common.config
>> index d1b45f5..c1dc425 100644
>> --- a/tests/qemu-iotests/common.config
>> +++ b/tests/qemu-iotests/common.config
>> @@ -103,6 +103,12 @@ if [ -z "$QEMU_VXHS_PROG" ]; then
>>      export QEMU_VXHS_PROG="`set_prog_path qnio_server`"
>>  fi
>>  
>> +export QEMU_PROG=$(realpath -- "$(type -p "$QEMU_PROG")")
> 
> ...now that you updated per my review to favor 'type' over 'which'?
> Otherwise, the R-b stands.

Thanks, will fix and apply then...

...and done, applied to my block branch:

https://github.com/XanClic/qemu/commits/block

Max


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

Re: [Qemu-devel] [PATCH v4 1/2] iotests: Use absolute paths for executables

[Eric Blake]

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

On 06/29/2017 09:46 PM, Max Reitz wrote:

>>> +++ b/tests/qemu-iotests/common.config
>>> @@ -103,6 +103,12 @@ if [ -z "$QEMU_VXHS_PROG" ]; then
>>>      export QEMU_VXHS_PROG="`set_prog_path qnio_server`"
>>>  fi
>>>  
>>> +export QEMU_PROG=$(realpath -- "$(type -p "$QEMU_PROG")")
>>
>> ...now that you updated per my review to favor 'type' over 'which'?
>> Otherwise, the R-b stands.
> 
> Thanks, will fix and apply then...
> 
> ...and done, applied to my block branch:
> 
> https://github.com/XanClic/qemu/commits/block

Sorry for not noticing sooner, but you'll need to replace v4 (commit
0f0fec82 on your branch) with a fix, because now your branch does the
following on all iotests for me:

068 3s ... - output mismatch (see 068.out.bad)
--- /home/eblake/qemu/tests/qemu-iotests/068.out	2017-06-26
22:02:56.057734882 -0500
+++ 068.out.bad	2017-06-30 14:35:28.720241398 -0500
@@ -1,4 +1,5 @@
 QA output created by 068
+realpath: '': No such file or directory

The culprit? $QEMU_VXHS_PROG is empty for me, which means `set_prog_path
qnio_server` found nothing to use.  You'll have to add in a safety valve
that only calls 'type' if operating on a non-empty path in the first place.

-- 
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 --]

Re: [Qemu-devel] [PATCH v4 1/2] iotests: Use absolute paths for executables

[Eric Blake]

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

On 06/30/2017 02:41 PM, Eric Blake wrote:

> +++ 068.out.bad	2017-06-30 14:35:28.720241398 -0500
> @@ -1,4 +1,5 @@
>  QA output created by 068
> +realpath: '': No such file or directory
> 
> The culprit? $QEMU_VXHS_PROG is empty for me, which means `set_prog_path
> qnio_server` found nothing to use.  You'll have to add in a safety valve
> that only calls 'type' if operating on a non-empty path in the first place.

I'm using this locally, in the meantime:

diff --git i/tests/qemu-iotests/common.config
w/tests/qemu-iotests/common.config
index c1dc425..6f97331 100644
--- i/tests/qemu-iotests/common.config
+++ w/tests/qemu-iotests/common.config
@@ -107,7 +107,9 @@ export QEMU_PROG=$(realpath -- "$(type -p
"$QEMU_PROG")")
 export QEMU_IMG_PROG=$(realpath -- "$(type -p "$QEMU_IMG_PROG")")
 export QEMU_IO_PROG=$(realpath -- "$(type -p "$QEMU_IO_PROG")")
 export QEMU_NBD_PROG=$(realpath -- "$(type -p "$QEMU_NBD_PROG")")
-export QEMU_VXHS_PROG=$(realpath -- "$(type -p "$QEMU_VXHS_PROG")")
+if [ -n "$QEMU_VXHS_PROG" ]; then
+    export QEMU_VXHS_PROG=$(realpath -- "$(type -p "$QEMU_VXHS_PROG")")
+fi

 _qemu_wrapper()
 {


Is qnio_server easily available for Fedora?

-- 
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 --]

Re: [Qemu-devel] [Qemu-block] [PATCH v4 1/2] iotests: Use absolute paths for executables

[Jeff Cody]

On Fri, Jun 30, 2017 at 02:45:46PM -0500, Eric Blake wrote:
> On 06/30/2017 02:41 PM, Eric Blake wrote:
> 
> > +++ 068.out.bad	2017-06-30 14:35:28.720241398 -0500
> > @@ -1,4 +1,5 @@
> >  QA output created by 068
> > +realpath: '': No such file or directory
> > 
> > The culprit? $QEMU_VXHS_PROG is empty for me, which means `set_prog_path
> > qnio_server` found nothing to use.  You'll have to add in a safety valve
> > that only calls 'type' if operating on a non-empty path in the first place.
> 
> I'm using this locally, in the meantime:
> 
> diff --git i/tests/qemu-iotests/common.config
> w/tests/qemu-iotests/common.config
> index c1dc425..6f97331 100644
> --- i/tests/qemu-iotests/common.config
> +++ w/tests/qemu-iotests/common.config
> @@ -107,7 +107,9 @@ export QEMU_PROG=$(realpath -- "$(type -p
> "$QEMU_PROG")")
>  export QEMU_IMG_PROG=$(realpath -- "$(type -p "$QEMU_IMG_PROG")")
>  export QEMU_IO_PROG=$(realpath -- "$(type -p "$QEMU_IO_PROG")")
>  export QEMU_NBD_PROG=$(realpath -- "$(type -p "$QEMU_NBD_PROG")")
> -export QEMU_VXHS_PROG=$(realpath -- "$(type -p "$QEMU_VXHS_PROG")")
> +if [ -n "$QEMU_VXHS_PROG" ]; then
> +    export QEMU_VXHS_PROG=$(realpath -- "$(type -p "$QEMU_VXHS_PROG")")
> +fi
> 
>  _qemu_wrapper()
>  {
> 
> 
> Is qnio_server easily available for Fedora?
>

Depends on your definition of "easily".  It needs to be built from the
upstream project github [1].  Most developers will likely not have it
installed.

https://github.com/VeritasHyperScale/libqnio

Re: [Qemu-devel] [PATCH v4 1/2] iotests: Use absolute paths for executables

[Max Reitz]

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

On 2017-06-30 21:41, Eric Blake wrote:
> On 06/29/2017 09:46 PM, Max Reitz wrote:
> 
>>>> +++ b/tests/qemu-iotests/common.config
>>>> @@ -103,6 +103,12 @@ if [ -z "$QEMU_VXHS_PROG" ]; then
>>>>      export QEMU_VXHS_PROG="`set_prog_path qnio_server`"
>>>>  fi
>>>>  
>>>> +export QEMU_PROG=$(realpath -- "$(type -p "$QEMU_PROG")")
>>>
>>> ...now that you updated per my review to favor 'type' over 'which'?
>>> Otherwise, the R-b stands.
>>
>> Thanks, will fix and apply then...
>>
>> ...and done, applied to my block branch:
>>
>> https://github.com/XanClic/qemu/commits/block
> 
> Sorry for not noticing sooner, but you'll need to replace v4 (commit
> 0f0fec82 on your branch) with a fix, because now your branch does the
> following on all iotests for me:
> 
> 068 3s ... - output mismatch (see 068.out.bad)
> --- /home/eblake/qemu/tests/qemu-iotests/068.out	2017-06-26
> 22:02:56.057734882 -0500
> +++ 068.out.bad	2017-06-30 14:35:28.720241398 -0500
> @@ -1,4 +1,5 @@
>  QA output created by 068
> +realpath: '': No such file or directory

Oops.

> The culprit? $QEMU_VXHS_PROG is empty for me, which means `set_prog_path
> qnio_server` found nothing to use.  You'll have to add in a safety valve
> that only calls 'type' if operating on a non-empty path in the first place.

Sure, will drop the patches and send a v5.

Thanks!

Max


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

[Qemu-devel] [PATCH v4 2/2] iotests: Add test for colon handling

[Max Reitz]

Reviewed-by: Eric Blake <eblake@redhat.com>
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..a2d4d6c
--- /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 the use of 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 318ae74..1bdb6d1 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