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. > > 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 <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. --js