Re: [Qemu-devel] [PATCH v6 01/18] build-sys: define QEMU_VERSION_{MAJOR, MINOR, MICRO}

[Eric Blake]

On 09/13/2016 02:33 AM, Markus Armbruster wrote:

>>>      info->qemu = g_new0(VersionTriple, 1);
>>> -    err = qemu_strtoll(version, &tmp, 10, &info->qemu->major);
>>> -    assert(err == 0);
>>> -    tmp++;
>>> -

>>
>> The old code silently ignores any garbage after the third integer (it
>> populates &tmp, but never checks the value of tmp).
> 
> It also accepts any separator character between the integers, not just
> '.'.

True, and didn't we have a release of qemu a few years ago (perhaps it
was a downstream Red Hat release; I can't quickly find it now...) where
we accidentally released the version string with a comma instead of a dot?

>>>      version=${line#*=}
>>> +    major=$(echo "$version" | cut -d. -f1)
>>> +    minor=$(echo "$version" | cut -d. -f2)
>>> +    micro=$(echo "$version" | cut -d. -f3)
>>>      echo "#define QEMU_VERSION \"$version\""
>>> +    echo "#define QEMU_VERSION_MAJOR $major"
>>> +    echo "#define QEMU_VERSION_MINOR $minor"
>>> +    echo "#define QEMU_VERSION_MICRO $micro"
>>
>> The new code likewise ignores any fourth field.
> 
> Nope:
> 
>     $ echo "2.7.50garbage" | cut -d. -f3
>     50garbage

That's not a fourth field.  I was thinking '2.7.50.0' ignoring '.0'.

> 
> The compiler will choke on
> 
>     info->qemu->micro = QEMU_VERSION_MICRO;
> 
> because it's
> 
>     info->qemu->micro = 50garbage;
> 
> after preprocessing.
> 
> The commit message could mention that VERSION is now parsed more
> strictly (configure checks '.', the compiler checks integers), but I
> guess it's not worth the bother now.

Indeed, the fact that we now separate on exactly '.' instead of the end
of an integer is different.

> 
>> Do we care either way?  Unless someone else has a reason for why we
>> should care, I'm fine with:
>> Reviewed-by: Eric Blake <address@hidden>
> 
> Does your R-by stand?

Yes - while the parsing may have changed, it doesn't affect the common
case of a correctly-formed VERSION file. I'm okay if you want to tweak
the commit message to call out a summary of this conversation, but since
the patch is already on qapi-next staging, I'm also okay if it doesn't
change.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

signature.asc
Description: OpenPGP digital signature