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

[John Snow <jsnow@redhat.com>]

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