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

[Max Reitz]

On 2017-05-12 21:47, John Snow wrote:
> 
> 
> On 05/12/2017 03:46 PM, Eric Blake wrote:
>> On 05/12/2017 01:07 PM, Max Reitz wrote:
>>> On 2017-05-11 20:27, John Snow wrote:
>>>> Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=1213786
>>>>
>>>> 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.
>>>>
>>
>>>> +++ b/block.c
>>>> @@ -4275,37 +4275,37 @@ 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) {
>>>
>>> "Hang on, why should this be -1 when the defval is 0? Where does the -1
>>> come from?"
>>> "..."
>>> "Oh, the option exists and is set to -1? Why is that?"
>>> "..."
>>> "Oh, because this function always sets it itself, and because @img_size
>>> is set to (uint64_t)-1."
>>
>> I had pretty much the same conversation on my v1 review.
>> https://lists.gnu.org/archive/html/qemu-devel/2017-05/msg01097.html
>>
>>>
>>> First, I won't start with how signed integer overflow is
>>> implementation-defined in C because I hope you have thrashed that out
>>> with Eric (I hope that "to thrash out" is a good translation for
>>> "auskaspern" (lit. "to buffoon out").).
>>
>> Sounds like a reasonable choice of words, even if I don't speak the
>> counterpart language to validate your translation.
>>
>> (uint64_t)-1 is well-defined in C (so I think we're just fine here). But
>> (int64_t)UINT64_MAX is where signed integer overflow does indeed throw
>> wrinkles at you.

We're not really fine because that conversion happens when the result of
qemu_opt_get_size() (a uint64_t) is stored in size (an int64_t).

>> I seem to recall that qemu has chosen to use compiler flags and/or
>> assumptions that we are using 2s-complement arithmetic with sane
>> behavior (that is, tighter behavior than the bare minimum that C
>> requires), because it was easier than auditing our code for strict C
>> compliance on border cases of conversions from unsigned to signed that
>> trigger undefined behavior.  But again, I don't think it affects this
>> patch (where our conversion is only from signed to unsigned, and that is
>> well-defined behavior).

Right. Which is why I said I won't even start on it, but of course I
did. O:-)

>>> Second, well, at least we should put -1 as the default value here, then.
>>
>> Indeed, now that two reviewers have tripped on it,
>> qemu_opt_get_size(,,-1) would be nicer.
>>
>>>
>>> Not strictly your fault or something that you need to fix, but it is
>>> just a single line in the vicinity...
>>>
>>> Let me know if you want to address this, for now I'll leave a
>>>
>>> Reviewed-by: Max Reitz <address@hidden>
>>>
>>> here if you don't want to.
>>
>> I'm okay whether you want to squash that fix into this patch, or whether
>> you do it as a separate followup patch.
>>
> 
> I had considered the issue separate; but you're welcome to either write
> a patch or squish it into this one, I'm not going to be picky.

Yep, it is a separate issue, just related. :-)

But since you and Eric agree, I've squashed it in and thus I'm more than
glad to thank you very much and announce this patch as applied to my
block branch:

https://github.com/XanClic/qemu/commits/block

Max

signature.asc
Description: OpenPGP digital signature