Re: [libvirt] [PATCH] esx: Replace scanf with STRSKIP and strtok_r
2010/4/15 Eric Blake <eblake(a)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