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

[Markus Armbruster <armbru@redhat.com>]

Eric Blake <eblake@redhat.com> writes:

> On 09/12/2016 04:18 AM, Marc-André Lureau wrote:
>> There are better chances to find what went wrong at build time than a
>> later assert in qmp_query_version
>> 
>> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
>> ---
>>  qmp.c                 | 16 +++-------------
>>  scripts/create_config |  6 ++++++
>>  2 files changed, 9 insertions(+), 13 deletions(-)
>> 
>> diff --git a/qmp.c b/qmp.c
>> index dea8f81..6733463 100644
>> --- a/qmp.c
>> +++ b/qmp.c
>> @@ -51,21 +51,11 @@ NameInfo *qmp_query_name(Error **errp)
>>  VersionInfo *qmp_query_version(Error **errp)
>>  {
>>      VersionInfo *info = g_new0(VersionInfo, 1);
>> -    const char *version = QEMU_VERSION;
>> -    const char *tmp;
>> -    int err;
>>  
>>      info->qemu = g_new0(VersionTriple, 1);
>> -    err = qemu_strtoll(version, &tmp, 10, &info->qemu->major);
>> -    assert(err == 0);
>> -    tmp++;
>> -
>> -    err = qemu_strtoll(tmp, &tmp, 10, &info->qemu->micro);
>> -    assert(err == 0);
>> +    info->qemu->major = QEMU_VERSION_MAJOR;
>> +    info->qemu->minor = QEMU_VERSION_MINOR;
>> +    info->qemu->micro = QEMU_VERSION_MICRO;
>>      info->package = g_strdup(QEMU_PKGVERSION);
>>  
>>      return info;
>
> 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
'.'.

>> diff --git a/scripts/create_config b/scripts/create_config
>> index 1dd6a35..e6929dd 100755
>> --- a/scripts/create_config
>> +++ b/scripts/create_config
>> @@ -7,7 +7,13 @@ while read line; do
>>  case $line in
>>   VERSION=*) # configuration
>>      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

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.

> Do we care either way?  Unless someone else has a reason for why we
> should care, I'm fine with:
> Reviewed-by: Eric Blake <eblake@redhat.com>

Does your R-by stand?