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

[Max Reitz]

On 2017-05-16 10:17, Kevin Wolf wrote:
> Am 15.05.2017 um 21:17 hat Max Reitz geschrieben:
>> 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.
> 
> I think we can legitimately set force-shared=on for opening the backing
> file when testing whether the file exists.

For testing whether the file exists, probably. I wouldn't call it
legitimately because I don't like making that the default at all, but it
should work.

But then we have to differentiate between testing whether the file
exists and opening it to read its size; I'm even more wary regarding the
latter.

All in all, I've stated my opinion on this matter on IRC: I don't know
how much we need this patch. It's nice to have, it's convenient to know
you have mistyped the backing filename before you try to use the image
in qemu, but it's not critical. I don't consider the current behavior a
bug per-se, it's just counterintuitive behavior (that will not cut your
head off if you don't know it, though!).

(Also, we have a lot of counterintuitive behavior. See this:

$ qemu-img create -f qcow2 sub/backing.qcow2 64M
Formatting 'sub/backing.qcow2', fmt=qcow2 size=67108864 encryption=off
cluster_size=65536 lazy_refcounts=off refcount_bits=16
$ qemu-img create -f qcow2 -b sub/backing.qcow2 sub/top.qcow2
qemu-img: sub/top.qcow2: Could not open 'sub/sub/backing.qcow2': No such
file or directory

Yeah, sure, you'll know after you've done the mistake once. But the same
applies here, too, doesn't it...?)

So for me, it's a question of convenience: How much does the current
behavior annoy users? How much annoyance will changing the behavior
bring? I'm not (or rather, no longer) convinced the former outweighs the
latter, especially since it means having to change management tools
(which create external snapshots with qemu-img create while the backing
image is in use by qemu).

If you think otherwise, well, you're the main maintainer. :-)

Max


PS: Can there be a middle ground? Like just printing a warning if the
backing file cannot be opened and we don't absolutely have to?

signature.asc
Description: OpenPGP digital signature