[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



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.

> @@ -213,37 +217,34 @@ esxUtil_ParseDatastoreRelatedPath(const char *datastoreRelatedPath,
>          return -1;
>      }
>  
> -    /*
> -     * Parse string as '[<datastore>] <path>'. '%as' is similar to '%s', but
> -     * sscanf() will allocate the memory for the string, so the caller doesn't
> -     * need to preallocate a buffer that's large enough.
> -     *
> -     * The s in '%as' can be replaced with a character set, e.g. [a-z].
> -     *
> -     * '%a[^]%]' matches <datastore>. '[^]%]' excludes ']' from the accepted
> -     * characters, otherwise sscanf() wont match what it should.
> -     *
> -     * '%a[^\n]' matches <path>. '[^\n]' excludes '\n' from the accepted
> -     * characters, otherwise sscanf() would only match up to the first space,
> -     * but spaces are valid in <path>.
> -     */
> -    if (sscanf(datastoreRelatedPath, "[%a[^]%]] %a[^\n]", datastoreName,
> -               &directoryAndFileName) != 2) {
> +    if (esxVI_String_DeepCopyValue(&copyOfDatastoreRelatedPath,
> +                                   datastoreRelatedPath) < 0) {
> +        goto failure;
> +    }
> +
> +    /* Expected format: '[<datastore>] <path>' */
> +    if ((tmp = STRSKIP(copyOfDatastoreRelatedPath, "[")) == NULL ||
> +        (preliminaryDatastoreName = strtok_r(tmp, "]", &saveptr)) == NULL ||
> +        (directoryAndFileName = strtok_r(NULL, "", &saveptr)) == NULL) {

Even though it is more lines of code, it is much smaller based on fewer
justification comments, and possibly faster execution to boot (strtok_r
is certainly simpler to implement than sscanf).  I like it.

> +int
> +esxVI_ParseHostCpuIdInfo(esxVI_ParsedHostCpuIdInfo *parsedhostCpuIdInfo,
> +                         esxVI_HostCpuIdInfo *hostCpuIdInfo)
> +{
> +    int expectedLength = 39; /* = strlen("----:----:----:----:----:----:----:----"); */
> +    char *input[4] = { hostCpuIdInfo->eax, hostCpuIdInfo->ebx,
> +                       hostCpuIdInfo->ecx, hostCpuIdInfo->edx };
> +    char *output[4] = { parsedhostCpuIdInfo->eax, parsedhostCpuIdInfo->ebx,
> +                        parsedhostCpuIdInfo->ecx, parsedhostCpuIdInfo->edx };
> +    const char *name[4] = { "eax", "ebx", "ecx", "edx" };
> +    int r, i, o;
> +
> +    memset(parsedhostCpuIdInfo, 0, sizeof (esxVI_ParsedHostCpuIdInfo));
> +
> +    parsedhostCpuIdInfo->level = hostCpuIdInfo->level->value;
> +
> +    for (r = 0; r < 4; ++r) {
> +        if (strlen(input[r]) != expectedLength) {
> +            ESX_VI_ERROR(VIR_ERR_INTERNAL_ERROR,
> +                         _("HostCpuIdInfo register '%s' has an unexpected length"),
> +                         name[r]);
> +            goto failure;
> +        }
> +
> +        /* Strip the ':' and invert the "bit" order from 31..0 to 0..31 */
> +        for (i = 0, o = 31; i < expectedLength; i += 5, o -= 4) {
> +            output[r][o] = input[r][i];
> +            output[r][o - 1] = input[r][i + 1];
> +            output[r][o - 2] = input[r][i + 2];
> +            output[r][o - 3] = input[r][i + 3];
> +
> +            if (i + 4 < expectedLength && input[r][i + 4] != ':') {
> +                ESX_VI_ERROR(VIR_ERR_INTERNAL_ERROR,
> +                             _("HostCpuIdInfo register '%s' has an unexpected format"),
> +                             name[r]);
> +                goto failure;
> +            }
> +        }

I spent quite a few minutes looking at this, and didn't see any semantic
differences between this longer implementation and the original sscanf.

> +    }
> +
> +    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).

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.

-- 
Eric Blake   eblake redhat com    +1-801-349-2682
Libvirt virtualization library http://libvirt.org

Attachment: signature.asc
Description: OpenPGP digital signature


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