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 <mre...@redhat.com> > >>>> > >>>> 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. Kevin
pgp5wL9H1o0v9.pgp
Description: PGP signature