Creating an image that requires format probing of the backing image is
inherently unsafe (we've had several CVEs over the years based on
probes leaking information to the guest on a subsequent boot, although
these days tools like libvirt are aware of the issue enough to prevent
the worst effects).  However, if our probing algorithm ever changes,
or if other tools like libvirt determine a different probe result than
we do, then subsequent use of that backing file under a different
format will present corrupted data to the guest.  Start a deprecation
clock so that future qemu-img can refuse to create unsafe backing
chains that would rely on probing.  The warnings are intentionally
emitted from the block layer rather than qemu-img (thus, all paths
into image creation or rewriting perform the check).

However, there is one time where probing is safe: if we probe raw,
then it is safe to record that implicitly in the image (but we still
warn, as it's better to teach the user to supply -F always than to
make them guess when it is safe).

iotest 114 specifically wants to create an unsafe image for later
amendment rather than defaulting to our new default of recording a
probed format, so it needs an update.  While touching it, expand it to
cover all of the various warnings enabled by this patch.

Signed-off-by: Eric Blake <eblake@redhat.com>
---
 docs/system/deprecated.rst | 19 +++++++++++++++++++
 block.c                    | 21 ++++++++++++++++++++-
 qemu-img.c                 |  2 +-
 tests/qemu-iotests/114     | 11 +++++++++++
 tests/qemu-iotests/114.out |  8 ++++++++
 5 files changed, 59 insertions(+), 2 deletions(-)

diff --git a/docs/system/deprecated.rst b/docs/system/deprecated.rst
index 6c1d9034d9e3..a8ffacf54a52 100644
--- a/docs/system/deprecated.rst
+++ b/docs/system/deprecated.rst
@@ -376,6 +376,25 @@ The above, converted to the current supported format::
 Related binaries
 ----------------

+qemu-img backing file without format (since 5.0.0)
+''''''''''''''''''''''''''''''''''''''''''''''''''
+
+The use of ``qemu-img create``, ``qemu-img rebase``, ``qemu-img
+convert``, or ``qemu-img amend`` to create or modify an image that
+depends on a backing file now recommends that an explicit backing
+format be provided.  This is for safety: if qemu probes a different
+format than what you thought, the data presented to the guest will be
+corrupt; similarly, presenting a raw image to a guest allows a
+potential security exploit if a future probe sees a non-raw image
+based on guest writes.  To avoid the warning message, or even future
+refusal to create an unsafe image, you must pass ``-o backing_fmt=``
+(or the shorthand ``-F`` during create) to specify the intended
+backing format.  You may use ``qemu-img rebase -u`` to retroactively
+add a backing format to an existing image.  However, be aware that
+there are already potential security risks to blindly using ``qemu-img
+info`` to probe the format of an untrusted backing image, when
+deciding what format to add into an existing image.
+
 ``qemu-img convert -n -o`` (since 4.2.0)
 ''''''''''''''''''''''''''''''''''''''''

diff --git a/block.c b/block.c
index 43452976acdc..ad49d515809c 100644
--- a/block.c
+++ b/block.c
@@ -6039,6 +6039,20 @@ void bdrv_img_create(const char *filename, const char *fmt,
                               "Could not open backing image to determine size.\n");
             goto out;
         } else {
+            if (!backing_fmt) {
+                warn_report("Deprecated use of backing file without explicit "
+                            "backing format (detected format of %s)",
+                            bs->drv->format_name);
+                if (bs->drv == &bdrv_raw) {
+                    /*
+                     * A probe of raw is always correct, so in this one
+                     * case, we can write that into the image.
+                     */
+                    backing_fmt = bs->drv->format_name;
+                    qemu_opt_set(opts, BLOCK_OPT_BACKING_FMT, backing_fmt,
+                                 NULL);
+                }
+            }
             if (size == -1) {
                 /* Opened BS, have no size */
                 size = bdrv_getlength(bs);
@@ -6052,7 +6066,12 @@ void bdrv_img_create(const char *filename, const char *fmt,
             }
             bdrv_unref(bs);
         }
-    } /* (backing_file && !(flags & BDRV_O_NO_BACKING)) */
+        /* (backing_file && !(flags & BDRV_O_NO_BACKING)) */
+    } else if (backing_file && !backing_fmt) {
+        warn_report("Deprecated use of unopened backing file without "
+                    "explicit backing format, use of this image requires "
+                    "potentially unsafe format probing");
+    }

     if (size == -1) {
         error_setg(errp, "Image creation needs a size parameter");
diff --git a/qemu-img.c b/qemu-img.c
index b9375427404d..48424f8dbcd4 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -3637,7 +3637,7 @@ static int img_rebase(int argc, char **argv)
      * doesn't change when we switch the backing file.
      */
     if (out_baseimg && *out_baseimg) {
-        ret = bdrv_change_backing_file(bs, out_baseimg, out_basefmt, false);
+        ret = bdrv_change_backing_file(bs, out_baseimg, out_basefmt, true);
     } else {
         ret = bdrv_change_backing_file(bs, NULL, NULL, false);
     }
diff --git a/tests/qemu-iotests/114 b/tests/qemu-iotests/114
index 26104fff6c67..5b06eab0ceee 100755
--- a/tests/qemu-iotests/114
+++ b/tests/qemu-iotests/114
@@ -42,9 +42,15 @@ _unsupported_proto vxhs
 # qcow2.py does not work too well with external data files
 _unsupported_imgopts data_file

+# Intentionally specify backing file without backing format; demonstrate
+# the difference in warning messages when backing file could be probed.
+# Note that only a raw probe result will affect the resulting image.
+truncate --size=64M "$TEST_IMG.orig"
+_make_test_img -b "$TEST_IMG.orig" 64M

 TEST_IMG="$TEST_IMG.base" _make_test_img 64M
 _make_test_img -b "$TEST_IMG.base" 64M
+_make_test_img -u -b "$TEST_IMG.base" 64M

 # Set an invalid backing file format
 $PYTHON qcow2.py "$TEST_IMG" add-header-ext 0xE2792ACA "foo"
@@ -55,6 +61,11 @@ _img_info
 $QEMU_IO -c "open $TEST_IMG" -c "read 0 4k" 2>&1 | _filter_qemu_io | _filter_testdir
 $QEMU_IO -c "open -o backing.driver=$IMGFMT $TEST_IMG" -c "read 0 4k" | _filter_qemu_io

+# Rebase the image, to show that omitting backing format triggers a warning,
+# but probing now lets us use the backing file.
+$QEMU_IMG rebase -u -b "$TEST_IMG.base" "$TEST_IMG"
+$QEMU_IO -c "open $TEST_IMG" -c "read 0 4k" 2>&1 | _filter_qemu_io | _filter_testdir
+
 # success, all done
 echo '*** done'
 rm -f $seq.full
diff --git a/tests/qemu-iotests/114.out b/tests/qemu-iotests/114.out
index 67adef37a4f6..59673abcd5e3 100644
--- a/tests/qemu-iotests/114.out
+++ b/tests/qemu-iotests/114.out
@@ -1,5 +1,10 @@
 QA output created by 114
+qemu-img: warning: Deprecated use of backing file without explicit backing format (detected format of raw)
+Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=67108864 backing_file=TEST_DIR/t.IMGFMT.orig backing_fmt=raw
 Formatting 'TEST_DIR/t.IMGFMT.base', fmt=IMGFMT size=67108864
+qemu-img: warning: Deprecated use of backing file without explicit backing format (detected format of IMGFMT)
+Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=67108864 backing_file=TEST_DIR/t.IMGFMT.base
+qemu-img: warning: Deprecated use of unopened backing file without explicit backing format, use of this image requires potentially unsafe format probing
 Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=67108864 backing_file=TEST_DIR/t.IMGFMT.base
 image: TEST_DIR/t.IMGFMT
 file format: IMGFMT
@@ -11,4 +16,7 @@ qemu-io: can't open device TEST_DIR/t.qcow2: Could not open backing file: Unknow
 no file open, try 'help open'
 read 4096/4096 bytes at offset 0
 4 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+qemu-img: warning: Deprecated use of backing file without explicit backing format, use of this image requires potentially unsafe format probing
+read 4096/4096 bytes at offset 0
+4 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
 *** done
-- 
2.25.1

On Fri, Mar 06, 2020 at 04:51:21PM -0600, Eric Blake wrote:
> Creating an image that requires format probing of the backing image is
> inherently unsafe (we've had several CVEs over the years based on
> probes leaking information to the guest on a subsequent boot, although
> these days tools like libvirt are aware of the issue enough to prevent
> the worst effects).  However, if our probing algorithm ever changes,
> or if other tools like libvirt determine a different probe result than
> we do, then subsequent use of that backing file under a different
> format will present corrupted data to the guest.  Start a deprecation
> clock so that future qemu-img can refuse to create unsafe backing
> chains that would rely on probing.  The warnings are intentionally
> emitted from the block layer rather than qemu-img (thus, all paths
> into image creation or rewriting perform the check).

I happily welcome this change.  I'm going around and fixing various docs
of differnt projects that create overlays without explicitly spelling
out backing files.  (FWIW, I also make sure to mention this, in context,
in all QEMU-related talks I give publicliy.)

This proactive action from QEMU will definitely help.

> However, there is one time where probing is safe: if we probe raw,
> then it is safe to record that implicitly in the image (but we still
> warn, as it's better to teach the user to supply -F always than to
> make them guess when it is safe).
> 
> iotest 114 specifically wants to create an unsafe image for later
> amendment rather than defaulting to our new default of recording a
> probed format, so it needs an update.  While touching it, expand it to
> cover all of the various warnings enabled by this patch.
> 
> Signed-off-by: Eric Blake <eblake@redhat.com>
> ---
>  docs/system/deprecated.rst | 19 +++++++++++++++++++
>  block.c                    | 21 ++++++++++++++++++++-
>  qemu-img.c                 |  2 +-
>  tests/qemu-iotests/114     | 11 +++++++++++
>  tests/qemu-iotests/114.out |  8 ++++++++
>  5 files changed, 59 insertions(+), 2 deletions(-)

Before (with: qemu-4.2.0-2.fc30):

    $> qemu-img create -f qcow2 -b ./base.raw ./overlay1.qcow2
    Formatting './overlay1.qcow2', fmt=qcow2 size=4294967296 backing_file=./base.raw cluster_size=65536 lazy_refcounts=off refcount_bits=16

After (with the patch series applied to QEMU Git):

    $> git describe
    v4.2.0-2204-gd6c7830114

    # Create; *without* specifying "-F raw"
    $> ~/build/qemu/qemu-img create -f qcow2 -b ./base.raw ./overlay2.qcow2
    qemu-img: warning: Deprecated use of backing file without explicit backing format (detected format of raw)
    Formatting './overlay2.qcow2', fmt=qcow2 size=4294967296 backing_file=./base.raw backing_fmt=raw cluster_size=65536 lazy_refcounts=off refcount_bits=16

    # Rebase; *without* specifying "-F raw"
    $> ~/build/qemu/qemu-img rebase -b base.raw overlay1.qcow2
    qemu-img: warning: Deprecated use of backing file without explicit backing format, use of this image requires potentially unsafe format probing


However, for the "Convert" case, is it correct that no warning is thrown
for the below?

    $> ~/build/qemu/qemu-img info overlay1.qcow2 
    image: overlay1.qcow2
    file format: qcow2
    virtual size: 4 GiB (4294967296 bytes)
    disk size: 196 KiB
    cluster_size: 65536
    backing file: base.raw
    Format specific information:
        compat: 1.1
        lazy refcounts: false
        refcount bits: 16
        corrupt: false
    
    
    $> ~/build/qemu/qemu-img convert -f qcow2 -O qcow2 overlay1.qcow2 flattened.raw

    $> echo $?
    0

> diff --git a/docs/system/deprecated.rst b/docs/system/deprecated.rst
> index 6c1d9034d9e3..a8ffacf54a52 100644
> --- a/docs/system/deprecated.rst
> +++ b/docs/system/deprecated.rst
> @@ -376,6 +376,25 @@ The above, converted to the current supported format::
>  Related binaries
>  ----------------
> 
> +qemu-img backing file without format (since 5.0.0)
> +''''''''''''''''''''''''''''''''''''''''''''''''''
> +
> +The use of ``qemu-img create``, ``qemu-img rebase``, ``qemu-img
> +convert``, or ``qemu-img amend`` to create or modify an image that
> +depends on a backing file now recommends that an explicit backing
> +format be provided.  This is for safety: if qemu probes a different
> +format than what you thought, the data presented to the guest will be
> +corrupt; similarly, presenting a raw image to a guest allows a
> +potential security exploit if a future probe sees a non-raw image
> +based on guest writes.  To avoid the warning message, or even future
> +refusal to create an unsafe image, you must pass ``-o backing_fmt=``
> +(or the shorthand ``-F`` during create) to specify the intended
> +backing format.  You may use ``qemu-img rebase -u`` to retroactively
> +add a backing format to an existing image.  However, be aware that
> +there are already potential security risks to blindly using ``qemu-img
> +info`` to probe the format of an untrusted backing image, when
> +deciding what format to add into an existing image.

Nit: s/qemu/QEMU/g/

Ultra Nit: should this paragraph be broken down into two?  Experience
tells people usually feel deterred read "substantial paragraphs" :-)

I'll report back the Amend case.  (And once I get clarification on the
Convert scenario, I'll be happy to give Tested-by.)

[...]

-- 
/kashyap

On 3/9/20 10:31 AM, Kashyap Chamarthy wrote:
> On Fri, Mar 06, 2020 at 04:51:21PM -0600, Eric Blake wrote:
>> Creating an image that requires format probing of the backing image is
>> inherently unsafe (we've had several CVEs over the years based on

>>
>> +qemu-img backing file without format (since 5.0.0)
>> +''''''''''''''''''''''''''''''''''''''''''''''''''
>> +
>> +The use of ``qemu-img create``, ``qemu-img rebase``, ``qemu-img
>> +convert``, or ``qemu-img amend`` to create or modify an image that
>> +depends on a backing file now recommends that an explicit backing
>> +format be provided.  This is for safety: if qemu probes a different
>> +format than what you thought, the data presented to the guest will be
>> +corrupt; similarly, presenting a raw image to a guest allows a
>> +potential security exploit if a future probe sees a non-raw image
>> +based on guest writes.  To avoid the warning message, or even future
>> +refusal to create an unsafe image, you must pass ``-o backing_fmt=``
>> +(or the shorthand ``-F`` during create) to specify the intended
>> +backing format.  You may use ``qemu-img rebase -u`` to retroactively
>> +add a backing format to an existing image.  However, be aware that
>> +there are already potential security risks to blindly using ``qemu-img
>> +info`` to probe the format of an untrusted backing image, when
>> +deciding what format to add into an existing image.
> 
> Nit: s/qemu/QEMU/g/
> 
> Ultra Nit: should this paragraph be broken down into two?  Experience
> tells people usually feel deterred read "substantial paragraphs" :-)

Shoot, I missed incorporating this comment during my v4 posting. It's 
now changed in my local tree, but I'll hold off on a v5 unless other 
review warrants it.

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

On 3/9/20 10:31 AM, Kashyap Chamarthy wrote:

> After (with the patch series applied to QEMU Git):
> 
>      $> git describe
>      v4.2.0-2204-gd6c7830114
> 
>      # Create; *without* specifying "-F raw"
>      $> ~/build/qemu/qemu-img create -f qcow2 -b ./base.raw ./overlay2.qcow2
>      qemu-img: warning: Deprecated use of backing file without explicit backing format (detected format of raw)
>      Formatting './overlay2.qcow2', fmt=qcow2 size=4294967296 backing_file=./base.raw backing_fmt=raw cluster_size=65536 lazy_refcounts=off refcount_bits=16

If you'll note, this case _did_ write an implied backing_fmt=raw into 
the image.  Constrast that with creating an image on a qcow2 backing 
file, which tells you it detected a format of qcow2, but does NOT write 
backing_fmt=qcow2 into the image (this was a change from v2, at Peter's 
request).  Thus, when the backing is raw, we warn but future use of the 
image is now safe where it previously was not; when the backing file is 
non-raw, we warn but do not change our behavior, but because the backing 
file is non-raw any future probes will not be any less safe than before.

> 
>      # Rebase; *without* specifying "-F raw"
>      $> ~/build/qemu/qemu-img rebase -b base.raw overlay1.qcow2
>      qemu-img: warning: Deprecated use of backing file without explicit backing format, use of this image requires potentially unsafe format probing
> 
> 
> However, for the "Convert" case, is it correct that no warning is thrown
> for the below?
> 
>      $> ~/build/qemu/qemu-img info overlay1.qcow2
>      image: overlay1.qcow2
>      file format: qcow2
>      virtual size: 4 GiB (4294967296 bytes)
>      disk size: 196 KiB
>      cluster_size: 65536
>      backing file: base.raw
>      Format specific information:
>          compat: 1.1
>          lazy refcounts: false
>          refcount bits: 16
>          corrupt: false

We have an image with no backing format, so we had to probe.  This patch 
series did not change the behavior of opening an existing image, only 
for creating a new image (or amending an image in-place).  So the lack 
of a warning on opening the unsafe image may be desirable, but it would 
be via even more patches.

>      
>      
>      $> ~/build/qemu/qemu-img convert -f qcow2 -O qcow2 overlay1.qcow2 flattened.raw

Ouch - you are creating a qcow2 destination file named 'flattened.raw', 
which is rather confusing on your part.

However, as your destination file is being created without a backing 
image, it is to be expected that there is no warning (when there is no 
backing file, -F makes no sense).  To provoke the warning during 
convert, you'll have to also pass -B (or -o backing_file), without -o 
backing_fmt (since convert lacks the -F shorthand).

> 
>      $> echo $?
>      0
> 
>> diff --git a/docs/system/deprecated.rst b/docs/system/deprecated.rst
>> index 6c1d9034d9e3..a8ffacf54a52 100644
>> --- a/docs/system/deprecated.rst
>> +++ b/docs/system/deprecated.rst
>> @@ -376,6 +376,25 @@ The above, converted to the current supported format::
>>   Related binaries
>>   ----------------
>>
>> +qemu-img backing file without format (since 5.0.0)
>> +''''''''''''''''''''''''''''''''''''''''''''''''''
>> +
>> +The use of ``qemu-img create``, ``qemu-img rebase``, ``qemu-img
>> +convert``, or ``qemu-img amend`` to create or modify an image that
>> +depends on a backing file now recommends that an explicit backing
>> +format be provided.  This is for safety: if qemu probes a different
>> +format than what you thought, the data presented to the guest will be
>> +corrupt; similarly, presenting a raw image to a guest allows a
>> +potential security exploit if a future probe sees a non-raw image
>> +based on guest writes.  To avoid the warning message, or even future
>> +refusal to create an unsafe image, you must pass ``-o backing_fmt=``
>> +(or the shorthand ``-F`` during create) to specify the intended
>> +backing format.  You may use ``qemu-img rebase -u`` to retroactively
>> +add a backing format to an existing image.  However, be aware that
>> +there are already potential security risks to blindly using ``qemu-img
>> +info`` to probe the format of an untrusted backing image, when
>> +deciding what format to add into an existing image.
> 
> Nit: s/qemu/QEMU/g/
> 
> Ultra Nit: should this paragraph be broken down into two?  Experience
> tells people usually feel deterred read "substantial paragraphs" :-)

Could do, right before 'To avoid the warning'.

> 
> I'll report back the Amend case.  (And once I get clarification on the
> Convert scenario, I'll be happy to give Tested-by.)
> 
> [...]
> 

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

On Mon, Mar 09, 2020 at 10:42:25AM -0500, Eric Blake wrote:
> On 3/9/20 10:31 AM, Kashyap Chamarthy wrote:
> 
> > After (with the patch series applied to QEMU Git):
> > 
> >      $> git describe
> >      v4.2.0-2204-gd6c7830114
> > 
> >      # Create; *without* specifying "-F raw"
> >      $> ~/build/qemu/qemu-img create -f qcow2 -b ./base.raw ./overlay2.qcow2
> >      qemu-img: warning: Deprecated use of backing file without explicit backing format (detected format of raw)
> >      Formatting './overlay2.qcow2', fmt=qcow2 size=4294967296 backing_file=./base.raw backing_fmt=raw cluster_size=65536 lazy_refcounts=off refcount_bits=16
> 
> If you'll note, this case _did_ write an implied backing_fmt=raw into the
> image.  

Ah, I missed to notice that.  Interesting.

> Constrast that with creating an image on a qcow2 backing file, which
> tells you it detected a format of qcow2, but does NOT write
> backing_fmt=qcow2 into the image (this was a change from v2, at Peter's
> request).  

Indeed, here we go, confirming the overlay of a QCOW2 backing file _not_
having the 'backing_fmt' written into the image:

    $> ~/build/qemu/qemu-img create -f qcow2 -b ./another_base.qcow2 ./overlay1_of_ab.qcow2
    qemu-img: warning: Deprecated use of backing file without explicit backing format (detected format of qcow2)
    Formatting './overlay1_of_ab.qcow2', fmt=qcow2 size=4294967296 backing_file=./another_base.qcow2 cluster_size=65536 lazy_refcounts=off refcount_bits=16

That's neat.

(I wonder if this bit should also be documented.)

> Thus, when the backing is raw, we warn but future use of the
> image is now safe where it previously was not; when the backing file is
> non-raw, we warn but do not change our behavior, but because the backing
> file is non-raw any future probes will not be any less safe than before.

Understood.  Easy to miss when not paying attention; thanks for pointing
it out.

[...]

> > However, for the "Convert" case, is it correct that no warning is thrown
> > for the below?
> > 
> >      $> ~/build/qemu/qemu-img info overlay1.qcow2
> >      image: overlay1.qcow2
> >      file format: qcow2
> >      virtual size: 4 GiB (4294967296 bytes)
> >      disk size: 196 KiB
> >      cluster_size: 65536
> >      backing file: base.raw
> >      Format specific information:
> >          compat: 1.1
> >          lazy refcounts: false
> >          refcount bits: 16
> >          corrupt: false
> 
> We have an image with no backing format, so we had to probe.  This patch
> series did not change the behavior of opening an existing image, only for
> creating a new image (or amending an image in-place).  So the lack of a
> warning on opening the unsafe image may be desirable, but it would be via
> even more patches.

Fair enough; it's an understandable compromise.

And your series is a strict improvement as-is; it should not be held up.

> 
> >      $> ~/build/qemu/qemu-img convert -f qcow2 -O qcow2 overlay1.qcow2 flattened.raw
> 
> Ouch - you are creating a qcow2 destination file named 'flattened.raw',
> which is rather confusing on your part.

Oops, yes it is; bad me.  Sorry for the mix-up.  I meant to amend the
format to 'raw'.

> However, as your destination file is being created without a backing image,
> it is to be expected that there is no warning (when there is no backing
> file, -F makes no sense).  

Yeah, fair enough.

> To provoke the warning during convert, you'll
> have to also pass -B (or -o backing_file), without -o backing_fmt (since
> convert lacks the -F shorthand).

Hmm, I tried the following way, but it doesn't provoke the warning:

    $> ~/build/qemu/qemu-img convert -B ./base.raw -O qcow2 overlay1.qcow2 flattened.qcow2

    $> ~/build/qemu/qemu-img info flattened.qcow2 
    image: flattened.qcow2
    file format: qcow2
    virtual size: 4 GiB (4294967296 bytes)
    disk size: 196 KiB
    cluster_size: 65536
    backing file: ./base.raw
    Format specific information:
        compat: 1.1
        lazy refcounts: false
        refcount bits: 16
        corrupt: false

What am I missing?


            - - -

<digression>

Ah, didn't realize the inconsistency of 'convert' lacking the '-F'
shorthand ... which reminds me, there are at least _three_ ways that I
know of, to specify backing file format with 'create':

    $ qemu-img create -f qcow2 -o 'backing_file=./base.raw,backing_fmt=raw' ./overlay1.qcow2
    $ qemu-img create -f qcow2 -b ./base.raw -o backing_fmt=raw overlay1.qcow2
    $ qemu-img create -f qcow2 -b ./base.raw -F raw ./overlay1.qcow2

I'm wondering about the consistency of having all the above three
supported for other operations too.  Now I at least know 'convert' lacks
the "-F".

Not sure how much people care about this :-)

</digression>


[...]

-- 
/kashyap

On 3/10/20 4:47 AM, Kashyap Chamarthy wrote:

>> To provoke the warning during convert, you'll
>> have to also pass -B (or -o backing_file), without -o backing_fmt (since
>> convert lacks the -F shorthand).
> 
> Hmm, I tried the following way, but it doesn't provoke the warning:
> 
>      $> ~/build/qemu/qemu-img convert -B ./base.raw -O qcow2 overlay1.qcow2 flattened.qcow2
> 
>      $> ~/build/qemu/qemu-img info flattened.qcow2
>      image: flattened.qcow2
>      file format: qcow2
>      virtual size: 4 GiB (4294967296 bytes)
>      disk size: 196 KiB
>      cluster_size: 65536
>      backing file: ./base.raw
>      Format specific information:
>          compat: 1.1
>          lazy refcounts: false
>          refcount bits: 16
>          corrupt: false
> 
> What am I missing?

Rather, it looks like my patch is missing a warning on that path.  I'll 
have to investigate, for v4.

> 
> 
>              - - -
> 
> <digression>
> 
> Ah, didn't realize the inconsistency of 'convert' lacking the '-F'
> shorthand ... which reminds me, there are at least _three_ ways that I
> know of, to specify backing file format with 'create':
> 
>      $ qemu-img create -f qcow2 -o 'backing_file=./base.raw,backing_fmt=raw' ./overlay1.qcow2
>      $ qemu-img create -f qcow2 -b ./base.raw -o backing_fmt=raw overlay1.qcow2
>      $ qemu-img create -f qcow2 -b ./base.raw -F raw ./overlay1.qcow2
> 
> I'm wondering about the consistency of having all the above three
> supported for other operations too.  Now I at least know 'convert' lacks
> the "-F".

The -o forms (backing_file= and backing_fmt=) always work.  Various 
commands then have additional shorthand: -b/-F for create, -B for 
convert.  You're right that we aren't very consistent, but I'm reluctant 
to change the inconsistencies in this patch (at one point in the past, 
we tried to get rid of the shorthand and force all users to go through 
-o, but that broke too many clients that were depending on the 
undocumented shorthand, so we documented the existing shorthand instead).

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

On Tue, Mar 10, 2020 at 07:15:29AM -0500, Eric Blake wrote:
> On 3/10/20 4:47 AM, Kashyap Chamarthy wrote:
 
[...]

> > <digression>
> > 
> > Ah, didn't realize the inconsistency of 'convert' lacking the '-F'
> > shorthand ... which reminds me, there are at least _three_ ways that I
> > know of, to specify backing file format with 'create':
> > 
> >      $ qemu-img create -f qcow2 -o 'backing_file=./base.raw,backing_fmt=raw' ./overlay1.qcow2
> >      $ qemu-img create -f qcow2 -b ./base.raw -o backing_fmt=raw overlay1.qcow2
> >      $ qemu-img create -f qcow2 -b ./base.raw -F raw ./overlay1.qcow2
> > 
> > I'm wondering about the consistency of having all the above three
> > supported for other operations too.  Now I at least know 'convert' lacks
> > the "-F".
> 
> The -o forms (backing_file= and backing_fmt=) always work.  Various commands
> then have additional shorthand: -b/-F for create, -B for convert.  You're
> right that we aren't very consistent, but I'm reluctant to change the
> inconsistencies in this patch 

Oh, I wasn't implying to tackle the inconsistency as part of this
patch, or series.  Hence the 'digression' :-)  Was just wondering out
loud.

> (at one point in the past, we tried to get rid
> of the shorthand and force all users to go through -o, but that broke too
> many clients that were depending on the undocumented shorthand, so we
> documented the existing shorthand instead).

Fair enough; let's not touch these things for now.

-- 
/kashyap

On Mon, Mar 09, 2020 at 10:42:25AM -0500, Eric Blake wrote:

[...]

> > > +qemu-img backing file without format (since 5.0.0)
> > > +''''''''''''''''''''''''''''''''''''''''''''''''''
> > > +
> > > +The use of ``qemu-img create``, ``qemu-img rebase``, ``qemu-img
> > > +convert``, or ``qemu-img amend`` to create or modify an image that
> > > +depends on a backing file now recommends that an explicit backing
> > > +format be provided.

Also for the `qemu-img amend` case, I'm not clear if the following scenario
ought to throw the warning:

    $> ~/build/qemu/qemu-img info --backing-chain overlay1.qcow2 
    image: overlay1.qcow2
    file format: qcow2
    virtual size: 4 GiB (4294967296 bytes)
    disk size: 196 KiB
    cluster_size: 65536
    backing file: base.raw
    Format specific information:
        compat: 1.1
        lazy refcounts: false
        refcount bits: 16
        corrupt: false
    
    image: base.raw
    file format: raw
    virtual size: 4 GiB (4294967296 bytes)
    disk size: 778 MiB

    $> ~/build/qemu/qemu-img amend -o compat=v3 overlay1.qcow2 

    $> echo $?
    0

I'm just trying to work out the scenarios where the warnings ought to
show up (for all the four cases: create, rebase, convert, amend).

[...]

-- 
/kashyap

On 3/10/20 5:57 AM, Kashyap Chamarthy wrote:
> On Mon, Mar 09, 2020 at 10:42:25AM -0500, Eric Blake wrote:
> 
> [...]
> 
>>>> +qemu-img backing file without format (since 5.0.0)
>>>> +''''''''''''''''''''''''''''''''''''''''''''''''''
>>>> +
>>>> +The use of ``qemu-img create``, ``qemu-img rebase``, ``qemu-img
>>>> +convert``, or ``qemu-img amend`` to create or modify an image that
>>>> +depends on a backing file now recommends that an explicit backing
>>>> +format be provided.
> 
> Also for the `qemu-img amend` case, I'm not clear if the following scenario
> ought to throw the warning:
> 
>      $> ~/build/qemu/qemu-img info --backing-chain overlay1.qcow2
>      image: overlay1.qcow2
>      file format: qcow2
>      virtual size: 4 GiB (4294967296 bytes)
>      disk size: 196 KiB
>      cluster_size: 65536
>      backing file: base.raw
>      Format specific information:
>          compat: 1.1
>          lazy refcounts: false
>          refcount bits: 16
>          corrupt: false
>      
>      image: base.raw
>      file format: raw
>      virtual size: 4 GiB (4294967296 bytes)
>      disk size: 778 MiB
> 
>      $> ~/build/qemu/qemu-img amend -o compat=v3 overlay1.qcow2

This particular amend is not changing the backing file, and since I did 
not add warnings on the opening of an existing unsafe image, it is 
silent.  Instead, you need to test a case where amend provokes a path 
that would change the backing file (perhaps as simple as '-o 
backing_file=./base.raw'), while omitting a format for the new backing 
file string.

> 
>      $> echo $?
>      0
> 
> I'm just trying to work out the scenarios where the warnings ought to
> show up (for all the four cases: create, rebase, convert, amend).

Look at patch 2/4 - that patch was written AFTER this patch in order to 
silence every warning that was introduced because of this patch, then 
rebased to occur first.  My experience in writing 2/4 was that I indeed 
hit warnings through all four sub-commands.

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

On Tue, Mar 10, 2020 at 07:19:25AM -0500, Eric Blake wrote:
> On 3/10/20 5:57 AM, Kashyap Chamarthy wrote:

(Slightly long e-mail, as it contains a bunch of tests and their
results; please bear with me.)

[...]

> >      $> ~/build/qemu/qemu-img amend -o compat=v3 overlay1.qcow2
> 
> This particular amend is not changing the backing file, and since I did not
> add warnings on the opening of an existing unsafe image, it is silent.

I see; okay, that's expected.

> Instead, you need to test a case where amend provokes a path that would
> change the backing file (perhaps as simple as '-o backing_file=./base.raw'),
> while omitting a format for the new backing file string.

I couldn't work out the black magic to change the backing file via
'qemu-img amend'.  

It is surely not this:

    $> ~/build/qemu/qemu-img amend -o 'backing_file=./bar.qcow2' -o another_base.qcow2 
    qemu-img: Expecting one image file name

Let's try something else: give a *non-existent* "bar.qcow2" to '-o':

    $> ~/build/qemu/qemu-img amend -o 'backing_file=./bar.qcow2' another_base.qcow2 
    qemu-img: Could not open 'another_base.qcow2': Could not open backing file: Failed to get shared "write" lock
    Is another process using the image [./another_base.qcow2]?

That's strange; there is no live QEMU process on this host (let alone
one that is using another_base.qcow2):

    $> pgrep qemu-system-x86
    $> echo $?
    1

Probably it is just complaning about the non-existent bar.qcow2 file?
Then I'd expect it to say as much.


On IRC you pointed out iotest 082 to look for help.  There I don't see a
way to change the backing file.  But only a combination of 'amend' +
'rebase':

    run_qemu_img amend -f $IMGFMT \
        -o backing_fmt=$IMGFMT,backing_file="$TEST_IMG",,\? "$TEST_IMG"
    run_qemu_img rebase -u -b "" -f $IMGFMT "$TEST_IMG"

(I know you can use 'rebase' alone to change the backing file format.)

Note to self: we really need to document 'amend' much better, in which
scenarios it is useful, and contrast it with 'rebase'.

            - - -

Meanwhile, I've done a bunch of tests with 'amend'.  Here are the
results.

Scenario: base.raw <-- overlay1.qcow2
-------------------------------------

Without "-f raw", the warning is provoked when trying to amend the
backing file (let's ignore for a moment that you can't seem to amend a
raw file):

    $> ~/build/qemu/qemu-img amend -o compat=v3 base.raw
    WARNING: Image format was not specified for 'base.raw' and probing guessed raw.
             Automatically detecting the format is dangerous for raw images, write operations on block 0 will be restricted.
             Specify the 'raw' format explicitly to remove the restrictions.
    qemu-img: Format driver 'raw' does not support option amendment

With "-f raw", the warning is not triggerred (correctly so?):

    $> ~/build/qemu/qemu-img amend -o compat=v3 -f raw base.raw
    qemu-img: Format driver 'raw' does not support option amendment


And these two tests (one with relative path; the other with absolute
path) don't trigger the warning either (on IRC you said the following is
_supposed_ to trigger a warning):


    $> ~/build/qemu/qemu-img amend \
            -o 'backing_file=base.raw' -f qcow2 overlay1.qcow2

    $> ~/build/qemu/qemu-img amend \
            -o 'backing_file=./base.raw' -f qcow2 overlay1.qcow2


'qemu-img info' of the above disk image chain:

    $> ~/build/qemu/qemu-img info --backing-chain overlay1.qcow2
    image: overlay1.qcow2
    file format: qcow2
    virtual size: 4 GiB (4294967296 bytes)
    disk size: 196 KiB
    cluster_size: 65536
    backing file: ./base.raw
    backing file format: raw
    Format specific information:
        compat: 1.1
        lazy refcounts: false
        refcount bits: 16
        corrupt: false
    
    image: ./base.raw
    file format: raw
    virtual size: 4 GiB (4294967296 bytes)
    disk size: 778 MiB


Scenario: another_base.qcow2 <-- overlay1_of_ab.qcow2
-----------------------------------------------------

With and w/o specifying the aAmend the backing file (none of these
provoke the warning -- expected?):

    $> ~/build/qemu/qemu-img amend another_base.qcow2

    $> ~/build/qemu/qemu-img amend -f qcow2 another_base.qcow2

Tests to amend the overlay file (none of these provoke the warning --
expected, per your previous reply):

    $> ~/build/qemu/qemu-img amend overlay1_of_ab.qcow2  

    $> ~/build/qemu/qemu-img amend -f qcow2 overlay1_of_ab.qcow2  


'qemu-img info' of the above disk image chain:

    $> ~/build/qemu/qemu-img info --backing-chain overlay1_of_ab.qcow2 
    image: overlay1_of_ab.qcow2
    file format: qcow2
    virtual size: 4 GiB (4294967296 bytes)
    disk size: 196 KiB
    cluster_size: 65536
    backing file: ./another_base.qcow2
    Format specific information:
        compat: 1.1
        lazy refcounts: false
        refcount bits: 16
        corrupt: false
    
    image: ./another_base.qcow2
    file format: qcow2
    virtual size: 4 GiB (4294967296 bytes)
    disk size: 293 MiB
    cluster_size: 65536
    Format specific information:
        compat: 1.1
        lazy refcounts: false
        refcount bits: 16
        corrupt: false


Strange command-line
--------------------

Does this make sense?  What is this even trying to do ...

    $> ~/build/qemu/qemu-img amend \
            -o 'backing_file=./another_base.qcow2' another_base.qcow2 
    
    $> echo $?
    0

> Look at patch 2/4 - that patch was written AFTER this patch in order to
> silence every warning that was introduced because of this patch, then
> rebased to occur first.  My experience in writing 2/4 was that I indeed hit
> warnings through all four sub-commands. 

Will look.

Thanks.

-- 
/kashyap

On 3/10/20 5:57 AM, Kashyap Chamarthy wrote:
> On Mon, Mar 09, 2020 at 10:42:25AM -0500, Eric Blake wrote:
> 
> [...]
> 
>>>> +qemu-img backing file without format (since 5.0.0)
>>>> +''''''''''''''''''''''''''''''''''''''''''''''''''
>>>> +
>>>> +The use of ``qemu-img create``, ``qemu-img rebase``, ``qemu-img
>>>> +convert``, or ``qemu-img amend`` to create or modify an image that
>>>> +depends on a backing file now recommends that an explicit backing
>>>> +format be provided.
> 
> Also for the `qemu-img amend` case, I'm not clear if the following scenario
> ought to throw the warning:
> 
>      $> ~/build/qemu/qemu-img info --backing-chain overlay1.qcow2
>      image: overlay1.qcow2
>      file format: qcow2
>      virtual size: 4 GiB (4294967296 bytes)
>      disk size: 196 KiB
>      cluster_size: 65536
>      backing file: base.raw
>      Format specific information:
>          compat: 1.1
>          lazy refcounts: false
>          refcount bits: 16
>          corrupt: false
>      
>      image: base.raw
>      file format: raw
>      virtual size: 4 GiB (4294967296 bytes)
>      disk size: 778 MiB
> 
>      $> ~/build/qemu/qemu-img amend -o compat=v3 overlay1.qcow2

This particular amend is not changing the backing file, and since I did 
not add warnings on the opening of an existing unsafe image, it is 
silent.  Instead, you need to test a case where amend provokes a path 
that would change the backing file (perhaps as simple as '-o 
backing_file=./base.raw'), while omitting a format for the new backing 
file string.

> 
>      $> echo $?
>      0
> 
> I'm just trying to work out the scenarios where the warnings ought to
> show up (for all the four cases: create, rebase, convert, amend).
> 
> [...]
> 

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