[Qemu-devel] [PATCH v3 0/5] block: Relative backing files

[Qemu-devel] [PATCH v3 0/5] block: Relative backing files

[Max Reitz]

Cover letter of v1:

Sometimes, qemu does not have a filename to work with (it then generates
a JSON filename), so it does not know which directory to use for a
backing file specified by a relative filename.

In this case, qemu should not somehow try to append the backing file's
name to the JSON object, but rather just print an error and bail out.


Part of the cover letter specific to v2:

Stefan already applied v1 before realizing that the test added in the
series was actually broken for vmdk (which I somehow missed myself).
This was due to vmdk trying to open the backing file on creation in
order to determine its format; however, normally the backing file path
is interpreted relatively to the backed image's base directory, whereas
in this case vmdk directly used the user-specified filename which was
therefore interpreted relatively to qemu's working directory.

Patch 4 of this v2 fixes this. A similar issue exists directly in
bdrv_img_create() which opens the backing file in order to determine its
size in case the size of the new image has not been specified. This is
fixed by patch 3.

The function both patches use is factored out from
bdrv_get_full_backing_filename() (which we cannot use here because it
requires a BDS which does not necessarily exist during image creation)
in patch 1. Patch 2 was then modified so it modifies this new function
(bdrv_get_full_backing_filename_from_filename()) and the old
bdrv_get_full_backing_filename(), which is now more or less just a
wrapper.

And finally, I added a test to patch 5 which tests creation of a backed
image in another directory only using relative paths while omitting the
image size (which is therefore inferred from the backing file).


v3:
- Solved merge conflict in the iotests group file [Kevin]
  (On a note unrelated to this series: I'm not sure patches should be
  stopped due to merge conflicts in that file. That would mean only one
  patch per week creating a new iotest, and I don't think that limit is
  necessary. Conflicts in that file can be easily resolved (it's even
  clear how to resolve issues when the test number is already taken, but
  that's a different story).)
  This series now applies to Kevin's and mine block-next, block and to
  master.


Max Reitz (5):
  block: Get full backing filename from string
  block: JSON filenames and relative backing files
  block: Relative backing file for image creation
  block/vmdk: Relative backing file for creation
  iotests: Add test for relative backing file names

 block.c                    | 45 +++++++++++++++++++---
 block/qapi.c               |  7 +++-
 block/vmdk.c               | 13 ++++++-
 include/block/block.h      |  6 ++-
 tests/qemu-iotests/110     | 94 ++++++++++++++++++++++++++++++++++++++++++++++
 tests/qemu-iotests/110.out | 19 ++++++++++
 tests/qemu-iotests/group   |  1 +
 7 files changed, 176 insertions(+), 9 deletions(-)
 create mode 100755 tests/qemu-iotests/110
 create mode 100644 tests/qemu-iotests/110.out

-- 
1.9.3

[Qemu-devel] [PATCH v3 1/5] block: Get full backing filename from string

[Max Reitz]

Introduce bdrv_get_full_backing_filename_from_filename(), a function
which takes the name of the backed file and a potentially relative
backing filename to produce the full (absolute) backing filename.

Use this function from bdrv_get_full_backing_filename().

Signed-off-by: Max Reitz <mreitz@redhat.com>
Reviewed-by: Kevin Wolf <kwolf@redhat.com>
---
 block.c               | 16 ++++++++++++----
 include/block/block.h |  3 +++
 2 files changed, 15 insertions(+), 4 deletions(-)

diff --git a/block.c b/block.c
index 866c8b4..0c1be37 100644
--- a/block.c
+++ b/block.c
@@ -303,15 +303,23 @@ void path_combine(char *dest, int dest_size,
     }
 }
 
-void bdrv_get_full_backing_filename(BlockDriverState *bs, char *dest, size_t sz)
+void bdrv_get_full_backing_filename_from_filename(const char *backed,
+                                                  const char *backing,
+                                                  char *dest, size_t sz)
 {
-    if (bs->backing_file[0] == '\0' || path_has_protocol(bs->backing_file)) {
-        pstrcpy(dest, sz, bs->backing_file);
+    if (backing[0] == '\0' || path_has_protocol(backing)) {
+        pstrcpy(dest, sz, backing);
     } else {
-        path_combine(dest, sz, bs->filename, bs->backing_file);
+        path_combine(dest, sz, backed, backing);
     }
 }
 
+void bdrv_get_full_backing_filename(BlockDriverState *bs, char *dest, size_t sz)
+{
+    bdrv_get_full_backing_filename_from_filename(bs->filename, bs->backing_file,
+                                                 dest, sz);
+}
+
 void bdrv_register(BlockDriver *bdrv)
 {
     /* Block drivers without coroutine functions need emulation */
diff --git a/include/block/block.h b/include/block/block.h
index 610be9f..1c7ecaa 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -399,6 +399,9 @@ void bdrv_get_backing_filename(BlockDriverState *bs,
                                char *filename, int filename_size);
 void bdrv_get_full_backing_filename(BlockDriverState *bs,
                                     char *dest, size_t sz);
+void bdrv_get_full_backing_filename_from_filename(const char *backed,
+                                                  const char *backing,
+                                                  char *dest, size_t sz);
 int bdrv_is_snapshot(BlockDriverState *bs);
 
 int path_is_absolute(const char *path);
-- 
1.9.3

[Qemu-devel] [PATCH v3 2/5] block: JSON filenames and relative backing files

[Max Reitz]

When using a relative backing file name, qemu needs to know the
directory of the top image file. For JSON filenames, such a directory
cannot be easily determined (e.g. how do you determine the directory of
a qcow2 BDS directly on top of a quorum BDS?). Therefore, do not allow
relative filenames for the backing file of BDSs only having a JSON
filename.

Furthermore, BDS::exact_filename should be used whenever possible. If
BDS::filename is not equal to BDS::exact_filename, the former will
always be a JSON object.

Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 block.c               | 27 +++++++++++++++++++++------
 block/qapi.c          |  7 ++++++-
 include/block/block.h |  5 +++--
 3 files changed, 30 insertions(+), 9 deletions(-)

diff --git a/block.c b/block.c
index 0c1be37..a0cddcd 100644
--- a/block.c
+++ b/block.c
@@ -305,19 +305,28 @@ void path_combine(char *dest, int dest_size,
 
 void bdrv_get_full_backing_filename_from_filename(const char *backed,
                                                   const char *backing,
-                                                  char *dest, size_t sz)
+                                                  char *dest, size_t sz,
+                                                  Error **errp)
 {
-    if (backing[0] == '\0' || path_has_protocol(backing)) {
+    if (backing[0] == '\0' || path_has_protocol(backing) ||
+        path_is_absolute(backing))
+    {
         pstrcpy(dest, sz, backing);
+    } else if (backed[0] == '\0' || strstart(backed, "json:", NULL)) {
+        error_setg(errp, "Cannot use relative backing file names for '%s'",
+                   backed);
     } else {
         path_combine(dest, sz, backed, backing);
     }
 }
 
-void bdrv_get_full_backing_filename(BlockDriverState *bs, char *dest, size_t sz)
+void bdrv_get_full_backing_filename(BlockDriverState *bs, char *dest, size_t sz,
+                                    Error **errp)
 {
-    bdrv_get_full_backing_filename_from_filename(bs->filename, bs->backing_file,
-                                                 dest, sz);
+    char *backed = bs->exact_filename[0] ? bs->exact_filename : bs->filename;
+
+    bdrv_get_full_backing_filename_from_filename(backed, bs->backing_file,
+                                                 dest, sz, errp);
 }
 
 void bdrv_register(BlockDriver *bdrv)
@@ -1209,7 +1218,13 @@ int bdrv_open_backing_file(BlockDriverState *bs, QDict *options, Error **errp)
         QDECREF(options);
         goto free_exit;
     } else {
-        bdrv_get_full_backing_filename(bs, backing_filename, PATH_MAX);
+        bdrv_get_full_backing_filename(bs, backing_filename, PATH_MAX, &local_err);
+        if (local_err) {
+            ret = -EINVAL;
+            error_propagate(errp, local_err);
+            QDECREF(options);
+            goto free_exit;
+        }
     }
 
     if (!bs->drv || !bs->drv->supports_backing) {
diff --git a/block/qapi.c b/block/qapi.c
index fa68ba7..a6fd6f7 100644
--- a/block/qapi.c
+++ b/block/qapi.c
@@ -214,7 +214,12 @@ void bdrv_query_image_info(BlockDriverState *bs,
         info->backing_filename = g_strdup(backing_filename);
         info->has_backing_filename = true;
         bdrv_get_full_backing_filename(bs, backing_filename2,
-                                       sizeof(backing_filename2));
+                                       sizeof(backing_filename2), &err);
+        if (err) {
+            error_propagate(errp, err);
+            qapi_free_ImageInfo(info);
+            return;
+        }
 
         if (strcmp(backing_filename, backing_filename2) != 0) {
             info->full_backing_filename =
diff --git a/include/block/block.h b/include/block/block.h
index 1c7ecaa..f34f63c 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -398,10 +398,11 @@ const char *bdrv_get_encrypted_filename(BlockDriverState *bs);
 void bdrv_get_backing_filename(BlockDriverState *bs,
                                char *filename, int filename_size);
 void bdrv_get_full_backing_filename(BlockDriverState *bs,
-                                    char *dest, size_t sz);
+                                    char *dest, size_t sz, Error **errp);
 void bdrv_get_full_backing_filename_from_filename(const char *backed,
                                                   const char *backing,
-                                                  char *dest, size_t sz);
+                                                  char *dest, size_t sz,
+                                                  Error **errp);
 int bdrv_is_snapshot(BlockDriverState *bs);
 
 int path_is_absolute(const char *path);
-- 
1.9.3

[Qemu-devel] [PATCH v3 3/5] block: Relative backing file for image creation

[Max Reitz]

Relative backing filenames are always relative to the backed image's
directory; the same applies to image creation. Therefore, if the backing
file has to be opened for determining its size (in case the size has not
been explicitly specified) its filename should be interpreted relative
to the new image's base directory and not relative to qemu's working
directory.

Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 block.c | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

diff --git a/block.c b/block.c
index a0cddcd..ed8e187 100644
--- a/block.c
+++ b/block.c
@@ -5633,16 +5633,26 @@ void bdrv_img_create(const char *filename, const char *fmt,
     if (size == -1) {
         if (backing_file) {
             BlockDriverState *bs;
+            char *full_backing = g_new0(char, PATH_MAX);
             int64_t size;
             int back_flags;
 
+            bdrv_get_full_backing_filename_from_filename(filename, backing_file,
+                                                         full_backing, PATH_MAX,
+                                                         &local_err);
+            if (local_err) {
+                g_free(full_backing);
+                goto out;
+            }
+
             /* backing files always opened read-only */
             back_flags =
                 flags & ~(BDRV_O_RDWR | BDRV_O_SNAPSHOT | BDRV_O_NO_BACKING);
 
             bs = NULL;
-            ret = bdrv_open(&bs, backing_file, NULL, NULL, back_flags,
+            ret = bdrv_open(&bs, full_backing, NULL, NULL, back_flags,
                             backing_drv, &local_err);
+            g_free(full_backing);
             if (ret < 0) {
                 goto out;
             }
-- 
1.9.3

[Qemu-devel] [PATCH v3 4/5] block/vmdk: Relative backing file for creation

[Max Reitz]

When a vmdk image is created with a backing file, it is opened to check
whether it is indeed a vmdk file by letting qemu probe it. When doing
so, the backing filename is relative to the image's base directory so it
should be interpreted accordingly.

Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 block/vmdk.c | 13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)

diff --git a/block/vmdk.c b/block/vmdk.c
index 2cbfd3e..aea8f07 100644
--- a/block/vmdk.c
+++ b/block/vmdk.c
@@ -1868,8 +1868,19 @@ static int vmdk_create(const char *filename, QemuOpts *opts, Error **errp)
     }
     if (backing_file) {
         BlockDriverState *bs = NULL;
-        ret = bdrv_open(&bs, backing_file, NULL, NULL, BDRV_O_NO_BACKING, NULL,
+        char *full_backing = g_new0(char, PATH_MAX);
+        bdrv_get_full_backing_filename_from_filename(filename, backing_file,
+                                                     full_backing, PATH_MAX,
+                                                     &local_err);
+        if (local_err) {
+            g_free(full_backing);
+            error_propagate(errp, local_err);
+            ret = -ENOENT;
+            goto exit;
+        }
+        ret = bdrv_open(&bs, full_backing, NULL, NULL, BDRV_O_NO_BACKING, NULL,
                         errp);
+        g_free(full_backing);
         if (ret != 0) {
             goto exit;
         }
-- 
1.9.3

[Qemu-devel] [PATCH v3 5/5] iotests: Add test for relative backing file names

[Max Reitz]

Sometimes, qemu does not have a filename to work with, so it does not
know which directory to use for a backing file specified by a relative
filename. Add a test which tests that qemu exits with an appropriate
error message.

Additionally, add a test for qemu-img create with a backing filename
relative to the backed image's base directory while omitting the image
size.

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

diff --git a/tests/qemu-iotests/110 b/tests/qemu-iotests/110
new file mode 100755
index 0000000..63664f5
--- /dev/null
+++ b/tests/qemu-iotests/110
@@ -0,0 +1,94 @@
+#!/bin/bash
+#
+# Test case for relative backing file names in complex BDS trees
+#
+# Copyright (C) 2014 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"
+tmp=/tmp/$$
+status=1	# failure is the default!
+
+_cleanup()
+{
+	_cleanup_test_img
+}
+trap "_cleanup; exit \$status" 0 1 2 3 15
+
+# get standard environment, filters and checks
+. ./common.rc
+. ./common.filter
+
+# Any format supporting backing files
+_supported_fmt qed qcow qcow2 vmdk
+_supported_proto file
+_supported_os Linux
+_unsupported_imgopts "subformat=monolithicFlat" "subformat=twoGbMaxExtentFlat"
+
+TEST_IMG_REL=$(basename "$TEST_IMG")
+
+echo
+echo '=== Reconstructable filename ==='
+echo
+
+TEST_IMG="$TEST_IMG.base" _make_test_img 64M
+_make_test_img -b "$TEST_IMG_REL.base" 64M
+# qemu should be able to reconstruct the filename, so relative backing names
+# should work
+TEST_IMG="json:{'driver':'$IMGFMT','file':{'driver':'file','filename':'$TEST_IMG'}}" \
+    _img_info | _filter_img_info
+
+echo
+echo '=== Non-reconstructable filename ==='
+echo
+
+# Across blkdebug without a config file, you cannot reconstruct filenames, so
+# qemu is incapable of knowing the directory of the top image
+TEST_IMG="json:{
+    'driver': '$IMGFMT',
+    'file': {
+        'driver': 'blkdebug',
+        'image': {
+            'driver': 'file',
+            'filename': '$TEST_IMG'
+        },
+        'set-state': [
+            {
+                'event': 'read_aio',
+                'new_state': 42
+            }
+        ]
+    }
+}" _img_info | _filter_img_info
+
+echo
+echo '=== Backing name is always relative to the backed image ==='
+echo
+
+# omit the image size; it shoud work anyway
+_make_test_img -b "$TEST_IMG_REL.base"
+
+
+# success, all done
+echo '*** done'
+rm -f $seq.full
+status=0
diff --git a/tests/qemu-iotests/110.out b/tests/qemu-iotests/110.out
new file mode 100644
index 0000000..056ffec
--- /dev/null
+++ b/tests/qemu-iotests/110.out
@@ -0,0 +1,19 @@
+QA output created by 110
+
+=== Reconstructable filename ===
+
+Formatting 'TEST_DIR/t.IMGFMT.base', fmt=IMGFMT size=67108864
+Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=67108864 backing_file='t.IMGFMT.base'
+image: TEST_DIR/t.IMGFMT
+file format: IMGFMT
+virtual size: 64M (67108864 bytes)
+backing file: t.IMGFMT.base (actual path: TEST_DIR/t.IMGFMT.base)
+
+=== Non-reconstructable filename ===
+
+qemu-img: Cannot use relative backing file names for 'json:{"driver": "IMGFMT", "file": {"set-state": [{"new_state": 42, "state": 0, "event": "read_aio"}], "image": {"driver": "file", "filename": "TEST_DIR/t.IMGFMT"}, "driver": "blkdebug"}}'
+
+=== Backing name is always relative to the backed image ===
+
+Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=67108864 backing_file='t.IMGFMT.base'
+*** done
diff --git a/tests/qemu-iotests/group b/tests/qemu-iotests/group
index 7dfe469..de3c643 100644
--- a/tests/qemu-iotests/group
+++ b/tests/qemu-iotests/group
@@ -111,4 +111,5 @@
 105 rw auto quick
 107 rw auto quick
 108 rw auto quick
+110 rw auto backing quick
 111 rw auto quick
-- 
1.9.3

Re: [Qemu-devel] [PATCH v3 1/5] block: Get full backing filename from string

[Eric Blake]

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

On 11/24/2014 02:43 AM, Max Reitz wrote:
> Introduce bdrv_get_full_backing_filename_from_filename(), a function
> which takes the name of the backed file and a potentially relative
> backing filename to produce the full (absolute) backing filename.
> 
> Use this function from bdrv_get_full_backing_filename().
> 
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> Reviewed-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  block.c               | 16 ++++++++++++----
>  include/block/block.h |  3 +++
>  2 files changed, 15 insertions(+), 4 deletions(-)
> 

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

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


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

Re: [Qemu-devel] [PATCH v3 2/5] block: JSON filenames and relative backing files

[Eric Blake]

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

On 11/24/2014 02:43 AM, Max Reitz wrote:
> When using a relative backing file name, qemu needs to know the
> directory of the top image file. For JSON filenames, such a directory
> cannot be easily determined (e.g. how do you determine the directory of
> a qcow2 BDS directly on top of a quorum BDS?). Therefore, do not allow
> relative filenames for the backing file of BDSs only having a JSON
> filename.
> 
> Furthermore, BDS::exact_filename should be used whenever possible. If
> BDS::filename is not equal to BDS::exact_filename, the former will
> always be a JSON object.
> 
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
>  block.c               | 27 +++++++++++++++++++++------
>  block/qapi.c          |  7 ++++++-
>  include/block/block.h |  5 +++--
>  3 files changed, 30 insertions(+), 9 deletions(-)
> 
> diff --git a/block.c b/block.c
> index 0c1be37..a0cddcd 100644
> --- a/block.c
> +++ b/block.c
> @@ -305,19 +305,28 @@ void path_combine(char *dest, int dest_size,
>  
>  void bdrv_get_full_backing_filename_from_filename(const char *backed,
>                                                    const char *backing,
> -                                                  char *dest, size_t sz)
> +                                                  char *dest, size_t sz,
> +                                                  Error **errp)
>  {
> -    if (backing[0] == '\0' || path_has_protocol(backing)) {
> +    if (backing[0] == '\0' || path_has_protocol(backing) ||
> +        path_is_absolute(backing))
> +    {

checkpatch.pl didn't complain about this?  The { should be on the
previous line.  With that fixed,
Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


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

Re: [Qemu-devel] [PATCH v3 3/5] block: Relative backing file for image creation

[Eric Blake]

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

On 11/24/2014 02:43 AM, Max Reitz wrote:
> Relative backing filenames are always relative to the backed image's
> directory; the same applies to image creation. Therefore, if the backing
> file has to be opened for determining its size (in case the size has not
> been explicitly specified) its filename should be interpreted relative
> to the new image's base directory and not relative to qemu's working
> directory.
> 
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
>  block.c | 12 +++++++++++-
>  1 file changed, 11 insertions(+), 1 deletion(-)
> 
> diff --git a/block.c b/block.c
> index a0cddcd..ed8e187 100644
> --- a/block.c
> +++ b/block.c
> @@ -5633,16 +5633,26 @@ void bdrv_img_create(const char *filename, const char *fmt,
>      if (size == -1) {
>          if (backing_file) {
>              BlockDriverState *bs;
> +            char *full_backing = g_new0(char, PATH_MAX);

PATH_MAX is undefined on GNU Hurd.  But qemu doesn't compile on GNU
Hurd.  Furthermore, Linux allows (relative) paths in deep hierarchies
such that the absolute path is longer than PATH_MAX.  On the other hand,
such usage is rare (in fact, as coreutils learned, if you aren't using
openat() and friends reliably for O(n) descent into such deep
hierarchies, you then suffer from O(n^2) effects that make such paths
painful to create and use in the first place).  Besides, there are
already existing usage sites in qemu capped at PATH_MAX.

Therefore, in spite of all of my rambling, your patch is correct as-is
(I'm NOT asking you to rewrite to something safer that can tolerate
arbitrary filename lengths).

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

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


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

Re: [Qemu-devel] [PATCH v3 4/5] block/vmdk: Relative backing file for creation

[Eric Blake]

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

On 11/24/2014 02:43 AM, Max Reitz wrote:
> When a vmdk image is created with a backing file, it is opened to check
> whether it is indeed a vmdk file by letting qemu probe it. When doing
> so, the backing filename is relative to the image's base directory so it
> should be interpreted accordingly.
> 
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
>  block/vmdk.c | 13 ++++++++++++-
>  1 file changed, 12 insertions(+), 1 deletion(-)
> 

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

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


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

Re: [Qemu-devel] [PATCH v3 5/5] iotests: Add test for relative backing file names

[Eric Blake]

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

On 11/24/2014 02:43 AM, Max Reitz wrote:
> Sometimes, qemu does not have a filename to work with, so it does not
> know which directory to use for a backing file specified by a relative
> filename. Add a test which tests that qemu exits with an appropriate
> error message.
> 
> Additionally, add a test for qemu-img create with a backing filename
> relative to the backed image's base directory while omitting the image
> size.
> 
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
>  tests/qemu-iotests/110     | 94 ++++++++++++++++++++++++++++++++++++++++++++++
>  tests/qemu-iotests/110.out | 19 ++++++++++
>  tests/qemu-iotests/group   |  1 +
>  3 files changed, 114 insertions(+)
>  create mode 100755 tests/qemu-iotests/110
>  create mode 100644 tests/qemu-iotests/110.out
> 

> +echo
> +echo '=== Backing name is always relative to the backed image ==='
> +echo
> +
> +# omit the image size; it shoud work anyway

s/shoud/should/

With the typo fix,
Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


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

Re: [Qemu-devel] [PATCH v3 2/5] block: JSON filenames and relative backing files

[Fam Zheng]

On Mon, 11/24 10:43, Max Reitz wrote:
> @@ -1209,7 +1218,13 @@ int bdrv_open_backing_file(BlockDriverState *bs, QDict *options, Error **errp)
>          QDECREF(options);
>          goto free_exit;
>      } else {
> -        bdrv_get_full_backing_filename(bs, backing_filename, PATH_MAX);
> +        bdrv_get_full_backing_filename(bs, backing_filename, PATH_MAX, &local_err);

Over 80 charaters?

Fam

Re: [Qemu-devel] [PATCH v3 4/5] block/vmdk: Relative backing file for creation

[Fam Zheng]

On Mon, 11/24 10:43, Max Reitz wrote:
> When a vmdk image is created with a backing file, it is opened to check
> whether it is indeed a vmdk file by letting qemu probe it. When doing
> so, the backing filename is relative to the image's base directory so it
> should be interpreted accordingly.
> 
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
>  block/vmdk.c | 13 ++++++++++++-
>  1 file changed, 12 insertions(+), 1 deletion(-)
> 
> diff --git a/block/vmdk.c b/block/vmdk.c
> index 2cbfd3e..aea8f07 100644
> --- a/block/vmdk.c
> +++ b/block/vmdk.c
> @@ -1868,8 +1868,19 @@ static int vmdk_create(const char *filename, QemuOpts *opts, Error **errp)
>      }
>      if (backing_file) {
>          BlockDriverState *bs = NULL;
> -        ret = bdrv_open(&bs, backing_file, NULL, NULL, BDRV_O_NO_BACKING, NULL,
> +        char *full_backing = g_new0(char, PATH_MAX);
> +        bdrv_get_full_backing_filename_from_filename(filename, backing_file,
> +                                                     full_backing, PATH_MAX,
> +                                                     &local_err);
> +        if (local_err) {
> +            g_free(full_backing);
> +            error_propagate(errp, local_err);
> +            ret = -ENOENT;
> +            goto exit;
> +        }
> +        ret = bdrv_open(&bs, full_backing, NULL, NULL, BDRV_O_NO_BACKING, NULL,
>                          errp);
> +        g_free(full_backing);
>          if (ret != 0) {
>              goto exit;
>          }
> -- 
> 1.9.3
> 
> 

Reviewed-by: Fam Zheng <famz@redhat.com>

Re: [Qemu-devel] [PATCH v3 1/5] block: Get full backing filename from string

[Fam Zheng]

On Mon, 11/24 10:43, Max Reitz wrote:
> Introduce bdrv_get_full_backing_filename_from_filename(), a function
> which takes the name of the backed file and a potentially relative
> backing filename to produce the full (absolute) backing filename.
> 
> Use this function from bdrv_get_full_backing_filename().
> 
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> Reviewed-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  block.c               | 16 ++++++++++++----
>  include/block/block.h |  3 +++
>  2 files changed, 15 insertions(+), 4 deletions(-)
> 
> diff --git a/block.c b/block.c
> index 866c8b4..0c1be37 100644
> --- a/block.c
> +++ b/block.c
> @@ -303,15 +303,23 @@ void path_combine(char *dest, int dest_size,
>      }
>  }
>  
> -void bdrv_get_full_backing_filename(BlockDriverState *bs, char *dest, size_t sz)
> +void bdrv_get_full_backing_filename_from_filename(const char *backed,
> +                                                  const char *backing,
> +                                                  char *dest, size_t sz)
>  {
> -    if (bs->backing_file[0] == '\0' || path_has_protocol(bs->backing_file)) {
> -        pstrcpy(dest, sz, bs->backing_file);
> +    if (backing[0] == '\0' || path_has_protocol(backing)) {
> +        pstrcpy(dest, sz, backing);
>      } else {
> -        path_combine(dest, sz, bs->filename, bs->backing_file);
> +        path_combine(dest, sz, backed, backing);
>      }
>  }
>  
> +void bdrv_get_full_backing_filename(BlockDriverState *bs, char *dest, size_t sz)
> +{
> +    bdrv_get_full_backing_filename_from_filename(bs->filename, bs->backing_file,
> +                                                 dest, sz);
> +}
> +
>  void bdrv_register(BlockDriver *bdrv)
>  {
>      /* Block drivers without coroutine functions need emulation */
> diff --git a/include/block/block.h b/include/block/block.h
> index 610be9f..1c7ecaa 100644
> --- a/include/block/block.h
> +++ b/include/block/block.h
> @@ -399,6 +399,9 @@ void bdrv_get_backing_filename(BlockDriverState *bs,
>                                 char *filename, int filename_size);
>  void bdrv_get_full_backing_filename(BlockDriverState *bs,
>                                      char *dest, size_t sz);
> +void bdrv_get_full_backing_filename_from_filename(const char *backed,
> +                                                  const char *backing,
> +                                                  char *dest, size_t sz);
>  int bdrv_is_snapshot(BlockDriverState *bs);
>  
>  int path_is_absolute(const char *path);
> -- 
> 1.9.3
> 
> 

Reviewed-by: Fam Zheng <famz@redhat.com>

Re: [Qemu-devel] [PATCH v3 3/5] block: Relative backing file for image creation

[Fam Zheng]

On Mon, 11/24 10:43, Max Reitz wrote:
> Relative backing filenames are always relative to the backed image's
> directory; the same applies to image creation. Therefore, if the backing
> file has to be opened for determining its size (in case the size has not
> been explicitly specified) its filename should be interpreted relative
> to the new image's base directory and not relative to qemu's working
> directory.
> 
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
>  block.c | 12 +++++++++++-
>  1 file changed, 11 insertions(+), 1 deletion(-)
> 
> diff --git a/block.c b/block.c
> index a0cddcd..ed8e187 100644
> --- a/block.c
> +++ b/block.c
> @@ -5633,16 +5633,26 @@ void bdrv_img_create(const char *filename, const char *fmt,
>      if (size == -1) {
>          if (backing_file) {
>              BlockDriverState *bs;
> +            char *full_backing = g_new0(char, PATH_MAX);
>              int64_t size;
>              int back_flags;
>  
> +            bdrv_get_full_backing_filename_from_filename(filename, backing_file,
> +                                                         full_backing, PATH_MAX,
> +                                                         &local_err);
> +            if (local_err) {
> +                g_free(full_backing);
> +                goto out;
> +            }
> +
>              /* backing files always opened read-only */
>              back_flags =
>                  flags & ~(BDRV_O_RDWR | BDRV_O_SNAPSHOT | BDRV_O_NO_BACKING);
>  
>              bs = NULL;
> -            ret = bdrv_open(&bs, backing_file, NULL, NULL, back_flags,
> +            ret = bdrv_open(&bs, full_backing, NULL, NULL, back_flags,
>                              backing_drv, &local_err);
> +            g_free(full_backing);
>              if (ret < 0) {
>                  goto out;
>              }
> -- 
> 1.9.3
> 
> 

Reviewed-by: Fam Zheng <famz@redhat.com>

Re: [Qemu-devel] [PATCH v3 5/5] iotests: Add test for relative backing file names

[Fam Zheng]

On Mon, 11/24 10:43, Max Reitz wrote:
> Sometimes, qemu does not have a filename to work with, so it does not
> know which directory to use for a backing file specified by a relative
> filename. Add a test which tests that qemu exits with an appropriate
> error message.
> 
> Additionally, add a test for qemu-img create with a backing filename
> relative to the backed image's base directory while omitting the image
> size.
> 
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
>  tests/qemu-iotests/110     | 94 ++++++++++++++++++++++++++++++++++++++++++++++
>  tests/qemu-iotests/110.out | 19 ++++++++++
>  tests/qemu-iotests/group   |  1 +
>  3 files changed, 114 insertions(+)
>  create mode 100755 tests/qemu-iotests/110
>  create mode 100644 tests/qemu-iotests/110.out
> 
> diff --git a/tests/qemu-iotests/110 b/tests/qemu-iotests/110
> new file mode 100755
> index 0000000..63664f5
> --- /dev/null
> +++ b/tests/qemu-iotests/110
> @@ -0,0 +1,94 @@
> +#!/bin/bash
> +#
> +# Test case for relative backing file names in complex BDS trees
> +#
> +# Copyright (C) 2014 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"
> +tmp=/tmp/$$
> +status=1	# failure is the default!
> +
> +_cleanup()
> +{
> +	_cleanup_test_img
> +}
> +trap "_cleanup; exit \$status" 0 1 2 3 15
> +
> +# get standard environment, filters and checks
> +. ./common.rc
> +. ./common.filter
> +
> +# Any format supporting backing files
> +_supported_fmt qed qcow qcow2 vmdk
> +_supported_proto file
> +_supported_os Linux
> +_unsupported_imgopts "subformat=monolithicFlat" "subformat=twoGbMaxExtentFlat"
> +
> +TEST_IMG_REL=$(basename "$TEST_IMG")
> +
> +echo
> +echo '=== Reconstructable filename ==='
> +echo
> +
> +TEST_IMG="$TEST_IMG.base" _make_test_img 64M
> +_make_test_img -b "$TEST_IMG_REL.base" 64M
> +# qemu should be able to reconstruct the filename, so relative backing names
> +# should work
> +TEST_IMG="json:{'driver':'$IMGFMT','file':{'driver':'file','filename':'$TEST_IMG'}}" \
> +    _img_info | _filter_img_info
> +
> +echo
> +echo '=== Non-reconstructable filename ==='
> +echo
> +
> +# Across blkdebug without a config file, you cannot reconstruct filenames, so
> +# qemu is incapable of knowing the directory of the top image
> +TEST_IMG="json:{
> +    'driver': '$IMGFMT',
> +    'file': {
> +        'driver': 'blkdebug',
> +        'image': {
> +            'driver': 'file',
> +            'filename': '$TEST_IMG'
> +        },
> +        'set-state': [
> +            {
> +                'event': 'read_aio',
> +                'new_state': 42
> +            }
> +        ]
> +    }
> +}" _img_info | _filter_img_info
> +
> +echo
> +echo '=== Backing name is always relative to the backed image ==='
> +echo
> +
> +# omit the image size; it shoud work anyway
> +_make_test_img -b "$TEST_IMG_REL.base"
> +
> +
> +# success, all done
> +echo '*** done'
> +rm -f $seq.full
> +status=0
> diff --git a/tests/qemu-iotests/110.out b/tests/qemu-iotests/110.out
> new file mode 100644
> index 0000000..056ffec
> --- /dev/null
> +++ b/tests/qemu-iotests/110.out
> @@ -0,0 +1,19 @@
> +QA output created by 110
> +
> +=== Reconstructable filename ===
> +
> +Formatting 'TEST_DIR/t.IMGFMT.base', fmt=IMGFMT size=67108864
> +Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=67108864 backing_file='t.IMGFMT.base'
> +image: TEST_DIR/t.IMGFMT
> +file format: IMGFMT
> +virtual size: 64M (67108864 bytes)
> +backing file: t.IMGFMT.base (actual path: TEST_DIR/t.IMGFMT.base)
> +
> +=== Non-reconstructable filename ===
> +
> +qemu-img: Cannot use relative backing file names for 'json:{"driver": "IMGFMT", "file": {"set-state": [{"new_state": 42, "state": 0, "event": "read_aio"}], "image": {"driver": "file", "filename": "TEST_DIR/t.IMGFMT"}, "driver": "blkdebug"}}'
> +
> +=== Backing name is always relative to the backed image ===
> +
> +Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=67108864 backing_file='t.IMGFMT.base'
> +*** done
> diff --git a/tests/qemu-iotests/group b/tests/qemu-iotests/group
> index 7dfe469..de3c643 100644
> --- a/tests/qemu-iotests/group
> +++ b/tests/qemu-iotests/group
> @@ -111,4 +111,5 @@
>  105 rw auto quick
>  107 rw auto quick
>  108 rw auto quick
> +110 rw auto backing quick
>  111 rw auto quick
> -- 
> 1.9.3
> 
> 

Reviewed-by: Fam Zheng <famz@redhat.com>

Re: [Qemu-devel] [PATCH v3 2/5] block: JSON filenames and relative backing files

[Max Reitz]

On 2014-11-25 at 20:57, Eric Blake wrote:
> On 11/24/2014 02:43 AM, Max Reitz wrote:
>> When using a relative backing file name, qemu needs to know the
>> directory of the top image file. For JSON filenames, such a directory
>> cannot be easily determined (e.g. how do you determine the directory of
>> a qcow2 BDS directly on top of a quorum BDS?). Therefore, do not allow
>> relative filenames for the backing file of BDSs only having a JSON
>> filename.
>>
>> Furthermore, BDS::exact_filename should be used whenever possible. If
>> BDS::filename is not equal to BDS::exact_filename, the former will
>> always be a JSON object.
>>
>> Signed-off-by: Max Reitz <mreitz@redhat.com>
>> ---
>>   block.c               | 27 +++++++++++++++++++++------
>>   block/qapi.c          |  7 ++++++-
>>   include/block/block.h |  5 +++--
>>   3 files changed, 30 insertions(+), 9 deletions(-)
>>
>> diff --git a/block.c b/block.c
>> index 0c1be37..a0cddcd 100644
>> --- a/block.c
>> +++ b/block.c
>> @@ -305,19 +305,28 @@ void path_combine(char *dest, int dest_size,
>>   
>>   void bdrv_get_full_backing_filename_from_filename(const char *backed,
>>                                                     const char *backing,
>> -                                                  char *dest, size_t sz)
>> +                                                  char *dest, size_t sz,
>> +                                                  Error **errp)
>>   {
>> -    if (backing[0] == '\0' || path_has_protocol(backing)) {
>> +    if (backing[0] == '\0' || path_has_protocol(backing) ||
>> +        path_is_absolute(backing))
>> +    {
> checkpatch.pl didn't complain about this?  The { should be on the
> previous line.  With that fixed,
> Reviewed-by: Eric Blake <eblake@redhat.com>

Oh, actually it does.

But there's no rule about that in CODING_STYLE. There is a rule about { 
being on the same line as the if, though ("The opening brace is on the 
line that contains the control
flow statement that introduces the new block"). With the condition split 
over multiple lines, that is no longer possible; therefore, we can place 
{ anywhere we want.

I always place { on the following line for multi-line conditions because 
I find that more readable[1]; maybe I'll change my mind in a year or 
two, but for now there is no rule about this case and I always do it 
this way; so far, nobody complained. :-)

[1] I don't like the following:

if (foo0() || foo1() || foo2() || foo3() ||
     bar0() || bar1() || bar2() || bar3()) {
     baz();

Because in my eyes it looks like baz() may be part of the condition list 
at first glance (because it is indented just as much as the last line of 
the condition list).

Max

Re: [Qemu-devel] [PATCH v3 2/5] block: JSON filenames and relative backing files

[Max Reitz]

On 2014-11-26 at 06:35, Fam Zheng wrote:
> On Mon, 11/24 10:43, Max Reitz wrote:
>> @@ -1209,7 +1218,13 @@ int bdrv_open_backing_file(BlockDriverState *bs, QDict *options, Error **errp)
>>           QDECREF(options);
>>           goto free_exit;
>>       } else {
>> -        bdrv_get_full_backing_filename(bs, backing_filename, PATH_MAX);
>> +        bdrv_get_full_backing_filename(bs, backing_filename, PATH_MAX, &local_err);
> Over 80 charaters?

Oops, will fix, thanks.

Max

Re: [Qemu-devel] [PATCH v3 5/5] iotests: Add test for relative backing file names

[Max Reitz]

On 2014-11-25 at 23:06, Eric Blake wrote:
> On 11/24/2014 02:43 AM, Max Reitz wrote:
>> Sometimes, qemu does not have a filename to work with, so it does not
>> know which directory to use for a backing file specified by a relative
>> filename. Add a test which tests that qemu exits with an appropriate
>> error message.
>>
>> Additionally, add a test for qemu-img create with a backing filename
>> relative to the backed image's base directory while omitting the image
>> size.
>>
>> Signed-off-by: Max Reitz <mreitz@redhat.com>
>> ---
>>   tests/qemu-iotests/110     | 94 ++++++++++++++++++++++++++++++++++++++++++++++
>>   tests/qemu-iotests/110.out | 19 ++++++++++
>>   tests/qemu-iotests/group   |  1 +
>>   3 files changed, 114 insertions(+)
>>   create mode 100755 tests/qemu-iotests/110
>>   create mode 100644 tests/qemu-iotests/110.out
>>
>> +echo
>> +echo '=== Backing name is always relative to the backed image ==='
>> +echo
>> +
>> +# omit the image size; it shoud work anyway
> s/shoud/should/

As I did the same mistake twice in some other series, I'm beginning to 
want to blame my keyboard... (I know that certain key sequences work 
bad, for instance, the U in NULL is sometimes omitted (not in null, only 
in NULL))

Max

> With the typo fix,
> Reviewed-by: Eric Blake <eblake@redhat.com>