[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [libvirt] [PATCH] esx: Replace scanf with STRSKIP and strtok_r



2010/4/15 Eric Blake <eblake redhat com>:
> On 04/14/2010 06:34 PM, Matthias Bolte wrote:
>> This also fixes a portability problem with the %a format modifier.
>> %a is not portable and made esxDomainDumpXML fail at runtime in
>> MinGW builds.
>>
>> Pull in strtok_r from gnulib, because MinGW lacks it.
>> ---
>>  bootstrap.conf       |    1 +
>
> See my comments about moving this hunk into your patch for .gnulib.
>
>> @@ -60,8 +60,8 @@ esxSupportsLongMode(esxPrivate *priv)
>>      esxVI_DynamicProperty *dynamicProperty = NULL;
>>      esxVI_HostCpuIdInfo *hostCpuIdInfoList = NULL;
>>      esxVI_HostCpuIdInfo *hostCpuIdInfo = NULL;
>> +    esxVI_ParsedHostCpuIdInfo parsedHostCpuIdInfo;
>>      char edxLongModeBit = '?';
>> -    char edxFirstBit = '?';
>>
>>      if (priv->supportsLongMode != esxVI_Boolean_Undefined) {
>>          return priv->supportsLongMode;
>> @@ -96,23 +96,12 @@ esxSupportsLongMode(esxPrivate *priv)
>>              for (hostCpuIdInfo = hostCpuIdInfoList; hostCpuIdInfo != NULL;
>>                   hostCpuIdInfo = hostCpuIdInfo->_next) {
>>                  if (hostCpuIdInfo->level->value == -2147483647) { /* 0x80000001 */
>> -#define _SKIP4 "%*c%*c%*c%*c"
>> -#define _SKIP12 _SKIP4":"_SKIP4":"_SKIP4
>> -
>> -                    /* Expected format: "--X-:----:----:----:----:----:----:----" */
>> -                    if (sscanf(hostCpuIdInfo->edx,
>> -                               "%*c%*c%c%*c:"_SKIP12":"_SKIP12":%*c%*c%*c%c",
>> -                               &edxLongModeBit, &edxFirstBit) != 2) {
>
> Oh my.  I see why you want this replaced.

I thinks the ParsedHostCpuIdInfo will also come in handy for adding
detailed CPU feature/selection, because it allows simpler access to
the individual bits of the CPUID output.


>> +    }
>> +
>> +    return 0;
>> +
>> +  failure:
>> +    memset(parsedhostCpuIdInfo, 0, sizeof (esxVI_ParsedHostCpuIdInfo));
>
> In general, I prefer to see:
>
> memset(parsedhostCpuIdInfo, 0, sizeof *parsedhostCpuIdInfo);
>
> on the grounds that sizeof expr is more robust if expr changes types, in
> contrast to sizeof(type).

On the other hand sizeof expr is more prone to missing *.

Anyway, I switch it to sizeof expr.

> Many existing uses in libvirt also have the style
> sizeof(*parsedhostCpuIdInfo), although that makes it harder to see at a
> glance whether you are using sizeof expr or sizeof(type) for expressions
> that do not include *.  So it's up to you whether to include () around
> the expr.
>
> ACK with that nit addressed.
>

Thanks, pushed.

Matthias


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]