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(©OfDatastoreRelatedPath,
> + 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@xxxxxxxxxx +1-801-349-2682
Libvirt virtualization library http://libvirt.org
Attachment:
signature.asc
Description: OpenPGP digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list