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

[Eric Blake <eblake@redhat.com>]

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

On 05/04/2017 02:30 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 of if a filesize

s/of if/when/

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

Indeed, reviewing under 'git diff -b' proved fruitful.

> Inspired by: https://bugzilla.redhat.com/show_bug.cgi?id=1213786
> 
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
>  block.c                    | 75 +++++++++++++++++++++++-----------------------
>  qemu-img-cmds.hx           |  4 +--

Conflicts with the documentation patch for [-b backing_file] (already on
PULL request:
https://lists.gnu.org/archive/html/qemu-devel/2017-04/msg05816.html

so you'll have to rebase, but it shouldn't be too difficult.

>  qemu-img.c                 | 15 ++++++----
>  tests/qemu-iotests/082     |  4 +--
>  tests/qemu-iotests/082.out |  4 +--
>  5 files changed, 54 insertions(+), 48 deletions(-)
> 
> diff --git a/block.c b/block.c
> index 5db266b..da28052 100644
> --- a/block.c
> +++ b/block.c
> @@ -4280,38 +4280,38 @@ void bdrv_img_create(const char *filename, const char *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
>      size = qemu_opt_get_size(opts, BLOCK_OPT_SIZE, 0);
> -    if (size == -1) {

Wait. How does this even work?  qemu_opt_get_size() returns an unsigned
value, either what was specified by the option, or else the default
parameter (which  we passed as 0), so how can size ever be -1?  It might
make sense if we had called:

size = qemu_opt_get_size(opts, BLOCK_OPT_SIZE, -1);

with the intent of allowing an explicit size of 0.  But I'm lost as to
how this code ever worked pre-patch...


> +
> +        if (size == -1) {

...which therefore carries over to the state of things post-patch.

/me goes and reads code

What?
if (desc && desc->def_value_str) {
  parse_option_size(name, desc->def_value_str, &ret, &error_abort);
  return ret;

Oh - so THAT's how: qemu_opt_get_size_helper() ignores the 'defval'
input and instead returns a DIFFERENT default associated with the
QemuOpts. UGLY.  But not your fault.

> +++ b/qemu-img.c
> @@ -145,9 +145,10 @@ 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 new \n"
> +           "       backing file match exactly. The image doesn't need a working backing file \n"
> +           "       before rebasing in this case (useful for renaming the backing file)\n"
> +           "       For image creation, allow creating without attempting to open the file.\n"

maybe s/to open the file/to open the backing file/

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

Nice test of the new usage.

I like where it's headed.  It will need a v2 to rebase and fix the
things I pointed out, but I'm looking forward to it.

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


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