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

[Max Reitz]

On 2017-05-15 20:41, Max Reitz wrote:
> 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

...well, so much for that. I'll have to unstage it again because it
breaks a bunch of iotests (41 85 96 118 139 141 144 155 156) due to
failing to acquire image locks. :-/

I suspect this is because the backing file is opened somewhere and
trying to open it breaks now with the locking series applied.

Max

signature.asc
Description: OpenPGP digital signature