[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v4] qemu-img: Check for backing image if specifi
From: |
Eric Blake |
Subject: |
Re: [Qemu-devel] [PATCH v4] qemu-img: Check for backing image if specified during create |
Date: |
Fri, 12 May 2017 14:46:08 -0500 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.1.0 |
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.
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).
>
> 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.
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3266
Virtualization: qemu.org | libvirt.org
signature.asc
Description: OpenPGP digital signature
- [Qemu-devel] [PATCH v4] qemu-img: Check for backing image if specified during create, John Snow, 2017/05/11
- Re: [Qemu-devel] [PATCH v4] qemu-img: Check for backing image if specified during create, Max Reitz, 2017/05/12
- Re: [Qemu-devel] [PATCH v4] qemu-img: Check for backing image if specified during create,
Eric Blake <=
- Re: [Qemu-devel] [PATCH v4] qemu-img: Check for backing image if specified during create, John Snow, 2017/05/12
- Re: [Qemu-devel] [PATCH v4] qemu-img: Check for backing image if specified during create, Max Reitz, 2017/05/15
- Re: [Qemu-devel] [PATCH v4] qemu-img: Check for backing image if specified during create, Max Reitz, 2017/05/15
- Re: [Qemu-devel] [PATCH v4] qemu-img: Check for backing image if specified during create, Kevin Wolf, 2017/05/16
- Re: [Qemu-devel] [PATCH v4] qemu-img: Check for backing image if specified during create, Max Reitz, 2017/05/17