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

Re: [libvirt] [PATCH 3/3] Rewrite usb device version parsing



On Fri, Apr 10, 2015 at 16:28:24 +0200, Ján Tomko wrote:
> Simplify the function by leaving out the local copy and checking
> return values of virStrToLong.
> ---
>  src/conf/domain_conf.c | 66 +++++++++++++++-----------------------------------
>  1 file changed, 20 insertions(+), 46 deletions(-)
> 
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index 65e2bac..bea98a1 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -11306,66 +11306,40 @@ virDomainRedirdevDefParseXML(xmlNodePtr node,
>  }
>  
>  /*
> - * This is the helper function to convert USB version from a
> + * This is the helper function to convert USB device version from a
>   * format of JJ.MN to a format of 0xJJMN where JJ is the major
>   * version number, M is the minor version number and N is the
>   * sub minor version number.
> - * e.g. USB 2.0 is reported as 0x0200,
> - *      USB 1.1 as 0x0110 and USB 1.0 as 0x0100.
> + * e.g. USB version 2.0 is reported as 0x0200,
> + *      USB version 4.07 as 0x0407
>   */
>  static int
>  virDomainRedirFilterUSBVersionHelper(const char *version,
>                                       virDomainRedirFilterUSBDevDefPtr def)
>  {
> -    char *version_copy = NULL;
> -    char *temp = NULL;
> -    int ret = -1;
> -    size_t len;
> -    size_t fraction_len;
> -    unsigned int major;
> -    unsigned int minor;
> -    unsigned int hex;
> -
> -    if (VIR_STRDUP(version_copy, version) < 0)
> -        return -1;
> +    unsigned int major, minor;
> +    char *s = NULL;
>  
> -    len = strlen(version_copy);
> -    /*
> -     * The valid format of version is like 01.10, 1.10, 1.1, etc.
> -     */
> -    if (len > 5 ||
> -        !(temp = strchr(version_copy, '.')) ||
> -        temp - version_copy < 1 ||
> -        temp - version_copy > 2 ||
> -        !(fraction_len = strlen(temp + 1)) ||
> -        fraction_len > 2) {
> -        virReportError(VIR_ERR_INTERNAL_ERROR,
> -                       _("Incorrect USB version format %s"), version);
> -        goto cleanup;
> -    }
> +    if ((virStrToLong_ui(version, &s, 10, &major)) < 0 ||
> +        *s++ != '.' ||
> +        (virStrToLong_ui(s, NULL, 10, &minor)) < 0)
> +        goto error;
>  
> -    *temp = '\0';
> -    temp++;
> +    if (major >= 100 || minor >= 100)
> +        goto error;
>  
> -    if ((virStrToLong_ui(version_copy, NULL, 10, &major)) < 0 ||
> -        (virStrToLong_ui(temp, NULL, 10, &minor)) < 0) {
> -        virReportError(VIR_ERR_XML_ERROR,
> -                       _("Cannot parse USB version %s"), version);
> -        goto cleanup;
> -    }
> +    if (strlen(s) == 1)
> +        minor *= 10;

Humm, do we really want to fix user input in this case? I think that it
makes sense but a comment  explaining what that part does would be
actually helpful.

>  
> -    hex = (major / 10) << 12 | (major % 10) << 8;
> -    if (fraction_len == 1)
> -        hex |= (minor % 10) << 4;
> -    else
> -        hex |= (minor / 10) << 4 | (minor % 10) << 0;
> +    def->version = (major / 10) << 12 | (major % 10) << 8 |
> +                   (minor / 10) << 4 | (minor % 10) << 0;
>  
> -    def->version = hex;
> -    ret = 0;
> +    return 0;
>  
> - cleanup:
> -    VIR_FREE(version_copy);
> -    return ret;
> + error:
> +    virReportError(VIR_ERR_XML_ERROR,
> +                   _("Cannot parse USB device version %s"), version);
> +    return -1;
>  }


ACK with the comment added. It looks much better now.

Peter

Attachment: signature.asc
Description: Digital signature


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