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

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

[Max Reitz]

Technically, this is a v2 to my previous series
"block: JSON filenames and relative backing files", although only patch
2 and 5 were already part of that series, and also patch 2 has been
changed quite a bit in this version.

So, first the original cover letter:

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.

And now about why this v2 exists:

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).


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                    | 50 ++++++++++++++++++------
 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(+), 14 deletions(-)
 create mode 100755 tests/qemu-iotests/110
 create mode 100644 tests/qemu-iotests/110.out

-- 
1.9.3

[Qemu-devel] [PATCH v2 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>
---
 block.c               | 16 ++++++++++++----
 include/block/block.h |  3 +++
 2 files changed, 15 insertions(+), 4 deletions(-)

diff --git a/block.c b/block.c
index dacd881..2366f19 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 13e4537..b48791b 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -395,6 +395,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 v2 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 2366f19..d9440c0 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 1301144..77baa36 100644
--- a/block/qapi.c
+++ b/block/qapi.c
@@ -229,7 +229,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 b48791b..80bbe56 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -394,10 +394,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 v2 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 | 17 +++++++++++------
 1 file changed, 11 insertions(+), 6 deletions(-)

diff --git a/block.c b/block.c
index d9440c0..b8fade9 100644
--- a/block.c
+++ b/block.c
@@ -5620,22 +5620,27 @@ 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) {
-                error_setg_errno(errp, -ret, "Could not open '%s': %s",
-                                 backing_file,
-                                 error_get_pretty(local_err));
-                error_free(local_err);
-                local_err = NULL;
                 goto out;
             }
             size = bdrv_getlength(bs);
-- 
1.9.3

[Qemu-devel] [PATCH v2 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 673d3f5..c180906 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 v2 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 7b2c666..6b44692 100644
--- a/tests/qemu-iotests/group
+++ b/tests/qemu-iotests/group
@@ -111,3 +111,4 @@
 105 rw auto quick
 107 rw auto quick
 108 rw auto quick
+110 rw auto backing quick
-- 
1.9.3

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

[Max Reitz]

On 2014-11-04 at 15:19, Max Reitz wrote:
> Technically, this is a v2 to my previous series
> "block: JSON filenames and relative backing files", although only patch
> 2 and 5 were already part of that series, and also patch 2 has been
> changed quite a bit in this version.
>
> So, first the original cover letter:
>
> 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.
>
> And now about why this v2 exists:
>
> 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).
>
>
> 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                    | 50 ++++++++++++++++++------
>   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(+), 14 deletions(-)
>   create mode 100755 tests/qemu-iotests/110
>   create mode 100644 tests/qemu-iotests/110.out

Ping

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

[Kevin Wolf]

Am 04.11.2014 um 15:19 hat Max Reitz geschrieben:
> 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>

This series needs to be rebased, so I'm just adding:

Reviewed-by: Kevin Wolf <kwolf@redhat.com>