[Qemu-devel] [PATCH v5 0/2] qemu-img: Check for backing image if specified during create

[Qemu-devel] [PATCH v5 0/2] qemu-img: Check for backing image if specified during create

[John Snow]

We do not currently guarantee that QEMU will or will not open a
backing file when creating a new overlay file. Presently, QEMU will
not open that file if you provide a filesize, because it has no reason
to want to open it in that case.

This series makes the contract more explicit: if '-u' is provided to
create, we will not open the backing image regardless, erroring out if
a size was not provided.

In the other case, if '-u' is not provided, we now endeavor to open the
backing image if possible to check that it exists. For now, if a size
is provided and the image does not exist, QEMU will only warn to maintain
compatibility with legacy behavior.

In the future, QEMU may treat the operation as a failure if '-u' was not
provided.

Tests are amended primarily to pass the '-u' flag where it makes sense;
which is when creating overlays for objects already open by QEMU. These
will now generally fail to succeed because of image locking. In this
case, they only warn instead of fail, but this keeps the output cleaner.

Test 111 is updated to accommodate a new error message.
082, 085, 139, 156 and 158 add '-u' just to suppress warnings.

John Snow (2):
  blockdev: move BDRV_O_NO_BACKING option forward
  qemu-img: Check for backing image if specified during create

 block.c                    | 96 +++++++++++++++++++++++++---------------------
 blockdev.c                 | 11 +++---
 qemu-img-cmds.hx           |  4 +-
 qemu-img.c                 | 16 +++++---
 tests/qemu-iotests/082     |  4 +-
 tests/qemu-iotests/082.out |  4 +-
 tests/qemu-iotests/085     |  2 +-
 tests/qemu-iotests/111.out |  1 +
 tests/qemu-iotests/139     |  2 +-
 tests/qemu-iotests/156     |  2 +-
 tests/qemu-iotests/158     |  2 +-
 11 files changed, 81 insertions(+), 63 deletions(-)

-- 
2.9.4

[Qemu-devel] [PATCH v5 1/2] blockdev: move BDRV_O_NO_BACKING option forward

[John Snow]

For both external_snapshot_prepare and qmp_drive_mirror, we eventually
append the option BDRV_O_NO_BACKING. However, we generally do so after
we create the image.

To accommodate image creation wanting to verify that a backing file
exists or not, add this option prior to create to override checking
the existence of the backing file. This prevents QEMU from trying to
re-open a backing file that's already in use (thanks to qcow2 locking).

Signed-off-by: John Snow <jsnow@redhat.com>
---
 blockdev.c | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/blockdev.c b/blockdev.c
index 7f53cc8..6469f16 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -1710,7 +1710,8 @@ static void external_snapshot_prepare(BlkActionState *common,
         }
 
         flags = state->old_bs->open_flags;
-        flags &= ~(BDRV_O_SNAPSHOT | BDRV_O_NO_BACKING | BDRV_O_COPY_ON_READ);
+        flags &= ~(BDRV_O_SNAPSHOT | BDRV_O_COPY_ON_READ);
+        flags |= BDRV_O_NO_BACKING;
 
         /* create new image w/backing file */
         mode = s->has_mode ? s->mode : NEW_IMAGE_MODE_ABSOLUTE_PATHS;
@@ -1735,8 +1736,6 @@ static void external_snapshot_prepare(BlkActionState *common,
             qdict_put_str(options, "node-name", snapshot_node_name);
         }
         qdict_put_str(options, "driver", format);
-
-        flags |= BDRV_O_NO_BACKING;
     }
 
     state->new_bs = bdrv_open(new_image_file, snapshot_ref, options, flags,
@@ -3548,6 +3547,9 @@ void qmp_drive_mirror(DriveMirror *arg, Error **errp)
         backing_mode = MIRROR_OPEN_BACKING_CHAIN;
     }
 
+    /* Don't open backing image in create() */
+    flags |= BDRV_O_NO_BACKING;
+
     if ((arg->sync == MIRROR_SYNC_MODE_FULL || !source)
         && arg->mode != NEW_IMAGE_MODE_EXISTING)
     {
@@ -3587,8 +3589,7 @@ void qmp_drive_mirror(DriveMirror *arg, Error **errp)
     /* Mirroring takes care of copy-on-write using the source's backing
      * file.
      */
-    target_bs = bdrv_open(arg->target, NULL, options,
-                          flags | BDRV_O_NO_BACKING, errp);
+    target_bs = bdrv_open(arg->target, NULL, options, flags, errp);
     if (!target_bs) {
         goto out;
     }
-- 
2.9.4

[Qemu-devel] [PATCH v5 2/2] qemu-img: Check for backing image if specified during create

[John Snow]

Or, rather, force the open of a backing image if one was specified
for creation. Using a similar -unsafe option as rebase, allow qemu-img
to ignore the backing file validation if possible.

It may not always be possible, as in the existing case when a filesize
for the new image was not specified.

This is accomplished by shifting around the conditionals in
bdrv_img_create, such that a backing file is always opened unless we
provide BDRV_O_NO_BACKING. qemu-img is adjusted to pass this new flag
when -u is provided to create.

Sorry for the heinous looking diffstat, but it's mostly whitespace.

Inspired by: https://bugzilla.redhat.com/show_bug.cgi?id=1213786

Reviewed-by: Eric Blake <eblake@redhat.com>
Signed-off-by: John Snow <jsnow@redhat.com>
---
 block.c                    | 96 +++++++++++++++++++++++++---------------------
 qemu-img-cmds.hx           |  4 +-
 qemu-img.c                 | 16 +++++---
 tests/qemu-iotests/082     |  4 +-
 tests/qemu-iotests/082.out |  4 +-
 tests/qemu-iotests/085     |  2 +-
 tests/qemu-iotests/111.out |  1 +
 tests/qemu-iotests/139     |  2 +-
 tests/qemu-iotests/156     |  2 +-
 tests/qemu-iotests/158     |  2 +-
 10 files changed, 75 insertions(+), 58 deletions(-)

diff --git a/block.c b/block.c
index 98a9209..17d0d4a 100644
--- a/block.c
+++ b/block.c
@@ -4396,55 +4396,65 @@ void bdrv_img_create(const char *filename, const char *fmt,
 
     backing_fmt = qemu_opt_get(opts, BLOCK_OPT_BACKING_FMT);
 
-    // The size for the image must always be specified, with one exception:
-    // If we are using a backing file, we can obtain the size from there
+    /* The size for the image must always be specified, unless we have a backing
+     * file and we have not been forbidden from opening it */
     size = qemu_opt_get_size(opts, BLOCK_OPT_SIZE, 0);
-    if (size == -1) {
-        if (backing_file) {
-            BlockDriverState *bs;
-            char *full_backing = g_new0(char, PATH_MAX);
-            int64_t size;
-            int back_flags;
-            QDict *backing_options = NULL;
-
-            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;
-            back_flags &= ~(BDRV_O_RDWR | BDRV_O_SNAPSHOT | BDRV_O_NO_BACKING);
-
-            if (backing_fmt) {
-                backing_options = qdict_new();
-                qdict_put_str(backing_options, "driver", backing_fmt);
-            }
-
-            bs = bdrv_open(full_backing, NULL, backing_options, back_flags,
-                           &local_err);
+    if (backing_file && !(flags & BDRV_O_NO_BACKING)) {
+        BlockDriverState *bs;
+        char *full_backing = g_new0(char, PATH_MAX);
+        int back_flags;
+        QDict *backing_options = NULL;
+
+        bdrv_get_full_backing_filename_from_filename(filename, backing_file,
+                                                     full_backing, PATH_MAX,
+                                                     &local_err);
+        if (local_err) {
             g_free(full_backing);
-            if (!bs) {
-                goto out;
-            }
-            size = bdrv_getlength(bs);
-            if (size < 0) {
-                error_setg_errno(errp, -size, "Could not get size of '%s'",
-                                 backing_file);
-                bdrv_unref(bs);
-                goto out;
-            }
+            goto out;
+        }
 
-            qemu_opt_set_number(opts, BLOCK_OPT_SIZE, size, &error_abort);
+        /* backing files always opened read-only */
+        back_flags = flags;
+        back_flags &= ~(BDRV_O_RDWR | BDRV_O_SNAPSHOT | BDRV_O_NO_BACKING);
 
+        if (backing_fmt) {
+            backing_options = qdict_new();
+            qdict_put_str(backing_options, "driver", backing_fmt);
+        }
+
+        bs = bdrv_open(full_backing, NULL, backing_options, back_flags,
+                       &local_err);
+        g_free(full_backing);
+        if (!bs && size != -1) {
+            /* Couldn't open BS, but we have a size, so it's nonfatal */
+            error_reportf_err(local_err,
+                              "Warning: could not verify backing image. "
+                              "This may become an error in future versions.\n");
+            local_err = NULL;
+        } else if (!bs) {
+            /* Couldn't open bs, do not have size */
+            error_append_hint(&local_err,
+                              "Could not open backing image to determine size.\n");
+            goto out;
+        } else {
+            if (size == -1) {
+                /* Opened BS, have no size */
+                size = bdrv_getlength(bs);
+                if (size < 0) {
+                    error_setg_errno(errp, -size, "Could not get size of '%s'",
+                                     backing_file);
+                    bdrv_unref(bs);
+                    goto out;
+                }
+                qemu_opt_set_number(opts, BLOCK_OPT_SIZE, size, &error_abort);
+            }
             bdrv_unref(bs);
-        } else {
-            error_setg(errp, "Image creation needs a size parameter");
-            goto out;
         }
+    } /* (backing_file && !(flags & BDRV_O_NO_BACKING)) */
+
+    if (size == -1) {
+        error_setg(errp, "Image creation needs a size parameter");
+        goto out;
     }
 
     if (!quiet) {
diff --git a/qemu-img-cmds.hx b/qemu-img-cmds.hx
index ac5946b..3763f13 100644
--- a/qemu-img-cmds.hx
+++ b/qemu-img-cmds.hx
@@ -22,9 +22,9 @@ STEXI
 ETEXI
 
 DEF("create", img_create,
-    "create [-q] [--object objectdef] [-f fmt] [-b backing_file] [-F backing_fmt] [-o options] filename [size]")
+    "create [-q] [--object objectdef] [-f fmt] [-b backing_file] [-F backing_fmt] [-u] [-o options] filename [size]")
 STEXI
-@item create [--object @var{objectdef}] [-q] [-f @var{fmt}] [-b @var{backing_file}] [-F @var{backing_fmt}] [-o @var{options}] @var{filename} [@var{size}]
+@item create [--object @var{objectdef}] [-q] [-f @var{fmt}] [-b @var{backing_file}] [-F @var{backing_fmt}] [-u] [-o @var{options}] @var{filename} [@var{size}]
 ETEXI
 
 DEF("commit", img_commit,
diff --git a/qemu-img.c b/qemu-img.c
index 182e697..eb32b93 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -150,9 +150,11 @@ static void QEMU_NORETURN help(void)
            "  'snapshot_id_or_name' is deprecated, use 'snapshot_param'\n"
            "    instead\n"
            "  '-c' indicates that target image must be compressed (qcow format only)\n"
-           "  '-u' enables unsafe rebasing. It is assumed that old and new backing file\n"
-           "       match exactly. The image doesn't need a working backing file before\n"
-           "       rebasing in this case (useful for renaming the backing file)\n"
+           "  '-u' allows unsafe backing chains. For rebasing, it is assumed that old and\n"
+           "       new backing file match exactly. The image doesn't need a working\n"
+           "       backing file before rebasing in this case (useful for renaming the\n"
+           "       backing file). For image creation, allow creating without attempting\n"
+           "       to open the backing file.\n"
            "  '-h' with or without a command shows this help and lists the supported formats\n"
            "  '-p' show progress of command (only certain commands)\n"
            "  '-q' use Quiet mode - do not print any output (except errors)\n"
@@ -429,6 +431,7 @@ static int img_create(int argc, char **argv)
     char *options = NULL;
     Error *local_err = NULL;
     bool quiet = false;
+    int flags = 0;
 
     for(;;) {
         static const struct option long_options[] = {
@@ -436,7 +439,7 @@ static int img_create(int argc, char **argv)
             {"object", required_argument, 0, OPTION_OBJECT},
             {0, 0, 0, 0}
         };
-        c = getopt_long(argc, argv, ":F:b:f:ho:q",
+        c = getopt_long(argc, argv, ":F:b:f:ho:qu",
                         long_options, NULL);
         if (c == -1) {
             break;
@@ -476,6 +479,9 @@ static int img_create(int argc, char **argv)
         case 'q':
             quiet = true;
             break;
+        case 'u':
+            flags |= BDRV_O_NO_BACKING;
+            break;
         case OPTION_OBJECT: {
             QemuOpts *opts;
             opts = qemu_opts_parse_noisily(&qemu_object_opts,
@@ -528,7 +534,7 @@ static int img_create(int argc, char **argv)
     }
 
     bdrv_img_create(filename, fmt, base_filename, base_fmt,
-                    options, img_size, 0, quiet, &local_err);
+                    options, img_size, flags, quiet, &local_err);
     if (local_err) {
         error_reportf_err(local_err, "%s: ", filename);
         goto fail;
diff --git a/tests/qemu-iotests/082 b/tests/qemu-iotests/082
index ad1d9fa..d5c83d4 100755
--- a/tests/qemu-iotests/082
+++ b/tests/qemu-iotests/082
@@ -85,8 +85,8 @@ run_qemu_img create -f $IMGFMT -o cluster_size=4k -o help "$TEST_IMG" $size
 run_qemu_img create -f $IMGFMT -o cluster_size=4k -o \? "$TEST_IMG" $size
 
 # Looks like a help option, but is part of the backing file name
-run_qemu_img create -f $IMGFMT -o backing_file="$TEST_IMG",,help "$TEST_IMG" $size
-run_qemu_img create -f $IMGFMT -o backing_file="$TEST_IMG",,\? "$TEST_IMG" $size
+run_qemu_img create -f $IMGFMT -u -o backing_file="$TEST_IMG",,help "$TEST_IMG" $size
+run_qemu_img create -f $IMGFMT -u -o backing_file="$TEST_IMG",,\? "$TEST_IMG" $size
 
 # Try to trick qemu-img into creating escaped commas
 run_qemu_img create -f $IMGFMT -o backing_file="$TEST_IMG", -o help "$TEST_IMG" $size
diff --git a/tests/qemu-iotests/082.out b/tests/qemu-iotests/082.out
index dbed67f..1527fbe 100644
--- a/tests/qemu-iotests/082.out
+++ b/tests/qemu-iotests/082.out
@@ -210,10 +210,10 @@ lazy_refcounts   Postpone refcount updates
 refcount_bits    Width of a reference count entry in bits
 nocow            Turn off copy-on-write (valid only on btrfs)
 
-Testing: create -f qcow2 -o backing_file=TEST_DIR/t.qcow2,,help TEST_DIR/t.qcow2 128M
+Testing: create -f qcow2 -u -o backing_file=TEST_DIR/t.qcow2,,help TEST_DIR/t.qcow2 128M
 Formatting 'TEST_DIR/t.qcow2', fmt=qcow2 size=134217728 backing_file=TEST_DIR/t.qcow2,,help cluster_size=65536 lazy_refcounts=off refcount_bits=16
 
-Testing: create -f qcow2 -o backing_file=TEST_DIR/t.qcow2,,? TEST_DIR/t.qcow2 128M
+Testing: create -f qcow2 -u -o backing_file=TEST_DIR/t.qcow2,,? TEST_DIR/t.qcow2 128M
 Formatting 'TEST_DIR/t.qcow2', fmt=qcow2 size=134217728 backing_file=TEST_DIR/t.qcow2,,? cluster_size=65536 lazy_refcounts=off refcount_bits=16
 
 Testing: create -f qcow2 -o backing_file=TEST_DIR/t.qcow2, -o help TEST_DIR/t.qcow2 128M
diff --git a/tests/qemu-iotests/085 b/tests/qemu-iotests/085
index b97adcd..71efe50 100755
--- a/tests/qemu-iotests/085
+++ b/tests/qemu-iotests/085
@@ -104,7 +104,7 @@ function add_snapshot_image()
 {
     base_image="${TEST_DIR}/$((${1}-1))-${snapshot_virt0}"
     snapshot_file="${TEST_DIR}/${1}-${snapshot_virt0}"
-    _make_test_img -b "${base_image}" "$size"
+    _make_test_img -u -b "${base_image}" "$size"
     mv "${TEST_IMG}" "${snapshot_file}"
     do_blockdev_add "$1" "'backing': '', " "${snapshot_file}"
 }
diff --git a/tests/qemu-iotests/111.out b/tests/qemu-iotests/111.out
index 683c01a..5279c46 100644
--- a/tests/qemu-iotests/111.out
+++ b/tests/qemu-iotests/111.out
@@ -1,3 +1,4 @@
 QA output created by 111
 qemu-img: TEST_DIR/t.IMGFMT: Could not open 'TEST_DIR/t.IMGFMT.inexistent': No such file or directory
+Could not open backing image to determine size.
 *** done
diff --git a/tests/qemu-iotests/139 b/tests/qemu-iotests/139
index 175d8f0..9ff51d9 100644
--- a/tests/qemu-iotests/139
+++ b/tests/qemu-iotests/139
@@ -65,7 +65,7 @@ class TestBlockdevDel(iotests.QMPTestCase):
     # Add a BlockDriverState that will be used as overlay for the base_img BDS
     def addBlockDriverStateOverlay(self, node):
         self.checkBlockDriverState(node, False)
-        iotests.qemu_img('create', '-f', iotests.imgfmt,
+        iotests.qemu_img('create', '-u', '-f', iotests.imgfmt,
                          '-b', base_img, new_img, '1M')
         opts = {'driver': iotests.imgfmt,
                 'node-name': node,
diff --git a/tests/qemu-iotests/156 b/tests/qemu-iotests/156
index d799b73..2c4a06e 100755
--- a/tests/qemu-iotests/156
+++ b/tests/qemu-iotests/156
@@ -66,7 +66,7 @@ _send_qemu_cmd $QEMU_HANDLE \
     'return'
 
 # Create snapshot
-TEST_IMG="$TEST_IMG.overlay" _make_test_img -b "$TEST_IMG" 1M
+TEST_IMG="$TEST_IMG.overlay" _make_test_img -u -b "$TEST_IMG" 1M
 _send_qemu_cmd $QEMU_HANDLE \
     "{ 'execute': 'blockdev-snapshot-sync',
        'arguments': { 'device': 'source',
diff --git a/tests/qemu-iotests/158 b/tests/qemu-iotests/158
index 823c120..24ac600 100755
--- a/tests/qemu-iotests/158
+++ b/tests/qemu-iotests/158
@@ -66,7 +66,7 @@ echo "== verify pattern =="
 $QEMU_IO --object $SECRET -c "read -P 0xa 0 $size" --image-opts $IMGSPECBASE | _filter_qemu_io | _filter_testdir
 
 echo "== create overlay =="
-_make_test_img --object $SECRET -o "encryption=on,encrypt.key-secret=sec0" -b "$TEST_IMG_BASE" $size
+_make_test_img -u --object $SECRET -o "encryption=on,encrypt.key-secret=sec0" -b "$TEST_IMG_BASE" $size
 
 echo
 echo "== writing part of a cluster =="
-- 
2.9.4

Re: [Qemu-devel] [PATCH v5 0/2] qemu-img: Check for backing image if specified during create

[John Snow]

Kevin: I took a stab at this 'feature', but if there are any fixups or
changes that need to occur and it's important that it happens before I'm
awake, please feel free to steal these patches and do whatever you need
to them, including setting them on fire.

Thanks,
--John

post-script: I think the only thing that I don't really do here is
attempt to force-open an image to see if it exists if a size is already
provided in order to quiet errors related to locking.

That change would just eliminate a little bit of "this image is locked!"
whining in the case that -u was omitted but a size was provided, which
is mostly QOL.

On 07/17/2017 08:34 PM, John Snow wrote:
> We do not currently guarantee that QEMU will or will not open a
> backing file when creating a new overlay file. Presently, QEMU will
> not open that file if you provide a filesize, because it has no reason
> to want to open it in that case.
> 
> This series makes the contract more explicit: if '-u' is provided to
> create, we will not open the backing image regardless, erroring out if
> a size was not provided.
> 
> In the other case, if '-u' is not provided, we now endeavor to open the
> backing image if possible to check that it exists. For now, if a size
> is provided and the image does not exist, QEMU will only warn to maintain
> compatibility with legacy behavior.
> 
> In the future, QEMU may treat the operation as a failure if '-u' was not
> provided.
> 
> Tests are amended primarily to pass the '-u' flag where it makes sense;
> which is when creating overlays for objects already open by QEMU. These
> will now generally fail to succeed because of image locking. In this
> case, they only warn instead of fail, but this keeps the output cleaner.
> 
> Test 111 is updated to accommodate a new error message.
> 082, 085, 139, 156 and 158 add '-u' just to suppress warnings.
> 
> John Snow (2):
>   blockdev: move BDRV_O_NO_BACKING option forward
>   qemu-img: Check for backing image if specified during create
> 
>  block.c                    | 96 +++++++++++++++++++++++++---------------------
>  blockdev.c                 | 11 +++---
>  qemu-img-cmds.hx           |  4 +-
>  qemu-img.c                 | 16 +++++---
>  tests/qemu-iotests/082     |  4 +-
>  tests/qemu-iotests/082.out |  4 +-
>  tests/qemu-iotests/085     |  2 +-
>  tests/qemu-iotests/111.out |  1 +
>  tests/qemu-iotests/139     |  2 +-
>  tests/qemu-iotests/156     |  2 +-
>  tests/qemu-iotests/158     |  2 +-
>  11 files changed, 81 insertions(+), 63 deletions(-)
> 

Re: [Qemu-devel] [PATCH v5 0/2] qemu-img: Check for backing image if specified during create

[no-reply]

Hi,

This series seems to have some coding style problems. See output below for
more information:

Subject: [Qemu-devel] [PATCH v5 0/2] qemu-img: Check for backing image if specified during create
Message-id: 20170718003422.4497-1-jsnow@redhat.com
Type: series

=== TEST SCRIPT BEGIN ===
#!/bin/bash

BASE=base
n=1
total=$(git log --oneline $BASE.. | wc -l)
failed=0

git config --local diff.renamelimit 0
git config --local diff.renames True

commits="$(git log --format=%H --reverse $BASE..)"
for c in $commits; do
    echo "Checking PATCH $n/$total: $(git log -n 1 --format=%s $c)..."
    if ! git show $c --format=email | ./scripts/checkpatch.pl --mailback -; then
        failed=1
        echo
    fi
    n=$((n+1))
done

exit $failed
=== TEST SCRIPT END ===

Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
From https://github.com/patchew-project/qemu
 - [tag update]      patchew/20170717145556.14142-1-aurelien@aurel32.net -> patchew/20170717145556.14142-1-aurelien@aurel32.net
Switched to a new branch 'test'
7ebdc98 qemu-img: Check for backing image if specified during create
8bc4293 blockdev: move BDRV_O_NO_BACKING option forward

=== OUTPUT BEGIN ===
Checking PATCH 1/2: blockdev: move BDRV_O_NO_BACKING option forward...
Checking PATCH 2/2: qemu-img: Check for backing image if specified during create...
ERROR: Error messages should not contain newlines
#103: FILE: block.c:4432:
+                              "This may become an error in future versions.\n");

total: 1 errors, 0 warnings, 222 lines checked

Your patch has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.

=== OUTPUT END ===

Test command exited with code: 1


---
Email generated automatically by Patchew [http://patchew.org/].
Please send your feedback to patchew-devel@freelists.org

Re: [Qemu-devel] [PATCH v5 1/2] blockdev: move BDRV_O_NO_BACKING option forward

[Eric Blake]

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

On 07/17/2017 07:34 PM, John Snow wrote:
> For both external_snapshot_prepare and qmp_drive_mirror, we eventually
> append the option BDRV_O_NO_BACKING. However, we generally do so after
> we create the image.
> 
> To accommodate image creation wanting to verify that a backing file
> exists or not, add this option prior to create to override checking
> the existence of the backing file. This prevents QEMU from trying to
> re-open a backing file that's already in use (thanks to qcow2 locking).
> 
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
>  blockdev.c | 11 ++++++-----
>  1 file changed, 6 insertions(+), 5 deletions(-)

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: 619 bytes --]

Re: [Qemu-devel] [PATCH v5 2/2] qemu-img: Check for backing image if specified during create

[Eric Blake]

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

On 07/17/2017 07:34 PM, John Snow wrote:
> Or, rather, force the open of a backing image if one was specified
> for creation. Using a similar -unsafe option as rebase, allow qemu-img
> to ignore the backing file validation if possible.
> 
> It may not always be possible, as in the existing case when a filesize
> for the new image was not specified.
> 
> This is accomplished by shifting around the conditionals in
> bdrv_img_create, such that a backing file is always opened unless we
> provide BDRV_O_NO_BACKING. qemu-img is adjusted to pass this new flag
> when -u is provided to create.
> 
> Sorry for the heinous looking diffstat, but it's mostly whitespace.

This sentence is not quite as applicable as it was on earlier versions,
as you now have added logic for determining which level (warning vs.
error) to print.

> 
> Inspired by: https://bugzilla.redhat.com/show_bug.cgi?id=1213786
> 
> Reviewed-by: Eric Blake <eblake@redhat.com>

Really? It seems like you've changed since v4.

> Signed-off-by: John Snow <jsnow@redhat.com>
> ---

> +++ b/block.c
> @@ -4396,55 +4396,65 @@ void bdrv_img_create(const char *filename, const char *fmt,
>  
>      backing_fmt = qemu_opt_get(opts, BLOCK_OPT_BACKING_FMT);
>  
> -    // The size for the image must always be specified, with one exception:
> -    // If we are using a backing file, we can obtain the size from there
> +    /* The size for the image must always be specified, unless we have a backing
> +     * file and we have not been forbidden from opening it */

Worth adding a trailing '.' to the sentence?

>      size = qemu_opt_get_size(opts, BLOCK_OPT_SIZE, 0);

On v4, we talked about making this use qemu_opt_get_size(, -1) to make
it less confusing about how qemu_opt_get_size() refers back to a
caller-provided default embedded in QemuOpt (rather than the parameter).

> +        if (!bs && size != -1) {
> +            /* Couldn't open BS, but we have a size, so it's nonfatal */
> +            error_reportf_err(local_err,
> +                              "Warning: could not verify backing image. "
> +                              "This may become an error in future versions.\n");

Patchew rightly complained here about the trailing newline. Also, we
have the new warning* functions merged in, this should probably be using
those (see commit 3dc6f869, for example)

> +            local_err = NULL;
> +        } else if (!bs) {
> +            /* Couldn't open bs, do not have size */
> +            error_append_hint(&local_err,
> +                              "Could not open backing image to determine size.\n");
> +            goto out;
> +        } else {
> +            if (size == -1) {
> +                /* Opened BS, have no size */
> +                size = bdrv_getlength(bs);
> +                if (size < 0) {
> +                    error_setg_errno(errp, -size, "Could not get size of '%s'",
> +                                     backing_file);

These look correct.

> +++ b/tests/qemu-iotests/111.out
> @@ -1,3 +1,4 @@
>  QA output created by 111
>  qemu-img: TEST_DIR/t.IMGFMT: Could not open 'TEST_DIR/t.IMGFMT.inexistent': No such file or directory
> +Could not open backing image to determine size.

Nice demonstration of the hint at work.

The rest of the patch looks fine to me.  We'll need a couple of tweaks,
but between you, Kevin, and myself, I think we can coordinate getting
something ready for a pull request today.

-- 
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: 619 bytes --]

Re: [Qemu-devel] [PATCH v5 0/2] qemu-img: Check for backing image if specified during create

[Kevin Wolf]

Am 18.07.2017 um 02:34 hat John Snow geschrieben:
> We do not currently guarantee that QEMU will or will not open a
> backing file when creating a new overlay file. Presently, QEMU will
> not open that file if you provide a filesize, because it has no reason
> to want to open it in that case.
> 
> This series makes the contract more explicit: if '-u' is provided to
> create, we will not open the backing image regardless, erroring out if
> a size was not provided.
> 
> In the other case, if '-u' is not provided, we now endeavor to open the
> backing image if possible to check that it exists. For now, if a size
> is provided and the image does not exist, QEMU will only warn to maintain
> compatibility with legacy behavior.
> 
> In the future, QEMU may treat the operation as a failure if '-u' was not
> provided.
> 
> Tests are amended primarily to pass the '-u' flag where it makes sense;
> which is when creating overlays for objects already open by QEMU. These
> will now generally fail to succeed because of image locking. In this
> case, they only warn instead of fail, but this keeps the output cleaner.
> 
> Test 111 is updated to accommodate a new error message.
> 082, 085, 139, 156 and 158 add '-u' just to suppress warnings.

Thanks, applied to the block branch.

Kevin

Re: [Qemu-devel] [PATCH v5 2/2] qemu-img: Check for backing image if specified during create

[John Snow]

On 07/18/2017 08:51 AM, Eric Blake wrote:
> On 07/17/2017 07:34 PM, John Snow wrote:
>> Or, rather, force the open of a backing image if one was specified
>> for creation. Using a similar -unsafe option as rebase, allow qemu-img
>> to ignore the backing file validation if possible.
>>
>> It may not always be possible, as in the existing case when a filesize
>> for the new image was not specified.
>>
>> This is accomplished by shifting around the conditionals in
>> bdrv_img_create, such that a backing file is always opened unless we
>> provide BDRV_O_NO_BACKING. qemu-img is adjusted to pass this new flag
>> when -u is provided to create.
>>
>> Sorry for the heinous looking diffstat, but it's mostly whitespace.
> 
> This sentence is not quite as applicable as it was on earlier versions,
> as you now have added logic for determining which level (warning vs.
> error) to print.
> 
>>
>> Inspired by: https://bugzilla.redhat.com/show_bug.cgi?id=1213786
>>
>> Reviewed-by: Eric Blake <eblake@redhat.com>
> 
> Really? It seems like you've changed since v4.
> 

Duh. I missed this because the patchset grew to two patches, same with
revising the message. I'm sorry about that.

>> Signed-off-by: John Snow <jsnow@redhat.com>
>> ---
> 
>> +++ b/block.c
>> @@ -4396,55 +4396,65 @@ void bdrv_img_create(const char *filename, const char *fmt,
>>  
>>      backing_fmt = qemu_opt_get(opts, BLOCK_OPT_BACKING_FMT);
>>  
>> -    // The size for the image must always be specified, with one exception:
>> -    // If we are using a backing file, we can obtain the size from there
>> +    /* The size for the image must always be specified, unless we have a backing
>> +     * file and we have not been forbidden from opening it */
> 
> Worth adding a trailing '.' to the sentence?
> 

Sure, why not?

>>      size = qemu_opt_get_size(opts, BLOCK_OPT_SIZE, 0);
> 
> On v4, we talked about making this use qemu_opt_get_size(, -1) to make
> it less confusing about how qemu_opt_get_size() refers back to a
> caller-provided default embedded in QemuOpt (rather than the parameter).>

I actually got scared away from this because of the get_size signature,
is it safe to pass -1 here?

>> +        if (!bs && size != -1) {
>> +            /* Couldn't open BS, but we have a size, so it's nonfatal */
>> +            error_reportf_err(local_err,
>> +                              "Warning: could not verify backing image. "
>> +                              "This may become an error in future versions.\n");
> 
> Patchew rightly complained here about the trailing newline. Also, we
> have the new warning* functions merged in, this should probably be using
> those (see commit 3dc6f869, for example)
> 

I tried omitting it, but the printing looked wrong, and the test would
mash input against the tail of the sentence.

>> +            local_err = NULL;
>> +        } else if (!bs) {
>> +            /* Couldn't open bs, do not have size */
>> +            error_append_hint(&local_err,
>> +                              "Could not open backing image to determine size.\n");
>> +            goto out;
>> +        } else {
>> +            if (size == -1) {
>> +                /* Opened BS, have no size */
>> +                size = bdrv_getlength(bs);
>> +                if (size < 0) {
>> +                    error_setg_errno(errp, -size, "Could not get size of '%s'",
>> +                                     backing_file);
> 
> These look correct.
> 
>> +++ b/tests/qemu-iotests/111.out
>> @@ -1,3 +1,4 @@
>>  QA output created by 111
>>  qemu-img: TEST_DIR/t.IMGFMT: Could not open 'TEST_DIR/t.IMGFMT.inexistent': No such file or directory
>> +Could not open backing image to determine size.
> 
> Nice demonstration of the hint at work.
> 
> The rest of the patch looks fine to me.  We'll need a couple of tweaks,
> but between you, Kevin, and myself, I think we can coordinate getting
> something ready for a pull request today.
> 

Re: [Qemu-devel] [PATCH v5 2/2] qemu-img: Check for backing image if specified during create

[Eric Blake]

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

On 07/18/2017 10:33 AM, John Snow wrote:
> 
> 
> On 07/18/2017 08:51 AM, Eric Blake wrote:
>> On 07/17/2017 07:34 PM, John Snow wrote:
>>> Or, rather, force the open of a backing image if one was specified
>>> for creation. Using a similar -unsafe option as rebase, allow qemu-img
>>> to ignore the backing file validation if possible.

>>> Reviewed-by: Eric Blake <eblake@redhat.com>
>>
>> Really? It seems like you've changed since v4.
>>
> 
> Duh. I missed this because the patchset grew to two patches, same with
> revising the message. I'm sorry about that.

Kevin's got it on his block branch, with that fixed already. No problem.

>>>      size = qemu_opt_get_size(opts, BLOCK_OPT_SIZE, 0);
>>
>> On v4, we talked about making this use qemu_opt_get_size(, -1) to make
>> it less confusing about how qemu_opt_get_size() refers back to a
>> caller-provided default embedded in QemuOpt (rather than the parameter).>
> 
> I actually got scared away from this because of the get_size signature,
> is it safe to pass -1 here?

I'm posting a separate patch for that now (yours is fine left alone,
because it is pre-existing).

> 
>>> +        if (!bs && size != -1) {
>>> +            /* Couldn't open BS, but we have a size, so it's nonfatal */
>>> +            error_reportf_err(local_err,
>>> +                              "Warning: could not verify backing image. "
>>> +                              "This may become an error in future versions.\n");
>>
>> Patchew rightly complained here about the trailing newline. Also, we
>> have the new warning* functions merged in, this should probably be using
>> those (see commit 3dc6f869, for example)
>>
> 
> I tried omitting it, but the printing looked wrong, and the test would
> mash input against the tail of the sentence.

Kevin adjusted it slightly on the block branch. If patchew still
complains, we may need to fix checkpatch.pl (the semantics of
error_reportf_err() are slightly different than error_setg(), after all).

-- 
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: 619 bytes --]