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

[Eric Blake]

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 <address@hidden>

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

> Signed-off-by: John Snow <address@hidden>
> ---

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

signature.asc
Description: OpenPGP digital signature