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
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list