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

Re: [libvirt] [PATCHv2 03/10] Refactor major.minor.micro version parsing into a function



On 03/31/2010 03:41 PM, Matthias Bolte wrote:
> virParseVersionString uses virStrToLong_ui instead of sscanf.
> 
> This also fixes a bug in the UML driver, that always returned 0
> as version number.
> 
> Introduce STRSKIP to check if a string has a certain prefix and
> to skip this prefix.

> +++ b/src/esx/esx_driver.c
> @@ -684,34 +684,17 @@ static int
>  esxGetVersion(virConnectPtr conn, unsigned long *version)
>  {
...
> +    if (virParseVersionString(priv->host->service->about->version,
> +                              version) < 0) {
> +        ESX_ERROR(VIR_ERR_INTERNAL_ERROR,
> +                  "Could not parse version number from '%s'",
> +                  priv->host->service->about->version);

String needs translation.

> diff --git a/src/internal.h b/src/internal.h
> index f82fbd2..807288b 100644
> --- a/src/internal.h
> +++ b/src/internal.h
> @@ -57,6 +57,7 @@
>  # define STRNEQLEN(a,b,n) (strncmp(a,b,n) != 0)
>  # define STRCASENEQLEN(a,b,n) (strncasecmp(a,b,n) != 0)
>  # define STRPREFIX(a,b) (strncmp(a,b,strlen(b)) == 0)
> +# define STRSKIP(a,b) (STRPREFIX(a,b) ? (a) + strlen(b) : NULL)
...

> +    /* expected format: vzctl version <major>.<minor>.<micro> */
> +    if ((tmp = STRSKIP(tmp, "vzctl version ")) == NULL)
>          goto cleanup2;

Yes, that looks nicer.  Glad I did my thinking out loud, and for other's
input on the name.

> +++ b/src/util/util.c
> @@ -2074,6 +2074,41 @@ virParseNumber(const char **str)
>      return (ret);
>  }
>  
> +
> +/**
> + * virParseVersionString:
> + * @str: const char pointer to the version string
> + * @version: unsigned long pointer to output the version number
> + *
> + * Parse an unsigned version number from a version string. Expecting
> + * 'major.minor.micro' format, ignoring an optional suffix.
> + *
> + * The major, minor and micro numbers are encoded into a single version number:
> + *
> + *   1000000 * major + 1000 * minor + micro
> + *
> + * Returns the 0 for success, -1 for error.
> + */
> +int
> +virParseVersionString(const char *str, unsigned long *version)
> +{
> +    unsigned int major, minor, micro;
> +    char *tmp = (char *)str;

Coreutils uses an idiom bad_cast(str) to make it obvious that we are
casting away the const, and that we have audited that the results are
still sane (virStrToLong_ui does not modify its argument), while at the
same time reducing the number of C-style casts that require you to think
why we are casting.  But introducing bad_cast() into libvirt would be a
separate patch; your code is fine as-is for this round.

ACK with the translation markup.

-- 
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]