[PATCH v2 00/19] Refactoring conf to use XPath
This is v2 from https://listman.redhat.com/archives/libvir-list/2021-April/msg00617.html Changes since v1: - rebase to the lastest git master This series reworks the outdated way of parsing XML to parsing by XPath, which is more obvious and saves a few lines of code. Kristina Hanicova (19): conf: Propagate xmlXPathContextPtr into virDomainHostdevSubsysUSBDefParseXML() Refactoring virDomainHostdevSubsysUSBDefParseXML() to use XPath conf: Propagate xmlXPathContextPtr into virDomainBlkioDeviceParseXML() Refactoring virDomainBlkioDeviceParseXML() to use XPath conf: Propagate xmlXPathContextPtr into virDomainHostdevSubsysPCIDefParseXML() Refactoring virDomainHostdevSubsysPCIDefParseXML() to use XPath conf: Propagate xmlXPathContextPtr into virDomainLeaseDefParseXML() Refactoring virDomainLeaseDefParseXML() to use XPath Refactoring virDomainFSDefParseXML() to use XPath Refactoring virDomainNetDefParseXML() to use XPath conf: Propagate xmlXPathContextPtr into virDomainChrDefParseTargetXML() Refactoring virDomainChrDefParseTargetXML() to use XPath Refactoring virDomainChrSourceDefParseXML() to use XPath Refactoring virDomainChrDefParseXML() to use XPath Refactoring virDomainSmartcardDefParseXML() to use XPath Refactoring virDomainGraphicsDefParseXMLSpice() to use XPath conf: Propagate xmlXPathContextPtr into virDomainVideoDriverDefParseXML() Refactoring virDomainVideoDriverDefParseXML() to use XPath Refactoring virDomainVideoDefParseXML() to use XPath src/conf/domain_conf.c | 1641 +++++++++++++++++----------------------- 1 file changed, 707 insertions(+), 934 deletions(-) -- 2.30.2
Signed-off-by: Kristina Hanicova <khanicov@redhat.com> --- src/conf/domain_conf.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index cb668d3d5e..e073644810 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -6709,6 +6709,7 @@ virDomainDeviceInfoParseXML(virDomainXMLOption *xmlopt, static int virDomainHostdevSubsysUSBDefParseXML(xmlNodePtr node, + xmlXPathContextPtr ctxt G_GNUC_UNUSED, virDomainHostdevDef *def) { bool got_product, got_vendor; @@ -7435,7 +7436,7 @@ virDomainHostdevDefParseXMLSubsys(xmlNodePtr node, break; case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_USB: - if (virDomainHostdevSubsysUSBDefParseXML(sourcenode, def) < 0) + if (virDomainHostdevSubsysUSBDefParseXML(sourcenode, ctxt, def) < 0) return -1; break; -- 2.30.2
Signed-off-by: Kristina Hanicova <khanicov@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) { 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) { -- 2.30.2
On 5/4/21 1:39 PM, Kristina Hanicova wrote:
Signed-off-by: Kristina Hanicova <khanicov@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
Signed-off-by: Kristina Hanicova <khanicov@redhat.com> --- src/conf/domain_conf.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 5c5b4ad6d7..b369c49b05 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -1666,6 +1666,7 @@ virBlkioDeviceArrayClear(virBlkioDevice *devices, */ static int virDomainBlkioDeviceParseXML(xmlNodePtr root, + xmlXPathContextPtr ctxt G_GNUC_UNUSED, virBlkioDevice *dev) { xmlNodePtr node; @@ -19730,7 +19731,7 @@ virDomainDefTunablesParse(virDomainDef *def, def->blkio.devices = g_new0(virBlkioDevice, n); for (i = 0; i < n; i++) { - if (virDomainBlkioDeviceParseXML(nodes[i], + if (virDomainBlkioDeviceParseXML(nodes[i], ctxt, &def->blkio.devices[i]) < 0) return -1; def->blkio.ndevices++; -- 2.30.2
Signed-off-by: Kristina Hanicova <khanicov@redhat.com> --- src/conf/domain_conf.c | 116 +++++++++++++++++------------------------ 1 file changed, 48 insertions(+), 68 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index b369c49b05..f658cf49e7 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -1666,85 +1666,65 @@ virBlkioDeviceArrayClear(virBlkioDevice *devices, */ static int virDomainBlkioDeviceParseXML(xmlNodePtr root, - xmlXPathContextPtr ctxt G_GNUC_UNUSED, + xmlXPathContextPtr ctxt, virBlkioDevice *dev) { - xmlNodePtr node; g_autofree char *path = NULL; + g_autofree char *weight = NULL; + g_autofree char *read_bytes_sec = NULL; + g_autofree char *write_bytes_sec = NULL; + g_autofree char *read_iops_sec = NULL; + g_autofree char *write_iops_sec = NULL; + VIR_XPATH_NODE_AUTORESTORE(ctxt) - for (node = root->children; node != NULL; node = node->next) { - g_autofree char *c = NULL; - - if (node->type != XML_ELEMENT_NODE) - continue; - - if (!(c = virXMLNodeContentString(node))) - return -1; - - if (virXMLNodeNameEqual(node, "path")) { - /* To avoid the need for explicit cleanup on failure, - * don't set dev->path until we're assured of - * success. Until then, store it in an autofree pointer. - */ - if (!path) - path = g_steal_pointer(&c); - continue; - } + ctxt->node = root; - if (virXMLNodeNameEqual(node, "weight")) { - if (virStrToLong_ui(c, NULL, 10, &dev->weight) < 0) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, - _("could not parse weight %s"), - c); - return -1; - } - continue; - } + /* To avoid the need for explicit cleanup on failure, + * don't set dev->path until we're assured of + * success. Until then, store it in an autofree pointer. + */ + if (!(path = virXPathString("string(./path)", ctxt))) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("missing per-device path")); + return -1; + } - if (virXMLNodeNameEqual(node, "read_bytes_sec")) { - if (virStrToLong_ull(c, NULL, 10, &dev->rbps) < 0) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, - _("could not parse read bytes sec %s"), - c); - return -1; - } - continue; - } + if ((weight = virXPathString("string(./weight)", ctxt)) && + (virStrToLong_ui(weight, NULL, 10, &dev->weight) < 0)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("could not parse weight %s"), weight); + return -1; + } - if (virXMLNodeNameEqual(node, "write_bytes_sec")) { - if (virStrToLong_ull(c, NULL, 10, &dev->wbps) < 0) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, - _("could not parse write bytes sec %s"), - c); - return -1; - } - continue; - } + if ((read_bytes_sec = virXPathString("string(./read_bytes_sec)", ctxt)) && + (virStrToLong_ull(read_bytes_sec, NULL, 10, &dev->rbps) < 0)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("could not parse read bytes sec %s"), + read_bytes_sec); + return -1; + } - if (virXMLNodeNameEqual(node, "read_iops_sec")) { - if (virStrToLong_ui(c, NULL, 10, &dev->riops) < 0) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, - _("could not parse read iops sec %s"), - c); - return -1; - } - continue; - } + if ((write_bytes_sec = virXPathString("string(./write_bytes_sec)", ctxt)) && + (virStrToLong_ull(write_bytes_sec, NULL, 10, &dev->wbps) < 0)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("could not parse write bytes sec %s"), + write_bytes_sec); + return -1; + } - if (virXMLNodeNameEqual(node, "write_iops_sec")) { - if (virStrToLong_ui(c, NULL, 10, &dev->wiops) < 0) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, - _("could not parse write iops sec %s"), - c); - return -1; - } - continue; - } + if ((read_iops_sec = virXPathString("string(./read_iops_sec)", ctxt)) && + (virStrToLong_ui(read_iops_sec, NULL, 10, &dev->riops) < 0)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("could not parse read iops sec %s"), + read_iops_sec); + return -1; } - if (!path) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", - _("missing per-device path")); + if ((write_iops_sec = virXPathString("string(./write_iops_sec)", ctxt)) && + (virStrToLong_ui(write_iops_sec, NULL, 10, &dev->wiops) < 0)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("could not parse write iops sec %s"), + write_iops_sec); return -1; } -- 2.30.2
Signed-off-by: Kristina Hanicova <khanicov@redhat.com> --- src/conf/domain_conf.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index f658cf49e7..3ca5211a35 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -6841,6 +6841,7 @@ virDomainHostdevSubsysPCIOrigStatesDefParseXML(xmlNodePtr node, static int virDomainHostdevSubsysPCIDefParseXML(xmlNodePtr node, + xmlXPathContextPtr ctxt G_GNUC_UNUSED, virDomainHostdevDef *def, unsigned int flags) { @@ -7390,7 +7391,7 @@ virDomainHostdevDefParseXMLSubsys(xmlNodePtr node, switch (def->source.subsys.type) { case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_PCI: - if (virDomainHostdevSubsysPCIDefParseXML(sourcenode, def, flags) < 0) + if (virDomainHostdevSubsysPCIDefParseXML(sourcenode, ctxt, def, flags) < 0) return -1; backend = VIR_DOMAIN_HOSTDEV_PCI_BACKEND_DEFAULT; -- 2.30.2
Signed-off-by: Kristina Hanicova <khanicov@redhat.com> --- src/conf/domain_conf.c | 37 +++++++++++++------------------------ 1 file changed, 13 insertions(+), 24 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 3ca5211a35..c5d4469b72 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -6841,12 +6841,16 @@ virDomainHostdevSubsysPCIOrigStatesDefParseXML(xmlNodePtr node, static int virDomainHostdevSubsysPCIDefParseXML(xmlNodePtr node, - xmlXPathContextPtr ctxt G_GNUC_UNUSED, + xmlXPathContextPtr ctxt, virDomainHostdevDef *def, unsigned int flags) { g_autofree char *filtering = NULL; - xmlNodePtr cur; + xmlNodePtr address = NULL; + xmlNodePtr origstates = NULL; + VIR_XPATH_NODE_AUTORESTORE(ctxt) + + ctxt->node = node; if ((filtering = virXMLPropString(node, "writeFiltering"))) { int val; @@ -6859,29 +6863,14 @@ virDomainHostdevSubsysPCIDefParseXML(xmlNodePtr node, def->writeFiltering = val; } - cur = node->children; - while (cur != NULL) { - if (cur->type == XML_ELEMENT_NODE) { - if (virXMLNodeNameEqual(cur, "address")) { - virPCIDeviceAddress *addr = - &def->source.subsys.u.pci.addr; + if ((address = virXPathNode("./address", ctxt)) && + virPCIDeviceAddressParseXML(address, &def->source.subsys.u.pci.addr) < 0) + return -1; - if (virPCIDeviceAddressParseXML(cur, addr) < 0) - return -1; - } else if ((flags & VIR_DOMAIN_DEF_PARSE_PCI_ORIG_STATES) && - virXMLNodeNameEqual(cur, "origstates")) { - virDomainHostdevOrigStates *states = &def->origstates; - if (virDomainHostdevSubsysPCIOrigStatesDefParseXML(cur, states) < 0) - return -1; - } else { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("unknown pci source type '%s'"), - cur->name); - return -1; - } - } - cur = cur->next; - } + if ((flags & VIR_DOMAIN_DEF_PARSE_PCI_ORIG_STATES) && + (origstates = virXPathNode("./origstates", ctxt)) && + virDomainHostdevSubsysPCIOrigStatesDefParseXML(origstates, &def->origstates) < 0) + return -1; return 0; } -- 2.30.2
Signed-off-by: Kristina Hanicova <khanicov@redhat.com> --- src/conf/domain_conf.c | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index c5d4469b72..1142b1214a 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -8084,7 +8084,8 @@ virSecurityDeviceLabelDefParseXML(virSecurityDeviceLabelDef ***seclabels_rtn, /* Parse the XML definition for a lease */ static virDomainLeaseDef * -virDomainLeaseDefParseXML(xmlNodePtr node) +virDomainLeaseDefParseXML(xmlNodePtr node, + xmlXPathContextPtr ctxt G_GNUC_UNUSED) { virDomainLeaseDef *def; xmlNodePtr cur; @@ -15508,7 +15509,7 @@ virDomainDeviceDefParse(const char *xmlStr, return NULL; break; case VIR_DOMAIN_DEVICE_LEASE: - if (!(dev->data.lease = virDomainLeaseDefParseXML(node))) + if (!(dev->data.lease = virDomainLeaseDefParseXML(node, ctxt))) return NULL; break; case VIR_DOMAIN_DEVICE_FS: @@ -20276,7 +20277,7 @@ virDomainDefParseXML(xmlDocPtr xml, if (n) def->leases = g_new0(virDomainLeaseDef *, n); for (i = 0; i < n; i++) { - virDomainLeaseDef *lease = virDomainLeaseDefParseXML(nodes[i]); + virDomainLeaseDef *lease = virDomainLeaseDefParseXML(nodes[i], ctxt); if (!lease) goto error; -- 2.30.2
Signed-off-by: Kristina Hanicova <khanicov@redhat.com> --- src/conf/domain_conf.c | 189 ++++++++++++++++++----------------------- 1 file changed, 82 insertions(+), 107 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 1142b1214a..365879ea98 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -8085,48 +8085,34 @@ virSecurityDeviceLabelDefParseXML(virSecurityDeviceLabelDef ***seclabels_rtn, */ static virDomainLeaseDef * virDomainLeaseDefParseXML(xmlNodePtr node, - xmlXPathContextPtr ctxt G_GNUC_UNUSED) + xmlXPathContextPtr ctxt) { virDomainLeaseDef *def; - xmlNodePtr cur; g_autofree char *lockspace = NULL; g_autofree char *key = NULL; g_autofree char *path = NULL; g_autofree char *offset = NULL; + VIR_XPATH_NODE_AUTORESTORE(ctxt) + ctxt->node = node; def = g_new0(virDomainLeaseDef, 1); - cur = node->children; - while (cur != NULL) { - if (cur->type == XML_ELEMENT_NODE) { - if (!key && virXMLNodeNameEqual(cur, "key")) { - if (!(key = virXMLNodeContentString(cur))) - goto error; - } else if (!lockspace && - virXMLNodeNameEqual(cur, "lockspace")) { - if (!(lockspace = virXMLNodeContentString(cur))) - goto error; - } else if (!path && - virXMLNodeNameEqual(cur, "target")) { - path = virXMLPropString(cur, "path"); - offset = virXMLPropString(cur, "offset"); - } - } - cur = cur->next; - } - - if (!key) { + if (!(key = virXPathString("string(./key)", ctxt))) { virReportError(VIR_ERR_XML_ERROR, "%s", _("Missing 'key' element for lease")); goto error; } - if (!path) { + + if (!(lockspace = virXPathString("string(./lockspace)", ctxt))) + goto error; + + if (!(path = virXPathString("string(./target/@path)", ctxt))) { virReportError(VIR_ERR_XML_ERROR, "%s", _("Missing 'target' element for lease")); goto error; } - if (offset && + if ((offset = virXPathString("string(./target/@offset)", ctxt)) && virStrToLong_ull(offset, NULL, 10, &def->offset) < 0) { virReportError(VIR_ERR_XML_ERROR, _("Malformed lease target offset %s"), offset); @@ -9464,9 +9450,8 @@ virDomainControllerDefParseXML(virDomainXMLOption *xmlopt, { g_autoptr(virDomainControllerDef) def = NULL; virDomainControllerType type = 0; - xmlNodePtr cur = NULL; - bool processedModel = false; - bool processedTarget = false; + xmlNodePtr driver = NULL; + xmlNodePtr target = NULL; int numaNode = -1; int ports = -1; VIR_XPATH_NODE_AUTORESTORE(ctxt) @@ -9502,94 +9487,84 @@ virDomainControllerDefParseXML(virDomainXMLOption *xmlopt, def->idx = idxVal; } - cur = node->children; - while (cur != NULL) { - if (cur->type == XML_ELEMENT_NODE) { - if (virXMLNodeNameEqual(cur, "driver")) { - if (virXMLPropUInt(cur, "queues", 10, VIR_XML_PROP_NONE, - &def->queues) < 0) - return NULL; + if ((driver = virXPathNode("./driver", ctxt)) && + (virDomainVirtioOptionsParseXML(driver, &def->virtio) < 0)) + return NULL; - if (virXMLPropUInt(cur, "cmd_per_lun", 10, VIR_XML_PROP_NONE, - &def->cmd_per_lun) < 0) - return NULL; + if (virXMLPropUInt(driver, "queues", 10, VIR_XML_PROP_NONE, + &def->queues) < 0) + return NULL; - if (virXMLPropUInt(cur, "max_sectors", 10, VIR_XML_PROP_NONE, - &def->max_sectors) < 0) - return NULL; + if (virXMLPropUInt(driver, "cmd_per_lun", 10, VIR_XML_PROP_NONE, + &def->cmd_per_lun) < 0) + return NULL; - if (virXMLPropTristateSwitch(cur, "ioeventfd", - VIR_XML_PROP_NONE, - &def->ioeventfd) < 0) - return NULL; + if (virXMLPropUInt(driver, "max_sectors", 10, VIR_XML_PROP_NONE, + &def->max_sectors) < 0) + return NULL; - if (virXMLPropUInt(cur, "iothread", 10, VIR_XML_PROP_NONE, - &def->iothread) < 0) - return NULL; + if (virXMLPropTristateSwitch(driver, "ioeventfd", + VIR_XML_PROP_NONE, + &def->ioeventfd) < 0) + return NULL; - if (virDomainVirtioOptionsParseXML(cur, &def->virtio) < 0) - return NULL; - } else if (virXMLNodeNameEqual(cur, "model")) { - if (processedModel) { - virReportError(VIR_ERR_XML_ERROR, "%s", - _("Multiple <model> elements in " - "controller definition not allowed")); - return NULL; - } + if (virXMLPropUInt(driver, "iothread", 10, VIR_XML_PROP_NONE, + &def->iothread) < 0) + return NULL; - if (def->type == VIR_DOMAIN_CONTROLLER_TYPE_PCI) { - if (virXMLPropEnum(cur, "name", - virDomainControllerPCIModelNameTypeFromString, - VIR_XML_PROP_NONE, - &def->opts.pciopts.modelName) < 0) - return NULL; - } + if (virXPathBoolean("boolean(count(./model) > 1)", ctxt)) { + virReportError(VIR_ERR_XML_ERROR, "%s", + _("Multiple <model> elements in " + "controller definition not allowed")); + return NULL; + } - processedModel = true; - } else if (virXMLNodeNameEqual(cur, "target")) { - if (processedTarget) { - virReportError(VIR_ERR_XML_ERROR, "%s", - _("Multiple <target> elements in " - "controller definition not allowed")); - return NULL; - } - if (def->type == VIR_DOMAIN_CONTROLLER_TYPE_PCI) { - if (virXMLPropInt(cur, "chassisNr", 0, VIR_XML_PROP_NONE, - &def->opts.pciopts.chassisNr) < 0) - return NULL; - - if (virXMLPropInt(cur, "chassis", 0, VIR_XML_PROP_NONE, - &def->opts.pciopts.chassis) < 0) - return NULL; - - if (virXMLPropInt(cur, "port", 0, VIR_XML_PROP_NONE, - &def->opts.pciopts.port) < 0) - return NULL; - - if (virXMLPropInt(cur, "busNr", 0, VIR_XML_PROP_NONE, - &def->opts.pciopts.busNr) < 0) - return NULL; - - if (virXMLPropTristateSwitch(cur, "hotplug", - VIR_XML_PROP_NONE, - &def->opts.pciopts.hotplug) < 0) - return NULL; - - if ((rc = virXMLPropInt(cur, "index", 0, VIR_XML_PROP_NONE, - &def->opts.pciopts.targetIndex)) < 0) - return NULL; - - if ((rc == 1) && def->opts.pciopts.targetIndex == -1) { - virReportError(VIR_ERR_XML_ERROR, - _("Invalid target index '%i' in PCI controller"), - def->opts.pciopts.targetIndex); - } - } + if (def->type == VIR_DOMAIN_CONTROLLER_TYPE_PCI) { + if (virXMLPropEnum(virXPathNode("./model", ctxt), "name", + virDomainControllerPCIModelNameTypeFromString, + VIR_XML_PROP_NONE, + &def->opts.pciopts.modelName) < 0) + return NULL; + } - processedTarget = true; - } - } - cur = cur->next; + if (virXPathBoolean("boolean(count(./target) > 1)", ctxt)) { + virReportError(VIR_ERR_XML_ERROR, "%s", + _("Multiple <target> elements in " + "controller definition not allowed")); + return NULL; + } + + if ((target = virXPathNode("./target", ctxt)) && + def->type == VIR_DOMAIN_CONTROLLER_TYPE_PCI) { + if (virXMLPropInt(target, "chassisNr", 0, VIR_XML_PROP_NONE, + &def->opts.pciopts.chassisNr) < 0) + return NULL; + + if (virXMLPropInt(target, "chassis", 0, VIR_XML_PROP_NONE, + &def->opts.pciopts.chassis) < 0) + return NULL; + + if (virXMLPropInt(target, "port", 0, VIR_XML_PROP_NONE, + &def->opts.pciopts.port) < 0) + return NULL; + + if (virXMLPropInt(target, "busNr", 0, VIR_XML_PROP_NONE, + &def->opts.pciopts.busNr) < 0) + return NULL; + + if (virXMLPropTristateSwitch(target, "hotplug", + VIR_XML_PROP_NONE, + &def->opts.pciopts.hotplug) < 0) + return NULL; + + if ((rc = virXMLPropInt(target, "index", 0, VIR_XML_PROP_NONE, + &def->opts.pciopts.targetIndex)) < 0) + return NULL; + + if ((rc == 1) && def->opts.pciopts.targetIndex == -1) + virReportError(VIR_ERR_XML_ERROR, + _("Invalid target index '%i' in PCI controller"), + def->opts.pciopts.targetIndex); } /* node is parsed differently from target attributes because -- 2.30.2
On 5/4/21 1:40 PM, Kristina Hanicova wrote:
Signed-off-by: Kristina Hanicova <khanicov@redhat.com> --- src/conf/domain_conf.c | 189 ++++++++++++++++++----------------------- 1 file changed, 82 insertions(+), 107 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 1142b1214a..365879ea98 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -8085,48 +8085,34 @@ virSecurityDeviceLabelDefParseXML(virSecurityDeviceLabelDef ***seclabels_rtn, */ static virDomainLeaseDef * virDomainLeaseDefParseXML(xmlNodePtr node, - xmlXPathContextPtr ctxt G_GNUC_UNUSED) + xmlXPathContextPtr ctxt) { virDomainLeaseDef *def; - xmlNodePtr cur; g_autofree char *lockspace = NULL; g_autofree char *key = NULL; g_autofree char *path = NULL; g_autofree char *offset = NULL; + VIR_XPATH_NODE_AUTORESTORE(ctxt)
+ ctxt->node = node; def = g_new0(virDomainLeaseDef, 1);
- cur = node->children; - while (cur != NULL) { - if (cur->type == XML_ELEMENT_NODE) { - if (!key && virXMLNodeNameEqual(cur, "key")) { - if (!(key = virXMLNodeContentString(cur))) - goto error; - } else if (!lockspace && - virXMLNodeNameEqual(cur, "lockspace")) { - if (!(lockspace = virXMLNodeContentString(cur))) - goto error; - } else if (!path && - virXMLNodeNameEqual(cur, "target")) { - path = virXMLPropString(cur, "path"); - offset = virXMLPropString(cur, "offset"); - } - } - cur = cur->next; - } - - if (!key) { + if (!(key = virXPathString("string(./key)", ctxt))) { virReportError(VIR_ERR_XML_ERROR, "%s", _("Missing 'key' element for lease")); goto error; } - if (!path) { + + if (!(lockspace = virXPathString("string(./lockspace)", ctxt))) + goto error; + + if (!(path = virXPathString("string(./target/@path)", ctxt))) { virReportError(VIR_ERR_XML_ERROR, "%s", _("Missing 'target' element for lease")); goto error; }
- if (offset && + if ((offset = virXPathString("string(./target/@offset)", ctxt)) && virStrToLong_ull(offset, NULL, 10, &def->offset) < 0) { virReportError(VIR_ERR_XML_ERROR, _("Malformed lease target offset %s"), offset); @@ -9464,9 +9450,8 @@ virDomainControllerDefParseXML(virDomainXMLOption *xmlopt, { g_autoptr(virDomainControllerDef) def = NULL; virDomainControllerType type = 0; - xmlNodePtr cur = NULL; - bool processedModel = false; - bool processedTarget = false; + xmlNodePtr driver = NULL; + xmlNodePtr target = NULL; int numaNode = -1; int ports = -1; VIR_XPATH_NODE_AUTORESTORE(ctxt)
Looks like you meant to split these? Michal
Signed-off-by: Kristina Hanicova <khanicov@redhat.com> --- src/conf/domain_conf.c | 110 ++++++++++++++++++----------------------- 1 file changed, 49 insertions(+), 61 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 365879ea98..d7bee155e7 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -9714,7 +9714,8 @@ virDomainFSDefParseXML(virDomainXMLOption *xmlopt, { VIR_XPATH_NODE_AUTORESTORE(ctxt) virDomainFSDef *def; - xmlNodePtr cur; + xmlNodePtr driver_node = NULL; + xmlNodePtr source_node = NULL; g_autofree char *type = NULL; g_autofree char *fsdriver = NULL; g_autofree char *source = NULL; @@ -9808,57 +9809,62 @@ virDomainFSDefParseXML(virDomainXMLOption *xmlopt, 1, ULLONG_MAX, false) < 0) goto error; - cur = node->children; - while (cur != NULL) { - if (cur->type == XML_ELEMENT_NODE) { - if (!source && !sock && - virXMLNodeNameEqual(cur, "source")) { - sock = virXMLPropString(cur, "socket"); - if (def->type == VIR_DOMAIN_FS_TYPE_MOUNT || - def->type == VIR_DOMAIN_FS_TYPE_BIND) { - source = virXMLPropString(cur, "dir"); - } else if (def->type == VIR_DOMAIN_FS_TYPE_FILE) { - source = virXMLPropString(cur, "file"); - } else if (def->type == VIR_DOMAIN_FS_TYPE_BLOCK) { - source = virXMLPropString(cur, "dev"); - } else if (def->type == VIR_DOMAIN_FS_TYPE_TEMPLATE) { - source = virXMLPropString(cur, "name"); - } else if (def->type == VIR_DOMAIN_FS_TYPE_RAM) { - usage = virXMLPropString(cur, "usage"); - units = virXMLPropString(cur, "units"); - } else if (def->type == VIR_DOMAIN_FS_TYPE_VOLUME) { - def->src->type = VIR_STORAGE_TYPE_VOLUME; - if (virDomainDiskSourcePoolDefParse(cur, &def->src->srcpool) < 0) - goto error; - } - } else if (!target && - virXMLNodeNameEqual(cur, "target")) { - target = virXMLPropString(cur, "dir"); - } else if (virXMLNodeNameEqual(cur, "readonly")) { - def->readonly = true; - } else if (virXMLNodeNameEqual(cur, "driver")) { - if (!fsdriver) - fsdriver = virXMLPropString(cur, "type"); - if (!wrpolicy) - wrpolicy = virXMLPropString(cur, "wrpolicy"); - if (!format) - format = virXMLPropString(cur, "format"); - - if (virDomainVirtioOptionsParseXML(cur, &def->virtio) < 0) - goto error; - } + if ((source_node = virXPathNode("./source", ctxt))) { + sock = virXMLPropString(source_node, "socket"); + if (def->type == VIR_DOMAIN_FS_TYPE_MOUNT || + def->type == VIR_DOMAIN_FS_TYPE_BIND) { + source = virXMLPropString(source_node, "dir"); + } else if (def->type == VIR_DOMAIN_FS_TYPE_FILE) { + source = virXMLPropString(source_node, "file"); + } else if (def->type == VIR_DOMAIN_FS_TYPE_BLOCK) { + source = virXMLPropString(source_node, "dev"); + } else if (def->type == VIR_DOMAIN_FS_TYPE_TEMPLATE) { + source = virXMLPropString(source_node, "name"); + } else if (def->type == VIR_DOMAIN_FS_TYPE_RAM) { + usage = virXMLPropString(source_node, "usage"); + units = virXMLPropString(source_node, "units"); + } else if (def->type == VIR_DOMAIN_FS_TYPE_VOLUME) { + def->src->type = VIR_STORAGE_TYPE_VOLUME; + if (virDomainDiskSourcePoolDefParse(source_node, &def->src->srcpool) < 0) + goto error; } - cur = cur->next; } - if (fsdriver) { - if ((def->fsdriver = virDomainFSDriverTypeFromString(fsdriver)) <= 0) { + target = virXPathString("string(./target/@dir)", ctxt); + + if (virXPathNode("./readonly", ctxt)) + def->readonly = true; + + if ((driver_node = virXPathNode("./driver", ctxt))) { + if (virDomainVirtioOptionsParseXML(driver_node, &def->virtio) < 0) + goto error; + + if ((fsdriver = virXMLPropString(driver_node, "type")) && + ((def->fsdriver = virDomainFSDriverTypeFromString(fsdriver)) <= 0)) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("unknown fs driver type '%s'"), fsdriver); goto error; } + + if ((wrpolicy = virXMLPropString(driver_node, "wrpolicy"))) { + if ((def->wrpolicy = virDomainFSWrpolicyTypeFromString(wrpolicy)) <= 0) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("unknown filesystem write policy '%s'"), wrpolicy); + goto error; + } + } else { + def->wrpolicy = VIR_DOMAIN_FS_WRPOLICY_DEFAULT; + } + + if ((format = virXMLPropString(driver_node, "format")) && + ((def->format = virStorageFileFormatTypeFromString(format)) <= 0)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("unknown driver format value '%s'"), format); + goto error; + } } + if (def->fsdriver == VIR_DOMAIN_FS_DRIVER_TYPE_VIRTIOFS) { g_autofree char *queue_size = virXPathString("string(./driver/@queue)", ctxt); g_autofree char *binary = virXPathString("string(./binary/@path)", ctxt); @@ -9927,24 +9933,6 @@ virDomainFSDefParseXML(virDomainXMLOption *xmlopt, } } - if (format) { - if ((def->format = virStorageFileFormatTypeFromString(format)) <= 0) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, - _("unknown driver format value '%s'"), format); - goto error; - } - } - - if (wrpolicy) { - if ((def->wrpolicy = virDomainFSWrpolicyTypeFromString(wrpolicy)) <= 0) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, - _("unknown filesystem write policy '%s'"), wrpolicy); - goto error; - } - } else { - def->wrpolicy = VIR_DOMAIN_FS_WRPOLICY_DEFAULT; - } - if (source == NULL && def->type != VIR_DOMAIN_FS_TYPE_RAM && def->type != VIR_DOMAIN_FS_TYPE_VOLUME && !sock) { virReportError(VIR_ERR_NO_SOURCE, -- 2.30.2
Signed-off-by: Kristina Hanicova <khanicov@redhat.com> --- src/conf/domain_conf.c | 340 +++++++++++++++++++---------------------- 1 file changed, 157 insertions(+), 183 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index d7bee155e7..dc72a91d8d 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -10245,7 +10245,13 @@ virDomainNetDefParseXML(virDomainXMLOption *xmlopt, { virDomainNetDef *def; virDomainHostdevDef *hostdev; - xmlNodePtr cur; + xmlNodePtr source_node = NULL; + xmlNodePtr virtualport_node = NULL; + xmlNodePtr driver_node = NULL; + xmlNodePtr filterref_node = NULL; + xmlNodePtr actual_node = NULL; + xmlNodePtr vlan_node = NULL; + xmlNodePtr bandwidth_node = NULL; xmlNodePtr tmpNode; GHashTable *filterparams = NULL; virDomainActualNetDef *actual = NULL; @@ -10288,6 +10294,8 @@ virDomainNetDefParseXML(virDomainXMLOption *xmlopt, g_autofree char *vhostuser_path = NULL; g_autofree char *vhostuser_type = NULL; g_autofree char *vhost_path = NULL; + g_autofree char *tap = NULL; + g_autofree char *vhost = NULL; const char *prefix = xmlopt ? xmlopt->config.netPrefix : NULL; if (!(def = virDomainNetDefNew(xmlopt))) @@ -10306,195 +10314,158 @@ virDomainNetDefParseXML(virDomainXMLOption *xmlopt, &def->trustGuestRxFilters) < 0) goto error; - cur = node->children; - while (cur != NULL) { - if (cur->type == XML_ELEMENT_NODE) { - if (virXMLNodeNameEqual(cur, "source")) { - xmlNodePtr tmpnode = ctxt->node; + if ((source_node = virXPathNode("./source", ctxt))) { + xmlNodePtr tmpnode = ctxt->node; + ctxt->node = source_node; + if (virDomainNetIPInfoParseXML(_("interface host IP"), ctxt, &def->hostIP) < 0) + goto error; + ctxt->node = tmpnode; - ctxt->node = cur; - if (virDomainNetIPInfoParseXML(_("interface host IP"), - ctxt, &def->hostIP) < 0) - goto error; - ctxt->node = tmpnode; - } - if (!macaddr && virXMLNodeNameEqual(cur, "mac")) { - macaddr = virXMLPropString(cur, "address"); - macaddr_type = virXMLPropString(cur, "type"); - macaddr_check = virXMLPropString(cur, "check"); - } else if (!network && - def->type == VIR_DOMAIN_NET_TYPE_NETWORK && - virXMLNodeNameEqual(cur, "source")) { - network = virXMLPropString(cur, "network"); - portgroup = virXMLPropString(cur, "portgroup"); - if (!(flags & VIR_DOMAIN_DEF_PARSE_INACTIVE)) - portid = virXMLPropString(cur, "portid"); - } else if (!internal && - def->type == VIR_DOMAIN_NET_TYPE_INTERNAL && - virXMLNodeNameEqual(cur, "source")) { - internal = virXMLPropString(cur, "name"); - } else if (!bridge && - def->type == VIR_DOMAIN_NET_TYPE_BRIDGE && - virXMLNodeNameEqual(cur, "source")) { - bridge = virXMLPropString(cur, "bridge"); - } else if (!dev && def->type == VIR_DOMAIN_NET_TYPE_DIRECT && - virXMLNodeNameEqual(cur, "source")) { - dev = virXMLPropString(cur, "dev"); - mode = virXMLPropString(cur, "mode"); - } else if (!dev && def->type == VIR_DOMAIN_NET_TYPE_ETHERNET && - virXMLNodeNameEqual(cur, "source")) { - /* This clause is only necessary because from 2010 to - * 2016 it was possible (but never documented) to - * configure the name of the guest-side interface of - * an openvz domain with <source dev='blah'/>. That - * was blatant misuse of <source>, so was likely - * (hopefully) never used, but just in case there was - * somebody using it, we need to generate an error. If - * the openvz driver is ever deprecated, this clause - * can be removed from here. - */ - if ((dev = virXMLPropString(cur, "dev"))) { - virReportError(VIR_ERR_XML_ERROR, - _("Invalid attempt to set <interface type='ethernet'> " - "device name with <source dev='%s'/>. " - "Use <target dev='%s'/> (for host-side) " - "or <guest dev='%s'/> (for guest-side) instead."), - dev, dev, dev); - goto error; - } - } else if (!vhostuser_path && !vhostuser_mode && !vhostuser_type - && def->type == VIR_DOMAIN_NET_TYPE_VHOSTUSER - && virXMLNodeNameEqual(cur, "source")) { - vhostuser_type = virXMLPropString(cur, "type"); - vhostuser_path = virXMLPropString(cur, "path"); - vhostuser_mode = virXMLPropString(cur, "mode"); - if (virDomainChrSourceReconnectDefParseXML(&reconnect, cur, ctxt) < 0) - goto error; + if (def->type == VIR_DOMAIN_NET_TYPE_NETWORK) { + network = virXMLPropString(source_node, "network"); + portgroup = virXMLPropString(source_node, "portgroup"); + if (!(flags & VIR_DOMAIN_DEF_PARSE_INACTIVE)) + portid = virXMLPropString(source_node, "portid"); + } - } else if (!dev - && def->type == VIR_DOMAIN_NET_TYPE_VDPA - && virXMLNodeNameEqual(cur, "source")) { - dev = virXMLPropString(cur, "dev"); - } else if (!def->virtPortProfile - && virXMLNodeNameEqual(cur, "virtualport")) { - if (def->type == VIR_DOMAIN_NET_TYPE_NETWORK) { - if (!(def->virtPortProfile - = virNetDevVPortProfileParse(cur, - VIR_VPORT_XML_GENERATE_MISSING_DEFAULTS))) { - goto error; - } - } else if (def->type == VIR_DOMAIN_NET_TYPE_BRIDGE || - def->type == VIR_DOMAIN_NET_TYPE_DIRECT || - def->type == VIR_DOMAIN_NET_TYPE_HOSTDEV) { - if (!(def->virtPortProfile - = virNetDevVPortProfileParse(cur, - VIR_VPORT_XML_GENERATE_MISSING_DEFAULTS| - VIR_VPORT_XML_REQUIRE_ALL_ATTRIBUTES| - VIR_VPORT_XML_REQUIRE_TYPE))) { - goto error; - } - } else { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, - _("<virtualport> element unsupported for" - " <interface type='%s'>"), - virDomainNetTypeToString(def->type)); - goto error; - } - } else if (!address && - (def->type == VIR_DOMAIN_NET_TYPE_SERVER || - def->type == VIR_DOMAIN_NET_TYPE_CLIENT || - def->type == VIR_DOMAIN_NET_TYPE_MCAST || - def->type == VIR_DOMAIN_NET_TYPE_UDP) && - virXMLNodeNameEqual(cur, "source")) { - address = virXMLPropString(cur, "address"); - port = virXMLPropString(cur, "port"); - if (!localaddr && def->type == VIR_DOMAIN_NET_TYPE_UDP) { - xmlNodePtr tmpnode = ctxt->node; - ctxt->node = cur; - if ((tmpNode = virXPathNode("./local", ctxt))) { - localaddr = virXMLPropString(tmpNode, "address"); - localport = virXMLPropString(tmpNode, "port"); - } - ctxt->node = tmpnode; - } - } else if (!ifname && - virXMLNodeNameEqual(cur, "target")) { - ifname = virXMLPropString(cur, "dev"); - managed_tap = virXMLPropString(cur, "managed"); - } else if ((!ifname_guest || !ifname_guest_actual) && - virXMLNodeNameEqual(cur, "guest")) { - ifname_guest = virXMLPropString(cur, "dev"); - ifname_guest_actual = virXMLPropString(cur, "actual"); - } else if (!linkstate && - virXMLNodeNameEqual(cur, "link")) { - linkstate = virXMLPropString(cur, "state"); - } else if (!script && - virXMLNodeNameEqual(cur, "script")) { - script = virXMLPropString(cur, "path"); - } else if (!downscript && - virXMLNodeNameEqual(cur, "downscript")) { - downscript = virXMLPropString(cur, "path"); - } else if (!domain_name && - virXMLNodeNameEqual(cur, "backenddomain")) { - domain_name = virXMLPropString(cur, "name"); - } else if (virXMLNodeNameEqual(cur, "model")) { - model = virXMLPropString(cur, "type"); - } else if (virXMLNodeNameEqual(cur, "driver")) { - backend = virXMLPropString(cur, "name"); - txmode = virXMLPropString(cur, "txmode"); - ioeventfd = virXMLPropString(cur, "ioeventfd"); - event_idx = virXMLPropString(cur, "event_idx"); - queues = virXMLPropString(cur, "queues"); - rx_queue_size = virXMLPropString(cur, "rx_queue_size"); - tx_queue_size = virXMLPropString(cur, "tx_queue_size"); + if (def->type == VIR_DOMAIN_NET_TYPE_INTERNAL) + internal = virXMLPropString(source_node, "name"); - if (virDomainVirtioOptionsParseXML(cur, &def->virtio) < 0) - goto error; - } else if (virXMLNodeNameEqual(cur, "filterref")) { - if (filter) { - virReportError(VIR_ERR_XML_ERROR, "%s", - _("Invalid specification of multiple <filterref>s " - "in a single <interface>")); - goto error; - } - filter = virXMLPropString(cur, "filter"); - virHashFree(filterparams); - filterparams = virNWFilterParseParamAttributes(cur); - } else if (virXMLNodeNameEqual(cur, "boot")) { - /* boot is parsed as part of virDomainDeviceInfoParseXML */ - } else if (!actual && - (flags & VIR_DOMAIN_DEF_PARSE_ACTUAL_NET) && - def->type == VIR_DOMAIN_NET_TYPE_NETWORK && - virXMLNodeNameEqual(cur, "actual")) { - if (virDomainActualNetDefParseXML(cur, ctxt, def, - &actual, flags, xmlopt) < 0) { - goto error; - } - } else if (virXMLNodeNameEqual(cur, "bandwidth")) { - if (virNetDevBandwidthParse(&def->bandwidth, - NULL, - cur, - def->type == VIR_DOMAIN_NET_TYPE_NETWORK) < 0) - goto error; - } else if (virXMLNodeNameEqual(cur, "vlan")) { - if (virNetDevVlanParse(cur, ctxt, &def->vlan) < 0) - goto error; - } else if (virXMLNodeNameEqual(cur, "backend")) { - char *tmp = NULL; + if (def->type == VIR_DOMAIN_NET_TYPE_BRIDGE) + bridge = virXMLPropString(source_node, "bridge"); - if ((tmp = virXMLPropString(cur, "tap"))) - def->backend.tap = virFileSanitizePath(tmp); - VIR_FREE(tmp); + if (def->type == VIR_DOMAIN_NET_TYPE_DIRECT) { + dev = virXMLPropString(source_node, "dev"); + mode = virXMLPropString(source_node, "mode"); + } - if (!vhost_path && (tmp = virXMLPropString(cur, "vhost"))) - vhost_path = virFileSanitizePath(tmp); - VIR_FREE(tmp); + /* This clause is only necessary because from 2010 to 2016 it was + * possible (but never documented) to configure the name of the + * guest-side interface of an openvz domain with <source dev='blah'/>. + * That was blatant misuse of <source>, so was likely (hopefully) + * never used, but just in case there was somebody using it, we + * need to generate an error. If the openvz driver is ever + * deprecated, this clause can be removed from here. + */ + if (def->type == VIR_DOMAIN_NET_TYPE_ETHERNET && + (dev = virXMLPropString(source_node, "dev"))) { + virReportError(VIR_ERR_XML_ERROR, + _("Invalid attempt to set <interface type='ethernet'> " + "device name with <source dev='%s'/>. " + "Use <target dev='%s'/> (for host-side) " + "or <guest dev='%s'/> (for guest-side) instead."), + dev, dev, dev); + goto error; + } + + if (def->type == VIR_DOMAIN_NET_TYPE_VHOSTUSER) { + vhostuser_type = virXMLPropString(source_node, "type"); + vhostuser_path = virXMLPropString(source_node, "path"); + vhostuser_mode = virXMLPropString(source_node, "mode"); + if (virDomainChrSourceReconnectDefParseXML(&reconnect, source_node, ctxt) < 0) + goto error; + } + + if (def->type == VIR_DOMAIN_NET_TYPE_VDPA) + dev = virXMLPropString(source_node, "dev"); + + if (def->type == VIR_DOMAIN_NET_TYPE_SERVER || + def->type == VIR_DOMAIN_NET_TYPE_CLIENT || + def->type == VIR_DOMAIN_NET_TYPE_MCAST || + def->type == VIR_DOMAIN_NET_TYPE_UDP) { + + address = virXMLPropString(source_node, "address"); + port = virXMLPropString(source_node, "port"); + if (def->type == VIR_DOMAIN_NET_TYPE_UDP) { + xmlNodePtr tmp_node = ctxt->node; + ctxt->node = source_node; + if ((tmpNode = virXPathNode("./local", ctxt))) { + localaddr = virXMLPropString(tmpNode, "address"); + localport = virXMLPropString(tmpNode, "port"); + } + ctxt->node = tmp_node; } } - cur = cur->next; } - if (macaddr) { + if ((virtualport_node = virXPathNode("./virtualport", ctxt))) { + if (def->type == VIR_DOMAIN_NET_TYPE_NETWORK) { + if (!(def->virtPortProfile + = virNetDevVPortProfileParse(virtualport_node, + VIR_VPORT_XML_GENERATE_MISSING_DEFAULTS))) { + goto error; + } + } else if (def->type == VIR_DOMAIN_NET_TYPE_BRIDGE || + def->type == VIR_DOMAIN_NET_TYPE_DIRECT || + def->type == VIR_DOMAIN_NET_TYPE_HOSTDEV) { + if (!(def->virtPortProfile + = virNetDevVPortProfileParse(virtualport_node, VIR_VPORT_XML_GENERATE_MISSING_DEFAULTS| + VIR_VPORT_XML_REQUIRE_ALL_ATTRIBUTES| + VIR_VPORT_XML_REQUIRE_TYPE))) { + goto error; + } + } else { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("<virtualport> element unsupported for" + " <interface type='%s'>"), + virDomainNetTypeToString(def->type)); + goto error; + } + } + + ifname = virXPathString("string(./target/@dev)", ctxt); + managed_tap = virXPathString("string(./target/@managed)", ctxt); + + ifname_guest = virXPathString("string(./guest/@dev)", ctxt); + ifname_guest_actual = virXPathString("string(./guest/@actual)", ctxt); + + linkstate = virXPathString("string(./link/@state)", ctxt); + script = virXPathString("string(./script/@path)", ctxt); + downscript = virXPathString("string(./downscript/@path)", ctxt); + domain_name = virXPathString("string(./backenddomain/@name)", ctxt); + model = virXPathString("string(./model/@type)", ctxt); + + if ((driver_node = virXPathNode("./driver", ctxt)) && + (virDomainVirtioOptionsParseXML(driver_node, &def->virtio) < 0)) + goto error; + + backend = virXMLPropString(driver_node, "name"); + txmode = virXMLPropString(driver_node, "txmode"); + ioeventfd = virXMLPropString(driver_node, "ioeventfd"); + event_idx = virXMLPropString(driver_node, "event_idx"); + queues = virXMLPropString(driver_node, "queues"); + rx_queue_size = virXMLPropString(driver_node, "rx_queue_size"); + tx_queue_size = virXMLPropString(driver_node, "tx_queue_size"); + + if ((filterref_node = virXPathNode("./filterref", ctxt))) { + filter = virXMLPropString(filterref_node, "filter"); + virHashFree(filterparams); + filterparams = virNWFilterParseParamAttributes(filterref_node); + } + + if ((flags & VIR_DOMAIN_DEF_PARSE_ACTUAL_NET) && + def->type == VIR_DOMAIN_NET_TYPE_NETWORK && + (actual_node = virXPathNode("./actual", ctxt)) && + (virDomainActualNetDefParseXML(actual_node, ctxt, def, + &actual, flags, xmlopt) < 0)) + goto error; + + if ((bandwidth_node = virXPathNode("./bandwidth", ctxt)) && + (virNetDevBandwidthParse(&def->bandwidth, NULL, bandwidth_node, + def->type == VIR_DOMAIN_NET_TYPE_NETWORK) < 0)) + goto error; + + if ((vlan_node = virXPathNode("./vlan", ctxt)) && + (virNetDevVlanParse(vlan_node, ctxt, &def->vlan) < 0)) + goto error; + + if ((tap = virXPathString("string(./backend/@tap)", ctxt))) + def->backend.tap = virFileSanitizePath(tap); + + if ((vhost = virXPathString("string(./backend/@vhost)", ctxt))) + vhost_path = virFileSanitizePath(vhost); + + if ((macaddr = virXPathString("string(./mac/@address)", ctxt))) { if (virMacAddrParse((const char *)macaddr, &def->mac) < 0) { virReportError(VIR_ERR_XML_ERROR, _("unable to parse mac address '%s'"), @@ -10512,8 +10483,9 @@ virDomainNetDefParseXML(virDomainXMLOption *xmlopt, def->mac_generated = true; } - if (macaddr_type) { + if ((macaddr_type = virXPathString("string(./mac/@type)", ctxt))) { int tmp; + if ((tmp = virDomainNetMacTypeTypeFromString(macaddr_type)) <= 0) { virReportError(VIR_ERR_XML_ERROR, _("invalid mac address type value: '%s'. Valid " @@ -10523,8 +10495,10 @@ virDomainNetDefParseXML(virDomainXMLOption *xmlopt, } def->mac_type = tmp; } - if (macaddr_check) { + + if ((macaddr_check = virXPathString("string(./mac/@check)", ctxt))) { int tmpCheck; + if ((tmpCheck = virTristateBoolTypeFromString(macaddr_check)) < 0) { virReportError(VIR_ERR_XML_ERROR, _("invalid mac address check value: '%s'"), -- 2.30.2
Signed-off-by: Kristina Hanicova <khanicov@redhat.com> --- src/conf/domain_conf.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index dc72a91d8d..df7079c7e6 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -11063,6 +11063,7 @@ virDomainChrTargetModelFromString(int devtype, static int virDomainChrDefParseTargetXML(virDomainChrDef *def, xmlNodePtr cur, + xmlXPathContextPtr ctxt G_GNUC_UNUSED, unsigned int flags) { xmlNodePtr child; @@ -11602,7 +11603,7 @@ virDomainChrDefParseXML(virDomainXMLOption *xmlopt, if (cur->type == XML_ELEMENT_NODE) { if (virXMLNodeNameEqual(cur, "target")) { seenTarget = true; - if (virDomainChrDefParseTargetXML(def, cur, flags) < 0) + if (virDomainChrDefParseTargetXML(def, cur, ctxt, flags) < 0) goto error; } } -- 2.30.2
Signed-off-by: Kristina Hanicova <khanicov@redhat.com> --- src/conf/domain_conf.c | 15 +++++---------- 1 file changed, 5 insertions(+), 10 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index df7079c7e6..5ac15fe9e8 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -11063,16 +11063,18 @@ virDomainChrTargetModelFromString(int devtype, static int virDomainChrDefParseTargetXML(virDomainChrDef *def, xmlNodePtr cur, - xmlXPathContextPtr ctxt G_GNUC_UNUSED, + xmlXPathContextPtr ctxt, unsigned int flags) { - xmlNodePtr child; unsigned int port; g_autofree char *targetType = virXMLPropString(cur, "type"); g_autofree char *targetModel = NULL; g_autofree char *addrStr = NULL; g_autofree char *portStr = NULL; g_autofree char *stateStr = NULL; + VIR_XPATH_NODE_AUTORESTORE(ctxt) + + ctxt->node = cur; if ((def->targetType = virDomainChrTargetTypeFromString(def->deviceType, @@ -11083,14 +11085,7 @@ virDomainChrDefParseTargetXML(virDomainChrDef *def, return -1; } - child = cur->children; - while (child != NULL) { - if (child->type == XML_ELEMENT_NODE && - virXMLNodeNameEqual(child, "model")) { - targetModel = virXMLPropString(child, "name"); - } - child = child->next; - } + targetModel = virXPathString("string(./model/@name)", ctxt); if ((def->targetModel = virDomainChrTargetModelFromString(def->deviceType, -- 2.30.2
Signed-off-by: Kristina Hanicova <khanicov@redhat.com> --- src/conf/domain_conf.c | 190 ++++++++++++++++++++--------------------- 1 file changed, 94 insertions(+), 96 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 5ac15fe9e8..c5b13783f3 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -11350,112 +11350,112 @@ virDomainChrSourceDefParseXML(virDomainChrSourceDef *def, virDomainChrDef *chr_def, xmlXPathContextPtr ctxt) { - bool logParsed = false; - bool protocolParsed = false; - int sourceParsed = 0; + xmlNodePtr log = NULL; + xmlNodePtr protocol = NULL; + g_autofree xmlNodePtr *source = NULL; + int n = 0; + VIR_XPATH_NODE_AUTORESTORE(ctxt) - for (; cur; cur = cur->next) { - if (cur->type != XML_ELEMENT_NODE) - continue; + ctxt->node = cur; - if (virXMLNodeNameEqual(cur, "source")) { - /* Parse only the first source element since only one is used - * for chardev devices, the only exception is UDP type, where - * user can specify two source elements. */ - if (sourceParsed >= 1 && def->type != VIR_DOMAIN_CHR_TYPE_UDP) { - virReportError(VIR_ERR_XML_ERROR, "%s", - _("only one source element is allowed for " - "character device")); - goto error; - } else if (sourceParsed >= 2) { - virReportError(VIR_ERR_XML_ERROR, "%s", - _("only two source elements are allowed for " - "character device")); - goto error; - } - sourceParsed++; + if ((n = virXPathNodeSet("./source", ctxt, &source)) > 0) { + /* Parse only the first source element since only one is used + * for chardev devices, the only exception is UDP type, where + * user can specify two source elements. */ + if (n > 1 && def->type != VIR_DOMAIN_CHR_TYPE_UDP) { + virReportError(VIR_ERR_XML_ERROR, "%s", + _("only one source element is allowed for " + "character device")); + goto error; + } else if (n > 2) { + virReportError(VIR_ERR_XML_ERROR, "%s", + _("only two source elements are allowed for " + "character device")); + goto error; + } - switch ((virDomainChrType) def->type) { - case VIR_DOMAIN_CHR_TYPE_FILE: - if (virDomainChrSourceDefParseFile(def, cur) < 0) - goto error; - break; + switch ((virDomainChrType) def->type) { + case VIR_DOMAIN_CHR_TYPE_FILE: + if (virDomainChrSourceDefParseFile(def, source[0]) < 0) + goto error; + break; - case VIR_DOMAIN_CHR_TYPE_PTY: - /* PTY path is only parsed from live xml. */ - if (!(flags & VIR_DOMAIN_DEF_PARSE_INACTIVE)) - def->data.file.path = virXMLPropString(cur, "path"); - break; + case VIR_DOMAIN_CHR_TYPE_PTY: + /* PTY path is only parsed from live xml. */ + if (!(flags & VIR_DOMAIN_DEF_PARSE_INACTIVE)) + def->data.file.path = virXPathString("string(./source/@path)", ctxt); + break; - case VIR_DOMAIN_CHR_TYPE_DEV: - case VIR_DOMAIN_CHR_TYPE_PIPE: - def->data.file.path = virXMLPropString(cur, "path"); - break; + case VIR_DOMAIN_CHR_TYPE_DEV: + case VIR_DOMAIN_CHR_TYPE_PIPE: + def->data.file.path = virXPathString("string(./source/@path)", ctxt); + break; - case VIR_DOMAIN_CHR_TYPE_UNIX: - if (virDomainChrSourceDefParseUnix(def, cur, ctxt) < 0) - goto error; - break; + case VIR_DOMAIN_CHR_TYPE_UNIX: + if (virDomainChrSourceDefParseUnix(def, source[0], ctxt) < 0) + goto error; + break; - case VIR_DOMAIN_CHR_TYPE_UDP: - if (virDomainChrSourceDefParseUDP(def, cur) < 0) + case VIR_DOMAIN_CHR_TYPE_UDP: + if ((virDomainChrSourceDefParseUDP(def, source[0]) < 0) || + (source[1] && virDomainChrSourceDefParseUDP(def, source[1]) < 0)) goto error; - break; + break; - case VIR_DOMAIN_CHR_TYPE_TCP: - if (virDomainChrSourceDefParseTCP(def, cur, ctxt, flags) < 0) - goto error; - break; + case VIR_DOMAIN_CHR_TYPE_TCP: + if (virDomainChrSourceDefParseTCP(def, source[0], ctxt, flags) < 0) + goto error; + break; - case VIR_DOMAIN_CHR_TYPE_SPICEPORT: - def->data.spiceport.channel = virXMLPropString(cur, "channel"); - break; + case VIR_DOMAIN_CHR_TYPE_SPICEPORT: + def->data.spiceport.channel = virXPathString("string(./source/@channel)", ctxt); + break; - case VIR_DOMAIN_CHR_TYPE_NMDM: - def->data.nmdm.master = virXMLPropString(cur, "master"); - def->data.nmdm.slave = virXMLPropString(cur, "slave"); - break; + case VIR_DOMAIN_CHR_TYPE_NMDM: + def->data.nmdm.master = virXPathString("string(./source/@master)", ctxt); + def->data.nmdm.slave = virXPathString("string(./source/@slave)", ctxt); + break; - case VIR_DOMAIN_CHR_TYPE_LAST: - case VIR_DOMAIN_CHR_TYPE_NULL: - case VIR_DOMAIN_CHR_TYPE_VC: - case VIR_DOMAIN_CHR_TYPE_STDIO: - case VIR_DOMAIN_CHR_TYPE_SPICEVMC: - break; - } + case VIR_DOMAIN_CHR_TYPE_LAST: + case VIR_DOMAIN_CHR_TYPE_NULL: + case VIR_DOMAIN_CHR_TYPE_VC: + case VIR_DOMAIN_CHR_TYPE_STDIO: + case VIR_DOMAIN_CHR_TYPE_SPICEVMC: + break; + } - /* Check for an optional seclabel override in <source/>. */ - if (chr_def) { - VIR_XPATH_NODE_AUTORESTORE(ctxt) - ctxt->node = cur; - if (virSecurityDeviceLabelDefParseXML(&def->seclabels, - &def->nseclabels, - ctxt, - flags) < 0) { - goto error; - } - } - } else if (virXMLNodeNameEqual(cur, "log")) { - if (logParsed) { - virReportError(VIR_ERR_XML_ERROR, "%s", - _("only one protocol element is allowed for " - "character device")); - goto error; - } - logParsed = true; - if (virDomainChrSourceDefParseLog(def, cur) < 0) - goto error; - } else if (virXMLNodeNameEqual(cur, "protocol")) { - if (protocolParsed) { - virReportError(VIR_ERR_XML_ERROR, "%s", - _("only one log element is allowed for " - "character device")); + /* Check for an optional seclabel override in <source/>. */ + if (chr_def) { + xmlNodePtr tmp = ctxt->node; + ctxt->node = source[0]; + if (virSecurityDeviceLabelDefParseXML(&def->seclabels, &def->nseclabels, + ctxt, flags) < 0) { goto error; } - protocolParsed = true; - if (virDomainChrSourceDefParseProtocol(def, cur) < 0) - goto error; + ctxt->node = tmp; + } + } + + if ((log = virXPathNode("./log", ctxt))) { + if (virXPathBoolean("boolean(count(./log) > 1)", ctxt)) { + virReportError(VIR_ERR_XML_ERROR, "%s", + _("only one log element is allowed for " + "character device")); + goto error; + } + if (virDomainChrSourceDefParseLog(def, log) < 0) + goto error; + } + + if ((protocol = virXPathNode("./protocol", ctxt))) { + if (virXPathBoolean("boolean(count(./protocol) > 1)", ctxt)) { + virReportError(VIR_ERR_XML_ERROR, "%s", + _("only one protocol element is allowed for " + "character device")); + goto error; } + if (virDomainChrSourceDefParseProtocol(def, protocol) < 0) + goto error; } return 0; @@ -11609,7 +11609,7 @@ virDomainChrDefParseXML(virDomainXMLOption *xmlopt, ((def->targetType = virDomainChrDefaultTargetType(def->deviceType)) < 0)) goto error; - if (virDomainChrSourceDefParseXML(def->source, node->children, flags, def, + if (virDomainChrSourceDefParseXML(def->source, node, flags, def, ctxt) < 0) goto error; @@ -11733,7 +11733,7 @@ virDomainSmartcardDefParseXML(virDomainXMLOption *xmlopt, } cur = node->children; - if (virDomainChrSourceDefParseXML(def->data.passthru, cur, flags, + if (virDomainChrSourceDefParseXML(def->data.passthru, node, flags, NULL, ctxt) < 0) return NULL; @@ -13686,7 +13686,7 @@ virDomainRNGDefParseXML(virDomainXMLOption *xmlopt, } if (virDomainChrSourceDefParseXML(def->source.chardev, - backends[0]->children, flags, + backends[0], flags, NULL, ctxt) < 0) goto error; break; @@ -14693,7 +14693,6 @@ virDomainRedirdevDefParseXML(virDomainXMLOption *xmlopt, xmlXPathContextPtr ctxt, unsigned int flags) { - xmlNodePtr cur; virDomainRedirdevDef *def; g_autofree char *bus = NULL; g_autofree char *type = NULL; @@ -14727,10 +14726,9 @@ virDomainRedirdevDefParseXML(virDomainXMLOption *xmlopt, goto error; } - cur = node->children; /* boot gets parsed in virDomainDeviceInfoParseXML * source gets parsed in virDomainChrSourceDefParseXML */ - if (virDomainChrSourceDefParseXML(def->source, cur, flags, + if (virDomainChrSourceDefParseXML(def->source, node, flags, NULL, ctxt) < 0) goto error; -- 2.30.2
On 5/4/21 1:40 PM, Kristina Hanicova wrote:
Signed-off-by: Kristina Hanicova <khanicov@redhat.com> --- src/conf/domain_conf.c | 190 ++++++++++++++++++++--------------------- 1 file changed, 94 insertions(+), 96 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 5ac15fe9e8..c5b13783f3 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -11350,112 +11350,112 @@ virDomainChrSourceDefParseXML(virDomainChrSourceDef *def, virDomainChrDef *chr_def, xmlXPathContextPtr ctxt) { - bool logParsed = false; - bool protocolParsed = false; - int sourceParsed = 0; + xmlNodePtr log = NULL; + xmlNodePtr protocol = NULL; + g_autofree xmlNodePtr *source = NULL; + int n = 0; + VIR_XPATH_NODE_AUTORESTORE(ctxt)
- for (; cur; cur = cur->next) { - if (cur->type != XML_ELEMENT_NODE) - continue; + ctxt->node = cur;
- if (virXMLNodeNameEqual(cur, "source")) { - /* Parse only the first source element since only one is used - * for chardev devices, the only exception is UDP type, where - * user can specify two source elements. */ - if (sourceParsed >= 1 && def->type != VIR_DOMAIN_CHR_TYPE_UDP) { - virReportError(VIR_ERR_XML_ERROR, "%s", - _("only one source element is allowed for " - "character device")); - goto error; - } else if (sourceParsed >= 2) { - virReportError(VIR_ERR_XML_ERROR, "%s", - _("only two source elements are allowed for " - "character device")); - goto error; - } - sourceParsed++; + if ((n = virXPathNodeSet("./source", ctxt, &source)) > 0) { + /* Parse only the first source element since only one is used + * for chardev devices, the only exception is UDP type, where + * user can specify two source elements. */ + if (n > 1 && def->type != VIR_DOMAIN_CHR_TYPE_UDP) { + virReportError(VIR_ERR_XML_ERROR, "%s", + _("only one source element is allowed for " + "character device")); + goto error; + } else if (n > 2) { + virReportError(VIR_ERR_XML_ERROR, "%s", + _("only two source elements are allowed for " + "character device")); + goto error; + }
- switch ((virDomainChrType) def->type) { - case VIR_DOMAIN_CHR_TYPE_FILE: - if (virDomainChrSourceDefParseFile(def, cur) < 0) - goto error; - break; + switch ((virDomainChrType) def->type) { + case VIR_DOMAIN_CHR_TYPE_FILE: + if (virDomainChrSourceDefParseFile(def, source[0]) < 0) + goto error; + break;
- case VIR_DOMAIN_CHR_TYPE_PTY: - /* PTY path is only parsed from live xml. */ - if (!(flags & VIR_DOMAIN_DEF_PARSE_INACTIVE)) - def->data.file.path = virXMLPropString(cur, "path"); - break; + case VIR_DOMAIN_CHR_TYPE_PTY: + /* PTY path is only parsed from live xml. */ + if (!(flags & VIR_DOMAIN_DEF_PARSE_INACTIVE)) + def->data.file.path = virXPathString("string(./source/@path)", ctxt); + break;
- case VIR_DOMAIN_CHR_TYPE_DEV: - case VIR_DOMAIN_CHR_TYPE_PIPE: - def->data.file.path = virXMLPropString(cur, "path"); - break; + case VIR_DOMAIN_CHR_TYPE_DEV: + case VIR_DOMAIN_CHR_TYPE_PIPE: + def->data.file.path = virXPathString("string(./source/@path)", ctxt); + break;
- case VIR_DOMAIN_CHR_TYPE_UNIX: - if (virDomainChrSourceDefParseUnix(def, cur, ctxt) < 0) - goto error; - break; + case VIR_DOMAIN_CHR_TYPE_UNIX: + if (virDomainChrSourceDefParseUnix(def, source[0], ctxt) < 0) + goto error; + break;
- case VIR_DOMAIN_CHR_TYPE_UDP: - if (virDomainChrSourceDefParseUDP(def, cur) < 0) + case VIR_DOMAIN_CHR_TYPE_UDP: + if ((virDomainChrSourceDefParseUDP(def, source[0]) < 0) || + (source[1] && virDomainChrSourceDefParseUDP(def, source[1]) < 0))
Ooops. While this may work by chance, it's not correct. virXPathNodeSet() does not return a NULL terminated array. Therefore, if n == 1, then source[0] is allocated, but source[1] is not. Fortunately, the fix is as easy as s/source[1]/n == 2/.
goto error; - break; + break;
Michal
Signed-off-by: Kristina Hanicova <khanicov@redhat.com> --- src/conf/domain_conf.c | 25 +++++++++---------------- 1 file changed, 9 insertions(+), 16 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index c5b13783f3..5d7fd6794f 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -11566,11 +11566,13 @@ virDomainChrDefParseXML(virDomainXMLOption *xmlopt, xmlNodePtr node, unsigned int flags) { - xmlNodePtr cur; + xmlNodePtr target; const char *nodeName; virDomainChrDef *def; - bool seenTarget = false; g_autofree char *type = NULL; + VIR_XPATH_NODE_AUTORESTORE(ctxt) + + ctxt->node = node; if (!(def = virDomainChrDefNew(xmlopt))) return NULL; @@ -11593,21 +11595,12 @@ virDomainChrDefParseXML(virDomainXMLOption *xmlopt, goto error; } - cur = node->children; - while (cur != NULL) { - if (cur->type == XML_ELEMENT_NODE) { - if (virXMLNodeNameEqual(cur, "target")) { - seenTarget = true; - if (virDomainChrDefParseTargetXML(def, cur, ctxt, flags) < 0) - goto error; - } - } - cur = cur->next; - } - - if (!seenTarget && - ((def->targetType = virDomainChrDefaultTargetType(def->deviceType)) < 0)) + if ((target = virXPathNode("./target", ctxt))) { + if (virDomainChrDefParseTargetXML(def, target, ctxt, flags) < 0) + goto error; + } else if ((def->targetType = virDomainChrDefaultTargetType(def->deviceType)) < 0) { goto error; + } if (virDomainChrSourceDefParseXML(def->source, node, flags, def, ctxt) < 0) -- 2.30.2
Signed-off-by: Kristina Hanicova <khanicov@redhat.com> --- src/conf/domain_conf.c | 59 ++++++++++++++++++------------------------ 1 file changed, 25 insertions(+), 34 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 5d7fd6794f..2511778b15 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -11642,12 +11642,14 @@ virDomainSmartcardDefParseXML(virDomainXMLOption *xmlopt, xmlXPathContextPtr ctxt, unsigned int flags) { - xmlNodePtr cur; g_autoptr(virDomainSmartcardDef) def = NULL; - size_t i; g_autofree char *mode = NULL; g_autofree char *type = NULL; + g_autofree xmlNodePtr *certificates = NULL; + int n = 0; + VIR_XPATH_NODE_AUTORESTORE(ctxt) + ctxt->node = node; def = g_new0(virDomainSmartcardDef, 1); mode = virXMLPropString(node, "mode"); @@ -11668,42 +11670,32 @@ virDomainSmartcardDefParseXML(virDomainXMLOption *xmlopt, break; case VIR_DOMAIN_SMARTCARD_TYPE_HOST_CERTIFICATES: - i = 0; - cur = node->children; - while (cur) { - if (cur->type == XML_ELEMENT_NODE && - virXMLNodeNameEqual(cur, "certificate")) { - if (i == 3) { - virReportError(VIR_ERR_XML_ERROR, "%s", - _("host-certificates mode needs " - "exactly three certificates")); - return NULL; - } - if (!(def->data.cert.file[i] = virXMLNodeContentString(cur))) - return NULL; - - i++; - } else if (cur->type == XML_ELEMENT_NODE && - virXMLNodeNameEqual(cur, "database") && - !def->data.cert.database) { - if (!(def->data.cert.database = virXMLNodeContentString(cur))) - return NULL; - - if (*def->data.cert.database != '/') { - virReportError(VIR_ERR_XML_ERROR, - _("expecting absolute path: %s"), - def->data.cert.database); - return NULL; - } - } - cur = cur->next; - } - if (i < 3) { + n = virXPathNodeSet("./certificate", ctxt, &certificates); + if (n != VIR_DOMAIN_SMARTCARD_NUM_CERTIFICATES) { virReportError(VIR_ERR_XML_ERROR, "%s", _("host-certificates mode needs " "exactly three certificates")); return NULL; } + + if (!(def->data.cert.file[0] = virXMLNodeContentString(certificates[0])) || + !(def->data.cert.file[1] = virXMLNodeContentString(certificates[1])) || + !(def->data.cert.file[2] = virXMLNodeContentString(certificates[2]))) + return NULL; + + if (virXPathNode("./database", ctxt) && + !def->data.cert.database) { + if (!(def->data.cert.database = + virXPathString("string(./database/text())", ctxt))) + return NULL; + + if (*def->data.cert.database != '/') { + virReportError(VIR_ERR_XML_ERROR, + _("expecting absolute path: %s"), + def->data.cert.database); + return NULL; + } + } break; case VIR_DOMAIN_SMARTCARD_TYPE_PASSTHROUGH: @@ -11725,7 +11717,6 @@ virDomainSmartcardDefParseXML(virDomainXMLOption *xmlopt, return NULL; } - cur = node->children; if (virDomainChrSourceDefParseXML(def->data.passthru, node, flags, NULL, ctxt) < 0) return NULL; -- 2.30.2
Signed-off-by: Kristina Hanicova <khanicov@redhat.com> --- src/conf/domain_conf.c | 303 ++++++++++++++++------------------------- 1 file changed, 114 insertions(+), 189 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 2511778b15..206816d76f 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -12690,12 +12690,29 @@ virDomainGraphicsDefParseXMLSpice(virDomainGraphicsDef *def, xmlXPathContextPtr ctxt, unsigned int flags) { - xmlNodePtr cur; int defaultModeVal; + int nameval; + g_autofree xmlNodePtr *node_list = NULL; + int n = 0; + size_t i = 0; + int value = 0; g_autofree char *port = virXMLPropString(node, "port"); g_autofree char *tlsPort = virXMLPropString(node, "tlsPort"); g_autofree char *autoport = virXMLPropString(node, "autoport"); g_autofree char *defaultMode = virXMLPropString(node, "defaultMode"); + g_autofree char *compression = NULL; + g_autofree char *jpeg_compression = NULL; + g_autofree char *zlib_compression = NULL; + g_autofree char *playback_compression = NULL; + g_autofree char *streaming_mode = NULL; + g_autofree char *copypaste = NULL; + g_autofree char *filetransfer_enable = NULL; + g_autofree char *gl_enable = NULL; + g_autofree char *mouse_mode = NULL; + g_autofree char *rendernode = NULL; + VIR_XPATH_NODE_AUTORESTORE(ctxt) + + ctxt->node = node; if (virDomainGraphicsListensParseXML(def, node, ctxt, flags) < 0) return -1; @@ -12751,207 +12768,115 @@ virDomainGraphicsDefParseXMLSpice(virDomainGraphicsDef *def, def->type) < 0) return -1; - cur = node->children; - while (cur != NULL) { - if (cur->type == XML_ELEMENT_NODE) { - if (virXMLNodeNameEqual(cur, "channel")) { - int nameval, modeval; - g_autofree char *name = NULL; - g_autofree char *mode = NULL; - - name = virXMLPropString(cur, "name"); - mode = virXMLPropString(cur, "mode"); - - if (!name || !mode) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("spice channel missing name/mode")); - return -1; - } - - if ((nameval = virDomainGraphicsSpiceChannelNameTypeFromString(name)) < 0) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, - _("unknown spice channel name %s"), - name); - return -1; - } - if ((modeval = virDomainGraphicsSpiceChannelModeTypeFromString(mode)) < 0) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, - _("unknown spice channel mode %s"), - mode); - return -1; - } - - def->data.spice.channels[nameval] = modeval; - } else if (virXMLNodeNameEqual(cur, "image")) { - int compressionVal; - g_autofree char *compression = virXMLPropString(cur, "compression"); - - if (!compression) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("spice image missing compression")); - return -1; - } - - if ((compressionVal = - virDomainGraphicsSpiceImageCompressionTypeFromString(compression)) <= 0) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, - _("unknown spice image compression %s"), - compression); - return -1; - } - - def->data.spice.image = compressionVal; - } else if (virXMLNodeNameEqual(cur, "jpeg")) { - int compressionVal; - g_autofree char *compression = virXMLPropString(cur, "compression"); - - if (!compression) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("spice jpeg missing compression")); - return -1; - } - - if ((compressionVal = - virDomainGraphicsSpiceJpegCompressionTypeFromString(compression)) <= 0) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, - _("unknown spice jpeg compression %s"), - compression); - return -1; - } - - def->data.spice.jpeg = compressionVal; - } else if (virXMLNodeNameEqual(cur, "zlib")) { - int compressionVal; - g_autofree char *compression = virXMLPropString(cur, "compression"); - - if (!compression) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("spice zlib missing compression")); - return -1; - } - - if ((compressionVal = - virDomainGraphicsSpiceZlibCompressionTypeFromString(compression)) <= 0) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, - _("unknown spice zlib compression %s"), - compression); - return -1; - } - - def->data.spice.zlib = compressionVal; - } else if (virXMLNodeNameEqual(cur, "playback")) { - int compressionVal; - g_autofree char *compression = virXMLPropString(cur, "compression"); - - if (!compression) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("spice playback missing compression")); - return -1; - } - - if ((compressionVal = - virTristateSwitchTypeFromString(compression)) <= 0) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", - _("unknown spice playback compression")); - return -1; - } - - def->data.spice.playback = compressionVal; - } else if (virXMLNodeNameEqual(cur, "streaming")) { - int modeVal; - g_autofree char *mode = virXMLPropString(cur, "mode"); - - if (!mode) { - virReportError(VIR_ERR_XML_ERROR, "%s", - _("spice streaming missing mode")); - return -1; - } - if ((modeVal = - virDomainGraphicsSpiceStreamingModeTypeFromString(mode)) <= 0) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", - _("unknown spice streaming mode")); - return -1; - } - - def->data.spice.streaming = modeVal; - } else if (virXMLNodeNameEqual(cur, "clipboard")) { - int copypasteVal; - g_autofree char *copypaste = virXMLPropString(cur, "copypaste"); - - if (!copypaste) { - virReportError(VIR_ERR_XML_ERROR, "%s", - _("spice clipboard missing copypaste")); - return -1; - } + if ((n = virXPathNodeSet("./channel", ctxt, &node_list)) < 0) + return -1; - if ((copypasteVal = - virTristateBoolTypeFromString(copypaste)) <= 0) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, - _("unknown copypaste value '%s'"), copypaste); - return -1; - } + for (i = 0; i < n; i++) { + g_autofree char *name = virXMLPropString(node_list[i], "name"); + g_autofree char *mode = virXMLPropString(node_list[i], "mode"); - def->data.spice.copypaste = copypasteVal; - } else if (virXMLNodeNameEqual(cur, "filetransfer")) { - int enableVal; - g_autofree char *enable = virXMLPropString(cur, "enable"); + if ((nameval = virDomainGraphicsSpiceChannelNameTypeFromString(name)) < 0) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("unknown spice channel name %s"), NULLSTR(name)); + return -1; + } + if ((value = virDomainGraphicsSpiceChannelModeTypeFromString(mode)) < 0) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("unknown spice channel mode %s"), NULLSTR(mode)); + return -1; + } + def->data.spice.channels[nameval] = value; + } - if (!enable) { - virReportError(VIR_ERR_XML_ERROR, "%s", - _("spice filetransfer missing enable")); - return -1; - } + if ((compression = virXPathString("string(./image/@compression)", ctxt))) { + if ((value = + virDomainGraphicsSpiceImageCompressionTypeFromString(compression)) <= 0) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("unknown spice image compression %s"), compression); + return -1; + } + def->data.spice.image = value; + } - if ((enableVal = - virTristateBoolTypeFromString(enable)) <= 0) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, - _("unknown enable value '%s'"), enable); - return -1; - } + if ((jpeg_compression = virXPathString("string(./jpeg/@compression)", ctxt))) { + if ((value = + virDomainGraphicsSpiceJpegCompressionTypeFromString(jpeg_compression)) <= 0) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("unknown spice jpeg compression %s"), + jpeg_compression); + return -1; + } + def->data.spice.jpeg = value; + } - def->data.spice.filetransfer = enableVal; - } else if (virXMLNodeNameEqual(cur, "gl")) { - int enableVal; - g_autofree char *enable = virXMLPropString(cur, "enable"); - g_autofree char *rendernode = virXMLPropString(cur, "rendernode"); + if ((zlib_compression = virXPathString("string(./zlib/@compression)", ctxt))) { + if ((value = + virDomainGraphicsSpiceZlibCompressionTypeFromString(zlib_compression)) <= 0) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("unknown spice zlib compression %s"), + zlib_compression); + return -1; + } + def->data.spice.zlib = value; + } - if (!enable) { - virReportError(VIR_ERR_XML_ERROR, "%s", - _("spice gl element missing enable")); - return -1; - } + if ((playback_compression = virXPathString("string(./playback/@compression)", ctxt))) { + if ((value = virTristateSwitchTypeFromString(playback_compression)) <= 0) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("unknown spice playback compression")); + return -1; + } + def->data.spice.playback = value; + } - if ((enableVal = - virTristateBoolTypeFromString(enable)) <= 0) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, - _("unknown enable value '%s'"), enable); - return -1; - } + if ((streaming_mode = virXPathString("string(./streaming/@mode)", ctxt))) { + if ((value = + virDomainGraphicsSpiceStreamingModeTypeFromString(streaming_mode)) <= 0) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("unknown spice streaming mode")); + return -1; + } + def->data.spice.streaming = value; + } - def->data.spice.gl = enableVal; - def->data.spice.rendernode = g_steal_pointer(&rendernode); + if ((copypaste = virXPathString("string(./clipboard/@copypaste)", ctxt))) { + if ((value = virTristateBoolTypeFromString(copypaste)) <= 0) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("unknown copypaste value '%s'"), copypaste); + return -1; + } + def->data.spice.copypaste = value; + } - } else if (virXMLNodeNameEqual(cur, "mouse")) { - int modeVal; - g_autofree char *mode = virXMLPropString(cur, "mode"); + if ((filetransfer_enable = virXPathString("string(./filetransfer/@enable)", ctxt))) { + if ((value = virTristateBoolTypeFromString(filetransfer_enable)) <= 0) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("unknown enable value '%s'"), + filetransfer_enable); + return -1; + } + def->data.spice.filetransfer = value; + } - if (!mode) { - virReportError(VIR_ERR_XML_ERROR, "%s", - _("spice mouse missing mode")); - return -1; - } + if ((gl_enable = virXPathString("string(./gl/@enable)", ctxt))) { + rendernode = virXPathString("string(./gl/@rendernode)", ctxt); - if ((modeVal = virDomainGraphicsSpiceMouseModeTypeFromString(mode)) <= 0) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, - _("unknown mouse mode value '%s'"), - mode); - return -1; - } + if ((value = virTristateBoolTypeFromString(gl_enable)) <= 0) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("unknown enable value '%s'"), gl_enable); + return -1; + } + def->data.spice.gl = value; + def->data.spice.rendernode = g_steal_pointer(&rendernode); + } - def->data.spice.mousemode = modeVal; - } + if ((mouse_mode = virXPathString("string(./mouse/@mode)", ctxt))) { + if ((value = virDomainGraphicsSpiceMouseModeTypeFromString(mouse_mode)) <= 0) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("unknown mouse mode value '%s'"), mouse_mode); + return -1; } - cur = cur->next; + def->data.spice.mousemode = value; } return 0; -- 2.30.2
On Tue, May 04, 2021 at 01:40:10PM +0200, Kristina Hanicova wrote:
- } else if (virXMLNodeNameEqual(cur, "playback")) { - int compressionVal; - g_autofree char *compression = virXMLPropString(cur, "compression"); - - if (!compression) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("spice playback missing compression")); - return -1; - } - - if ((compressionVal = - virTristateSwitchTypeFromString(compression)) <= 0) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", - _("unknown spice playback compression")); - return -1; - } - - def->data.spice.playback = compressionVal; + if ((playback_compression = virXPathString("string(./playback/@compression)", ctxt))) { + if ((value = virTristateSwitchTypeFromString(playback_compression)) <= 0) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("unknown spice playback compression")); + return -1; + } + def->data.spice.playback = value; + }
Apologies for the thread necromancy :) If I'm reading the change above correctly, then the presence of the compression property of the <playback> element has gone from being required, with an error being raised and the function terminating early if it's not there, to being parsed if found and ignored otherwise. Tim later restored the original behavior in commit bb94b3d28db909d43d83b3f2ab73aa3f881b5c95 Author: Tim Wiederhake <twiederh@redhat.com> Date: Wed May 5 12:55:48 2021 +0200 virDomainGraphicsDefParseXMLSpice: Use virXMLProp* Signed-off-by: Tim Wiederhake <twiederh@redhat.com> Reviewed-by: Michal Privoznik <mprivozn@redhat.com> with the hunk
- if ((playback_compression = virXPathString("string(./playback/@compression)", ctxt))) { - if ((value = virTristateSwitchTypeFromString(playback_compression)) <= 0) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", - _("unknown spice playback compression")); + if ((cur = virXPathNode("./playback", ctxt))) { + if (virXMLPropTristateSwitch(cur, "compression", + VIR_XML_PROP_REQUIRED, + &def->data.spice.playback) < 0) return -1; - } - def->data.spice.playback = value; }
and specifically the VIR_XML_PROP_REQUIRED bit, but I can't help wondering if there are other cases where switching to virXPathString() in the way seen here might have introduced undesired changes in behavior. Thoughts? -- Andrea Bolognani / Red Hat / Virtualization
On 3/24/22 18:02, Andrea Bolognani wrote:
but I can't help wondering if there are other cases where switching to virXPathString() in the way seen here might have introduced undesired changes in behavior.
Thoughts?
Unless you want to audit every virXPath*() call with its history, I say we're good as we didn't get any bug reports. It is unfortunate, but the master was broken only for one day. Michal
On Thu, Mar 24, 2022 at 07:16:28PM +0100, Michal Prívozník wrote:
On 3/24/22 18:02, Andrea Bolognani wrote:
but I can't help wondering if there are other cases where switching to virXPathString() in the way seen here might have introduced undesired changes in behavior.
Thoughts?
Unless you want to audit every virXPath*() call with its history, I say we're good as we didn't get any bug reports. It is unfortunate, but the master was broken only for one day.
I'm not concerned about the brief period of time between Kristina's and Tim's patches were merged, but about any subtle changes in behavior that might still be lingering even after a year. That said, I have just finished auditing all uses of virXMLPropTristate*() and I don't think I can find the strength in me to go through the same process again for virXPath*() - at least not right away. So let's just hope that the lack of bug reports is an accurate indicator of the lack of significant regressions, I guess :) -- Andrea Bolognani / Red Hat / Virtualization
Signed-off-by: Kristina Hanicova <khanicov@redhat.com> --- src/conf/domain_conf.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 206816d76f..97cf940dad 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -14348,7 +14348,8 @@ virDomainVideoResolutionDefParseXML(xmlNodePtr node) } static virDomainVideoDriverDef * -virDomainVideoDriverDefParseXML(xmlNodePtr node) +virDomainVideoDriverDefParseXML(xmlNodePtr node, + xmlXPathContextPtr ctxt G_GNUC_UNUSED) { xmlNodePtr cur; virDomainVideoDriverDef *def; @@ -14513,7 +14514,7 @@ virDomainVideoDefParseXML(virDomainXMLOption *xmlopt, if (virDomainDeviceInfoParseXML(xmlopt, node, ctxt, &def->info, flags) < 0) return NULL; - def->driver = virDomainVideoDriverDefParseXML(node); + def->driver = virDomainVideoDriverDefParseXML(node, ctxt); return g_steal_pointer(&def); } -- 2.30.2
Signed-off-by: Kristina Hanicova <khanicov@redhat.com> --- src/conf/domain_conf.c | 17 ++++------------- 1 file changed, 4 insertions(+), 13 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 97cf940dad..f90f720b44 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -14349,25 +14349,16 @@ virDomainVideoResolutionDefParseXML(xmlNodePtr node) static virDomainVideoDriverDef * virDomainVideoDriverDefParseXML(xmlNodePtr node, - xmlXPathContextPtr ctxt G_GNUC_UNUSED) + xmlXPathContextPtr ctxt) { - xmlNodePtr cur; virDomainVideoDriverDef *def; int val; g_autofree char *vgaconf = NULL; + VIR_XPATH_NODE_AUTORESTORE(ctxt) - cur = node->children; - while (cur != NULL) { - if (cur->type == XML_ELEMENT_NODE) { - if (!vgaconf && - virXMLNodeNameEqual(cur, "driver")) { - vgaconf = virXMLPropString(cur, "vgaconf"); - } - } - cur = cur->next; - } + ctxt->node = node; - if (!vgaconf) + if (!(vgaconf = virXPathString("string(./driver/@vgaconf)", ctxt))) return NULL; def = g_new0(virDomainVideoDriverDef, 1); -- 2.30.2
Signed-off-by: Kristina Hanicova <khanicov@redhat.com> --- src/conf/domain_conf.c | 88 ++++++++++++++++-------------------------- 1 file changed, 33 insertions(+), 55 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index f90f720b44..990c6be309 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -14380,7 +14380,9 @@ virDomainVideoDefParseXML(virDomainXMLOption *xmlopt, unsigned int flags) { g_autoptr(virDomainVideoDef) def = NULL; - xmlNodePtr cur; + xmlNodePtr driver; + xmlNodePtr accel_node; + xmlNodePtr res_node; VIR_XPATH_NODE_AUTORESTORE(ctxt) g_autofree char *type = NULL; g_autofree char *driver_name = NULL; @@ -14396,62 +14398,28 @@ virDomainVideoDefParseXML(virDomainXMLOption *xmlopt, ctxt->node = node; - cur = node->children; - while (cur != NULL) { - if (cur->type == XML_ELEMENT_NODE) { - if (!type && !vram && !ram && !heads && - virXMLNodeNameEqual(cur, "model")) { - xmlNodePtr child; - type = virXMLPropString(cur, "type"); - ram = virXMLPropString(cur, "ram"); - vram = virXMLPropString(cur, "vram"); - vram64 = virXMLPropString(cur, "vram64"); - vgamem = virXMLPropString(cur, "vgamem"); - heads = virXMLPropString(cur, "heads"); - - if ((primary = virXMLPropString(cur, "primary")) != NULL) { - ignore_value(virStringParseYesNo(primary, &def->primary)); - VIR_FREE(primary); - } + if ((primary = virXPathString("string(./model/@primary)", ctxt)) != NULL) + ignore_value(virStringParseYesNo(primary, &def->primary)); - child = cur->children; - while (child != NULL) { - if (child->type == XML_ELEMENT_NODE) { - if (def->accel == NULL && - virXMLNodeNameEqual(child, "acceleration")) { - if ((def->accel = virDomainVideoAccelDefParseXML(child)) == NULL) - return NULL; - } - if (def->res == NULL && - virXMLNodeNameEqual(child, "resolution")) { - if ((def->res = virDomainVideoResolutionDefParseXML(child)) == NULL) - return NULL; - } - } - child = child->next; - } - } - if (virXMLNodeNameEqual(cur, "driver")) { - if (virDomainVirtioOptionsParseXML(cur, &def->virtio) < 0) - return NULL; - driver_name = virXMLPropString(cur, "name"); - } - } - cur = cur->next; - } + if (def->accel == NULL && + (accel_node = virXPathNode("./model/acceleration", ctxt))) + if ((def->accel = virDomainVideoAccelDefParseXML(accel_node)) == NULL) + return NULL; - if (type) { - if ((def->type = virDomainVideoTypeFromString(type)) < 0) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, - _("unknown video model '%s'"), type); + if (def->res == NULL && + (res_node = virXPathNode("./model/resolution", ctxt))) + if ((def->res = virDomainVideoResolutionDefParseXML(res_node)) == NULL) return NULL; - } - } else { - def->type = VIR_DOMAIN_VIDEO_TYPE_DEFAULT; + + if ((driver = virXPathNode("./driver", ctxt))) { + if (virDomainVirtioOptionsParseXML(driver, &def->virtio) < 0) + return NULL; + driver_name = virXPathString("string(./driver/@name)", ctxt); } if (driver_name) { int backend; + if ((backend = virDomainVideoBackendTypeFromString(driver_name)) <= 0) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("unknown video driver '%s'"), driver_name); @@ -14462,7 +14430,17 @@ virDomainVideoDefParseXML(virDomainXMLOption *xmlopt, def->backend = VIR_DOMAIN_VIDEO_BACKEND_TYPE_DEFAULT; } - if (ram) { + if ((type = virXPathString("string(./model/@type)", ctxt))) { + if ((def->type = virDomainVideoTypeFromString(type)) < 0) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("unknown video model '%s'"), type); + return NULL; + } + } else { + def->type = VIR_DOMAIN_VIDEO_TYPE_DEFAULT; + } + + if ((ram = virXPathString("string(./model/@ram)", ctxt))) { if (virStrToLong_uip(ram, NULL, 10, &def->ram) < 0) { virReportError(VIR_ERR_XML_ERROR, _("cannot parse video ram '%s'"), ram); @@ -14470,7 +14448,7 @@ virDomainVideoDefParseXML(virDomainXMLOption *xmlopt, } } - if (vram) { + if ((vram = virXPathString("string(./model/@vram)", ctxt))) { if (virStrToLong_uip(vram, NULL, 10, &def->vram) < 0) { virReportError(VIR_ERR_XML_ERROR, _("cannot parse video vram '%s'"), vram); @@ -14478,7 +14456,7 @@ virDomainVideoDefParseXML(virDomainXMLOption *xmlopt, } } - if (vram64) { + if ((vram64 = virXPathString("string(./model/@vram64)", ctxt))) { if (virStrToLong_uip(vram64, NULL, 10, &def->vram64) < 0) { virReportError(VIR_ERR_XML_ERROR, _("cannot parse video vram64 '%s'"), vram64); @@ -14486,7 +14464,7 @@ virDomainVideoDefParseXML(virDomainXMLOption *xmlopt, } } - if (vgamem) { + if ((vgamem = virXPathString("string(./model/@vgamem)", ctxt))) { if (virStrToLong_uip(vgamem, NULL, 10, &def->vgamem) < 0) { virReportError(VIR_ERR_XML_ERROR, _("cannot parse video vgamem '%s'"), vgamem); @@ -14494,7 +14472,7 @@ virDomainVideoDefParseXML(virDomainXMLOption *xmlopt, } } - if (heads) { + if ((heads = virXPathString("string(./model/@heads)", ctxt))) { if (virStrToLong_uip(heads, NULL, 10, &def->heads) < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, _("cannot parse video heads '%s'"), heads); -- 2.30.2
On 5/4/21 1:39 PM, Kristina Hanicova wrote:
This is v2 from https://listman.redhat.com/archives/libvir-list/2021-April/msg00617.html
Changes since v1: - rebase to the lastest git master
This series reworks the outdated way of parsing XML to parsing by XPath, which is more obvious and saves a few lines of code.
Kristina Hanicova (19): conf: Propagate xmlXPathContextPtr into virDomainHostdevSubsysUSBDefParseXML() Refactoring virDomainHostdevSubsysUSBDefParseXML() to use XPath conf: Propagate xmlXPathContextPtr into virDomainBlkioDeviceParseXML() Refactoring virDomainBlkioDeviceParseXML() to use XPath conf: Propagate xmlXPathContextPtr into virDomainHostdevSubsysPCIDefParseXML() Refactoring virDomainHostdevSubsysPCIDefParseXML() to use XPath conf: Propagate xmlXPathContextPtr into virDomainLeaseDefParseXML() Refactoring virDomainLeaseDefParseXML() to use XPath Refactoring virDomainFSDefParseXML() to use XPath Refactoring virDomainNetDefParseXML() to use XPath conf: Propagate xmlXPathContextPtr into virDomainChrDefParseTargetXML() Refactoring virDomainChrDefParseTargetXML() to use XPath Refactoring virDomainChrSourceDefParseXML() to use XPath Refactoring virDomainChrDefParseXML() to use XPath Refactoring virDomainSmartcardDefParseXML() to use XPath Refactoring virDomainGraphicsDefParseXMLSpice() to use XPath conf: Propagate xmlXPathContextPtr into virDomainVideoDriverDefParseXML() Refactoring virDomainVideoDriverDefParseXML() to use XPath Refactoring virDomainVideoDefParseXML() to use XPath
src/conf/domain_conf.c | 1641 +++++++++++++++++----------------------- 1 file changed, 707 insertions(+), 934 deletions(-)
Reviewed-by: Michal Privoznik <mprivozn@redhat.com> and pushed. Michal
participants (3)
-
Andrea Bolognani -
Kristina Hanicova -
Michal Prívozník