[PATCH v2 02/19] Refactoring virDomainHostdevSubsysUSBDefParseXML() to use XPath

[Michal Prívozník]

On 5/4/21 1:39 PM, Kristina Hanicova wrote:
> Signed-off-by: Kristina Hanicova <khanicov at redhat.com>
> ---
>  src/conf/domain_conf.c | 130 +++++++++++++++++++----------------------
>  1 file changed, 60 insertions(+), 70 deletions(-)
> 
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index e073644810..5c5b4ad6d7 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -6709,14 +6709,16 @@ virDomainDeviceInfoParseXML(virDomainXMLOption *xmlopt,
>  
>  static int
>  virDomainHostdevSubsysUSBDefParseXML(xmlNodePtr node,
> -                                     xmlXPathContextPtr ctxt G_GNUC_UNUSED,
> +                                     xmlXPathContextPtr ctxt,
>                                       virDomainHostdevDef *def)
>  {
>      bool got_product, got_vendor;
> -    xmlNodePtr cur;
>      virDomainHostdevSubsysUSB *usbsrc = &def->source.subsys.u.usb;
>      g_autofree char *startupPolicy = NULL;
>      g_autofree char *autoAddress = NULL;
> +    VIR_XPATH_NODE_AUTORESTORE(ctxt)
> +
> +    ctxt->node = node;
>  
>      if ((startupPolicy = virXMLPropString(node, "startupPolicy"))) {
>          def->startupPolicy =
> @@ -6737,79 +6739,67 @@ virDomainHostdevSubsysUSBDefParseXML(xmlNodePtr node,
>      got_product = false;
>      got_vendor = false;
>  
> -    cur = node->children;
> -    while (cur != NULL) {
> -        if (cur->type == XML_ELEMENT_NODE) {
> -            if (virXMLNodeNameEqual(cur, "vendor")) {
> -                g_autofree char *vendor = virXMLPropString(cur, "id");
> -
> -                if (vendor) {
> -                    got_vendor = true;
> -                    if (virStrToLong_ui(vendor, NULL, 0, &usbsrc->vendor) < 0) {
> -                        virReportError(VIR_ERR_INTERNAL_ERROR,
> -                                       _("cannot parse vendor id %s"), vendor);
> -                        return -1;
> -                    }
> -                } else {
> -                    virReportError(VIR_ERR_INTERNAL_ERROR,
> -                                   "%s", _("usb vendor needs id"));
> -                    return -1;
> -                }
> -            } else if (virXMLNodeNameEqual(cur, "product")) {
> -                g_autofree char *product = virXMLPropString(cur, "id");
> -
> -                if (product) {
> -                    got_product = true;
> -                    if (virStrToLong_ui(product, NULL, 0,
> -                                        &usbsrc->product) < 0) {
> -                        virReportError(VIR_ERR_INTERNAL_ERROR,
> -                                       _("cannot parse product %s"),
> -                                       product);
> -                        return -1;
> -                    }
> -                } else {
> -                    virReportError(VIR_ERR_INTERNAL_ERROR,
> -                                   "%s", _("usb product needs id"));
> -                    return -1;
> -                }
> -            } else if (virXMLNodeNameEqual(cur, "address")) {
> -                g_autofree char *bus = NULL;
> -                g_autofree char *device = NULL;
> -
> -                bus = virXMLPropString(cur, "bus");
> -                if (bus) {
> -                    if (virStrToLong_ui(bus, NULL, 0, &usbsrc->bus) < 0) {
> -                        virReportError(VIR_ERR_INTERNAL_ERROR,
> -                                       _("cannot parse bus %s"), bus);
> -                        return -1;
> -                    }
> -                } else {
> -                    virReportError(VIR_ERR_INTERNAL_ERROR,
> -                                   "%s", _("usb address needs bus id"));
> -                    return -1;
> -                }
> +    if (virXPathNode("./vendor", ctxt)) {
> +        g_autofree char *vendor = NULL;
>  
> -                device = virXMLPropString(cur, "device");
> -                if (device) {
> -                    if (virStrToLong_ui(device, NULL, 0, &usbsrc->device) < 0) {
> -                        virReportError(VIR_ERR_INTERNAL_ERROR,
> -                                       _("cannot parse device %s"),
> -                                       device);
> -                        return -1;
> -                    }
> -                } else {
> -                    virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> -                                   _("usb address needs device id"));
> -                    return -1;
> -                }
> -            } else {
> +        if ((vendor = virXPathString("string(./vendor/@id)", ctxt))) {
> +            got_vendor = true;
> +            if (virStrToLong_ui(vendor, NULL, 0, &usbsrc->vendor) < 0) {

Since you are touching this strToint conversion anyway, you could have
used virXMLPropUInt(). Nothing critical, just pointing that out. It also
applies to the rest of patches. I'll fix them before merge.

>                  virReportError(VIR_ERR_INTERNAL_ERROR,
> -                               _("unknown usb source type '%s'"),
> -                               cur->name);
> +                               _("cannot parse vendor id %s"), vendor);
>                  return -1;
>              }
> +        } else {
> +            virReportError(VIR_ERR_INTERNAL_ERROR,
> +                           "%s", _("usb vendor needs id"));
> +            return -1;
> +        }
> +    }
> +
> +    if (virXPathNode("./product", ctxt)) {
> +        g_autofree char *product = NULL;
> +
> +        if ((product = virXPathString("string(./product/@id)", ctxt))) {
> +            got_product = true;
> +            if (virStrToLong_ui(product, NULL, 0, &usbsrc->product) < 0) {
> +                virReportError(VIR_ERR_INTERNAL_ERROR,
> +                               _("cannot parse product %s"), product);
> +                return -1;
> +            }
> +        } else {
> +            virReportError(VIR_ERR_INTERNAL_ERROR,
> +                           "%s", _("usb product needs id"));
> +            return -1;
> +        }
> +    }
> +
> +    if (virXPathNode("./address", ctxt)) {
> +        g_autofree char *bus = NULL;
> +        g_autofree char *device = NULL;
> +
> +        if ((bus = virXPathString("string(./address/@bus)", ctxt))) {
> +            if (virStrToLong_ui(bus, NULL, 0, &usbsrc->bus) < 0) {
> +                virReportError(VIR_ERR_INTERNAL_ERROR,
> +                               _("cannot parse bus %s"), bus);
> +                return -1;
> +            }
> +        } else {
> +            virReportError(VIR_ERR_INTERNAL_ERROR,
> +                           "%s", _("usb address needs bus id"));
> +            return -1;
> +        }
> +
> +        if ((device = virXPathString("string(./address/@device)", ctxt))) {
> +            if (virStrToLong_ui(device, NULL, 0, &usbsrc->device) < 0) {
> +                virReportError(VIR_ERR_INTERNAL_ERROR,
> +                               _("cannot parse device %s"), device);
> +                return -1;
> +            }
> +        } else {
> +            virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> +                           _("usb address needs device id"));
> +            return -1;
>          }
> -        cur = cur->next;
>      }
>  
>      if (got_vendor && usbsrc->vendor == 0) {
> 


Michal