[PATCH 00/10] virDomainControllerDef(Validate|Parse|Format): Refactor and fix several problems
A relatively simple-looking bug report (see 10/10) lead me looking and finding few more problems. Peter Krempa (10): virDomainControllerDefValidate: Un-break lines in error messages virDomainControllerDefFormat: Use proper type for enum virDomainControllerDefFormat: Split out formatting of PCI controller virDomainControllerDefFormatPCI: Refactor formatting of '<target>' subelement virDomainControllerDefParseXML: Fix broken code indentation when parsing PCI contoller target virDomainControllerDefParseXML: Fix broken error reporting when parsing 'index' virDomainControllerDefParseXML: Remove explicit checks for negative value virDomainControllerDefParseXML: Parse 'index' by virXMLPropInt virDomainControllerDefParseXML: Return early if there's unexpectedly many elements virDomainControllerDefParseXML: Reject '-1' for PCI controller target properties src/conf/domain_conf.c | 286 ++++++++---------- src/conf/domain_validate.c | 24 +- .../pci-bridge-negative-index-invalid.err | 2 +- 3 files changed, 132 insertions(+), 180 deletions(-) -- 2.37.1
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/conf/domain_validate.c | 24 ++++++++---------------- 1 file changed, 8 insertions(+), 16 deletions(-) diff --git a/src/conf/domain_validate.c b/src/conf/domain_validate.c index 1c78a3d31c..6ecf6d1c11 100644 --- a/src/conf/domain_validate.c +++ b/src/conf/domain_validate.c @@ -1129,8 +1129,7 @@ virDomainControllerDefValidate(const virDomainControllerDef *controller) controller->model == VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT) { if (controller->info.type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", - _("pci-root and pcie-root controllers " - "should not have an address")); + _("pci-root and pcie-root controllers should not have an address")); return -1; } } @@ -1147,8 +1146,7 @@ virDomainControllerDefValidate(const virDomainControllerDef *controller) if (opts->targetIndex < 0 || opts->targetIndex > 30) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, - _("PCI controller target index '%d' out of " - "range - must be 0-30"), + _("PCI controller target index '%d' out of range - must be 0-30"), opts->targetIndex); return -1; } @@ -1156,8 +1154,7 @@ virDomainControllerDefValidate(const virDomainControllerDef *controller) if ((controller->idx == 0 && opts->targetIndex != 0) || (controller->idx != 0 && opts->targetIndex == 0)) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", - _("Only the PCI controller with index 0 can " - "have target index 0, and vice versa")); + _("Only the PCI controller with index 0 can have target index 0, and vice versa")); return -1; } } @@ -1165,8 +1162,7 @@ virDomainControllerDefValidate(const virDomainControllerDef *controller) if (opts->chassisNr != -1) { if (opts->chassisNr < 1 || opts->chassisNr > 255) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, - _("PCI controller chassisNr '%d' out of range " - "- must be 1-255"), + _("PCI controller chassisNr '%d' out of range - must be 1-255"), opts->chassisNr); return -1; } @@ -1175,8 +1171,7 @@ virDomainControllerDefValidate(const virDomainControllerDef *controller) if (opts->chassis != -1) { if (opts->chassis < 0 || opts->chassis > 255) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, - _("PCI controller chassis '%d' out of range " - "- must be 0-255"), + _("PCI controller chassis '%d' out of range - must be 0-255"), opts->chassis); return -1; } @@ -1185,8 +1180,7 @@ virDomainControllerDefValidate(const virDomainControllerDef *controller) if (opts->port != -1) { if (opts->port < 0 || opts->port > 255) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, - _("PCI controller port '%d' out of range " - "- must be 0-255"), + _("PCI controller port '%d' out of range - must be 0-255"), opts->port); return -1; } @@ -1195,8 +1189,7 @@ virDomainControllerDefValidate(const virDomainControllerDef *controller) if (opts->busNr != -1) { if (opts->busNr < 1 || opts->busNr > 254) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, - _("PCI controller busNr '%d' out of range " - "- must be 1-254"), + _("PCI controller busNr '%d' out of range - must be 1-254"), opts->busNr); return -1; } @@ -1204,8 +1197,7 @@ virDomainControllerDefValidate(const virDomainControllerDef *controller) if (opts->numaNode >= 0 && controller->idx == 0) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", - _("The PCI controller with index=0 can't " - "be associated with a NUMA node")); + _("The PCI controller with index=0 can't be associated with a NUMA node")); return -1; } } -- 2.37.1
Typecast the controller type variable and add all cases to the switch statement. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/conf/domain_conf.c | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 6950f7ec1d..673e8a77c3 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -22684,7 +22684,7 @@ virDomainControllerDefFormat(virBuffer *buf, if (model) virBufferEscapeString(&attrBuf, " model='%s'", model); - switch (def->type) { + switch ((virDomainControllerType) def->type) { case VIR_DOMAIN_CONTROLLER_TYPE_VIRTIO_SERIAL: if (def->opts.vioserial.ports != -1) { virBufferAsprintf(&attrBuf, " ports='%d'", @@ -22714,7 +22714,14 @@ virDomainControllerDefFormat(virBuffer *buf, } break; - default: + case VIR_DOMAIN_CONTROLLER_TYPE_IDE: + case VIR_DOMAIN_CONTROLLER_TYPE_FDC: + case VIR_DOMAIN_CONTROLLER_TYPE_SCSI: + case VIR_DOMAIN_CONTROLLER_TYPE_SATA: + case VIR_DOMAIN_CONTROLLER_TYPE_CCID: + case VIR_DOMAIN_CONTROLLER_TYPE_PCI: + case VIR_DOMAIN_CONTROLLER_TYPE_ISA: + case VIR_DOMAIN_CONTROLLER_TYPE_LAST: break; } -- 2.37.1
Move the PCI controller code into virDomainControllerDefFormatPCI. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/conf/domain_conf.c | 162 ++++++++++++++++++++++------------------- 1 file changed, 86 insertions(+), 76 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 673e8a77c3..b2ee4d5979 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -22650,6 +22650,88 @@ virDomainControllerDriverFormat(virBuffer *buf, } +static int +virDomainControllerDefFormatPCI(virBuffer *buf, + virDomainControllerDef *def, + unsigned int flags) +{ + bool formatModelName = true; + + if (def->opts.pciopts.modelName == VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_NONE) + formatModelName = false; + + /* Historically, libvirt didn't support specifying a model name for + * pci-root controllers; starting from 3.6.0, however, pSeries guests + * use pci-root controllers with model name spapr-pci-host-bridge to + * represent all PHBs, including the default one. + * + * In order to allow migration of pSeries guests from older libvirt + * versions and back, we don't format the model name in the migratable + * XML if it's spapr-pci-host-bridge, thus making "no model name" and + * "spapr-pci-host-bridge model name" basically equivalent. + * + * The spapr-pci-host-bridge device is specific to pSeries. + */ + if (def->model == VIR_DOMAIN_CONTROLLER_MODEL_PCI_ROOT && + def->opts.pciopts.modelName == VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_SPAPR_PCI_HOST_BRIDGE && + flags & VIR_DOMAIN_DEF_FORMAT_MIGRATABLE) { + formatModelName = false; + } + + if (formatModelName) { + const char *modelName = virDomainControllerPCIModelNameTypeToString(def->opts.pciopts.modelName); + if (!modelName) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("unexpected model name value %d"), + def->opts.pciopts.modelName); + return -1; + } + virBufferAsprintf(buf, "<model name='%s'/>\n", modelName); + } + + if (def->opts.pciopts.chassisNr != -1 || + def->opts.pciopts.chassis != -1 || + def->opts.pciopts.port != -1 || + def->opts.pciopts.busNr != -1 || + def->opts.pciopts.targetIndex != -1 || + def->opts.pciopts.numaNode != -1 || + def->opts.pciopts.hotplug != VIR_TRISTATE_SWITCH_ABSENT) { + virBufferAddLit(buf, "<target"); + if (def->opts.pciopts.chassisNr != -1) + virBufferAsprintf(buf, " chassisNr='%d'", + def->opts.pciopts.chassisNr); + if (def->opts.pciopts.chassis != -1) + virBufferAsprintf(buf, " chassis='%d'", + def->opts.pciopts.chassis); + if (def->opts.pciopts.port != -1) + virBufferAsprintf(buf, " port='0x%x'", + def->opts.pciopts.port); + if (def->opts.pciopts.busNr != -1) + virBufferAsprintf(buf, " busNr='%d'", + def->opts.pciopts.busNr); + if (def->opts.pciopts.targetIndex != -1) + virBufferAsprintf(buf, " index='%d'", + def->opts.pciopts.targetIndex); + if (def->opts.pciopts.hotplug != VIR_TRISTATE_SWITCH_ABSENT) { + virBufferAsprintf(buf, " hotplug='%s'", + virTristateSwitchTypeToString(def->opts.pciopts.hotplug)); + } + if (def->opts.pciopts.numaNode == -1) { + virBufferAddLit(buf, "/>\n"); + } else { + virBufferAddLit(buf, ">\n"); + virBufferAdjustIndent(buf, 2); + virBufferAsprintf(buf, "<node>%d</node>\n", + def->opts.pciopts.numaNode); + virBufferAdjustIndent(buf, -2); + virBufferAddLit(buf, "</target>\n"); + } + } + + return 0; +} + + static int virDomainControllerDefFormat(virBuffer *buf, virDomainControllerDef *def, @@ -22657,7 +22739,6 @@ virDomainControllerDefFormat(virBuffer *buf, { const char *type = virDomainControllerTypeToString(def->type); const char *model = NULL; - const char *modelName = NULL; g_auto(virBuffer) attrBuf = VIR_BUFFER_INITIALIZER; g_auto(virBuffer) childBuf = VIR_BUFFER_INIT_CHILD(buf); @@ -22714,91 +22795,20 @@ virDomainControllerDefFormat(virBuffer *buf, } break; + case VIR_DOMAIN_CONTROLLER_TYPE_PCI: + if (virDomainControllerDefFormatPCI(&childBuf, def, flags) < 0) + return -1; + case VIR_DOMAIN_CONTROLLER_TYPE_IDE: case VIR_DOMAIN_CONTROLLER_TYPE_FDC: case VIR_DOMAIN_CONTROLLER_TYPE_SCSI: case VIR_DOMAIN_CONTROLLER_TYPE_SATA: case VIR_DOMAIN_CONTROLLER_TYPE_CCID: - case VIR_DOMAIN_CONTROLLER_TYPE_PCI: case VIR_DOMAIN_CONTROLLER_TYPE_ISA: case VIR_DOMAIN_CONTROLLER_TYPE_LAST: break; } - if (def->type == VIR_DOMAIN_CONTROLLER_TYPE_PCI) { - bool formatModelName = true; - - if (def->opts.pciopts.modelName == VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_NONE) - formatModelName = false; - - /* Historically, libvirt didn't support specifying a model name for - * pci-root controllers; starting from 3.6.0, however, pSeries guests - * use pci-root controllers with model name spapr-pci-host-bridge to - * represent all PHBs, including the default one. - * - * In order to allow migration of pSeries guests from older libvirt - * versions and back, we don't format the model name in the migratable - * XML if it's spapr-pci-host-bridge, thus making "no model name" and - * "spapr-pci-host-bridge model name" basically equivalent. - * - * The spapr-pci-host-bridge device is specific to pSeries. - */ - if (def->model == VIR_DOMAIN_CONTROLLER_MODEL_PCI_ROOT && - def->opts.pciopts.modelName == VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_SPAPR_PCI_HOST_BRIDGE && - flags & VIR_DOMAIN_DEF_FORMAT_MIGRATABLE) { - formatModelName = false; - } - - if (formatModelName) { - modelName = virDomainControllerPCIModelNameTypeToString(def->opts.pciopts.modelName); - if (!modelName) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("unexpected model name value %d"), - def->opts.pciopts.modelName); - return -1; - } - virBufferAsprintf(&childBuf, "<model name='%s'/>\n", modelName); - } - - if (def->opts.pciopts.chassisNr != -1 || - def->opts.pciopts.chassis != -1 || - def->opts.pciopts.port != -1 || - def->opts.pciopts.busNr != -1 || - def->opts.pciopts.targetIndex != -1 || - def->opts.pciopts.numaNode != -1 || - def->opts.pciopts.hotplug != VIR_TRISTATE_SWITCH_ABSENT) { - virBufferAddLit(&childBuf, "<target"); - if (def->opts.pciopts.chassisNr != -1) - virBufferAsprintf(&childBuf, " chassisNr='%d'", - def->opts.pciopts.chassisNr); - if (def->opts.pciopts.chassis != -1) - virBufferAsprintf(&childBuf, " chassis='%d'", - def->opts.pciopts.chassis); - if (def->opts.pciopts.port != -1) - virBufferAsprintf(&childBuf, " port='0x%x'", - def->opts.pciopts.port); - if (def->opts.pciopts.busNr != -1) - virBufferAsprintf(&childBuf, " busNr='%d'", - def->opts.pciopts.busNr); - if (def->opts.pciopts.targetIndex != -1) - virBufferAsprintf(&childBuf, " index='%d'", - def->opts.pciopts.targetIndex); - if (def->opts.pciopts.hotplug != VIR_TRISTATE_SWITCH_ABSENT) { - virBufferAsprintf(&childBuf, " hotplug='%s'", - virTristateSwitchTypeToString(def->opts.pciopts.hotplug)); - } - if (def->opts.pciopts.numaNode == -1) { - virBufferAddLit(&childBuf, "/>\n"); - } else { - virBufferAddLit(&childBuf, ">\n"); - virBufferAdjustIndent(&childBuf, 2); - virBufferAsprintf(&childBuf, "<node>%d</node>\n", - def->opts.pciopts.numaNode); - virBufferAdjustIndent(&childBuf, -2); - virBufferAddLit(&childBuf, "</target>\n"); - } - } - } virDomainControllerDriverFormat(&childBuf, def); -- 2.37.1
Rewrite the code to use virXMLFormat element so that we can avoid a bunch of unnecessary checks. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/conf/domain_conf.c | 56 ++++++++++++++---------------------------- 1 file changed, 19 insertions(+), 37 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index b2ee4d5979..8573be36d0 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -22655,6 +22655,8 @@ virDomainControllerDefFormatPCI(virBuffer *buf, virDomainControllerDef *def, unsigned int flags) { + g_auto(virBuffer) targetAttrBuf = VIR_BUFFER_INITIALIZER; + g_auto(virBuffer) targetChildBuf = VIR_BUFFER_INIT_CHILD(buf); bool formatModelName = true; if (def->opts.pciopts.modelName == VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_NONE) @@ -22689,45 +22691,25 @@ virDomainControllerDefFormatPCI(virBuffer *buf, virBufferAsprintf(buf, "<model name='%s'/>\n", modelName); } - if (def->opts.pciopts.chassisNr != -1 || - def->opts.pciopts.chassis != -1 || - def->opts.pciopts.port != -1 || - def->opts.pciopts.busNr != -1 || - def->opts.pciopts.targetIndex != -1 || - def->opts.pciopts.numaNode != -1 || - def->opts.pciopts.hotplug != VIR_TRISTATE_SWITCH_ABSENT) { - virBufferAddLit(buf, "<target"); - if (def->opts.pciopts.chassisNr != -1) - virBufferAsprintf(buf, " chassisNr='%d'", - def->opts.pciopts.chassisNr); - if (def->opts.pciopts.chassis != -1) - virBufferAsprintf(buf, " chassis='%d'", - def->opts.pciopts.chassis); - if (def->opts.pciopts.port != -1) - virBufferAsprintf(buf, " port='0x%x'", - def->opts.pciopts.port); - if (def->opts.pciopts.busNr != -1) - virBufferAsprintf(buf, " busNr='%d'", - def->opts.pciopts.busNr); - if (def->opts.pciopts.targetIndex != -1) - virBufferAsprintf(buf, " index='%d'", - def->opts.pciopts.targetIndex); - if (def->opts.pciopts.hotplug != VIR_TRISTATE_SWITCH_ABSENT) { - virBufferAsprintf(buf, " hotplug='%s'", - virTristateSwitchTypeToString(def->opts.pciopts.hotplug)); - } - if (def->opts.pciopts.numaNode == -1) { - virBufferAddLit(buf, "/>\n"); - } else { - virBufferAddLit(buf, ">\n"); - virBufferAdjustIndent(buf, 2); - virBufferAsprintf(buf, "<node>%d</node>\n", - def->opts.pciopts.numaNode); - virBufferAdjustIndent(buf, -2); - virBufferAddLit(buf, "</target>\n"); - } + if (def->opts.pciopts.chassisNr != -1) + virBufferAsprintf(&targetAttrBuf, " chassisNr='%d'", def->opts.pciopts.chassisNr); + if (def->opts.pciopts.chassis != -1) + virBufferAsprintf(&targetAttrBuf, " chassis='%d'", def->opts.pciopts.chassis); + if (def->opts.pciopts.port != -1) + virBufferAsprintf(&targetAttrBuf, " port='0x%x'", def->opts.pciopts.port); + if (def->opts.pciopts.busNr != -1) + virBufferAsprintf(&targetAttrBuf, " busNr='%d'", def->opts.pciopts.busNr); + if (def->opts.pciopts.targetIndex != -1) + virBufferAsprintf(&targetAttrBuf, " index='%d'", def->opts.pciopts.targetIndex); + if (def->opts.pciopts.hotplug != VIR_TRISTATE_SWITCH_ABSENT) { + virBufferAsprintf(&targetAttrBuf, " hotplug='%s'", + virTristateSwitchTypeToString(def->opts.pciopts.hotplug)); } + if (def->opts.pciopts.numaNode != -1) + virBufferAsprintf(&targetChildBuf, "<node>%d</node>\n", def->opts.pciopts.numaNode); + + virXMLFormatElement(buf, "target", &targetAttrBuf, &targetChildBuf); return 0; } -- 2.37.1
Code was not indented properly for one of the nested conditions. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/conf/domain_conf.c | 56 +++++++++++++++++++++--------------------- 1 file changed, 28 insertions(+), 28 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 8573be36d0..3ff9fffba8 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -8208,40 +8208,40 @@ virDomainControllerDefParseXML(virDomainXMLOption *xmlopt, ntargetNodes = virXPathNodeSet("./target", ctxt, &targetNodes); if (ntargetNodes == 1) { if (def->type == VIR_DOMAIN_CONTROLLER_TYPE_PCI) { - if (virXMLPropInt(targetNodes[0], "chassisNr", 0, VIR_XML_PROP_NONE, - &def->opts.pciopts.chassisNr, - def->opts.pciopts.chassisNr) < 0) - return NULL; + if (virXMLPropInt(targetNodes[0], "chassisNr", 0, VIR_XML_PROP_NONE, + &def->opts.pciopts.chassisNr, + def->opts.pciopts.chassisNr) < 0) + return NULL; - if (virXMLPropInt(targetNodes[0], "chassis", 0, VIR_XML_PROP_NONE, - &def->opts.pciopts.chassis, - def->opts.pciopts.chassis) < 0) - return NULL; + if (virXMLPropInt(targetNodes[0], "chassis", 0, VIR_XML_PROP_NONE, + &def->opts.pciopts.chassis, + def->opts.pciopts.chassis) < 0) + return NULL; - if (virXMLPropInt(targetNodes[0], "port", 0, VIR_XML_PROP_NONE, - &def->opts.pciopts.port, - def->opts.pciopts.port) < 0) - return NULL; + if (virXMLPropInt(targetNodes[0], "port", 0, VIR_XML_PROP_NONE, + &def->opts.pciopts.port, + def->opts.pciopts.port) < 0) + return NULL; - if (virXMLPropInt(targetNodes[0], "busNr", 0, VIR_XML_PROP_NONE, - &def->opts.pciopts.busNr, - def->opts.pciopts.busNr) < 0) - return NULL; + if (virXMLPropInt(targetNodes[0], "busNr", 0, VIR_XML_PROP_NONE, + &def->opts.pciopts.busNr, + def->opts.pciopts.busNr) < 0) + return NULL; - if (virXMLPropTristateSwitch(targetNodes[0], "hotplug", - VIR_XML_PROP_NONE, - &def->opts.pciopts.hotplug) < 0) - return NULL; + if (virXMLPropTristateSwitch(targetNodes[0], "hotplug", + VIR_XML_PROP_NONE, + &def->opts.pciopts.hotplug) < 0) + return NULL; - if ((rc = virXMLPropInt(targetNodes[0], "index", 0, VIR_XML_PROP_NONE, - &def->opts.pciopts.targetIndex, - def->opts.pciopts.targetIndex)) < 0) - return NULL; + if ((rc = virXMLPropInt(targetNodes[0], "index", 0, VIR_XML_PROP_NONE, + &def->opts.pciopts.targetIndex, + 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 ((rc == 1) && def->opts.pciopts.targetIndex == -1) + virReportError(VIR_ERR_XML_ERROR, + _("Invalid target index '%i' in PCI controller"), + def->opts.pciopts.targetIndex); } } else if (ntargetNodes > 1) { virReportError(VIR_ERR_XML_ERROR, "%s", -- 2.37.1
The code attempted to report an error if the user added the 'index' attribute to the 'target' element, but neglected to actually return an error code. Fix it by using the VIR_XML_PROP_NONNEGATIVE flag for virXMLPropInt which refuses user passed negative numbers. Fixes: 020dd80ecbd Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/conf/domain_conf.c | 11 +++-------- 1 file changed, 3 insertions(+), 8 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 3ff9fffba8..a9a2afc7f1 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -8233,15 +8233,10 @@ virDomainControllerDefParseXML(virDomainXMLOption *xmlopt, &def->opts.pciopts.hotplug) < 0) return NULL; - if ((rc = virXMLPropInt(targetNodes[0], "index", 0, VIR_XML_PROP_NONE, - &def->opts.pciopts.targetIndex, - def->opts.pciopts.targetIndex)) < 0) + if (virXMLPropInt(targetNodes[0], "index", 0, VIR_XML_PROP_NONNEGATIVE, + &def->opts.pciopts.targetIndex, + 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); } } else if (ntargetNodes > 1) { virReportError(VIR_ERR_XML_ERROR, "%s", -- 2.37.1
Refactor all cases which use virXMLPropInt and then subsequently check that the parsed value is not '-1'/negative by using the VIR_XML_PROP_NONNEGATIVE flag for virXMLPropInt. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/conf/domain_conf.c | 46 +++++++++--------------------------------- 1 file changed, 10 insertions(+), 36 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index a9a2afc7f1..77cb5f1456 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -8263,27 +8263,15 @@ virDomainControllerDefParseXML(virDomainXMLOption *xmlopt, return NULL; } - if ((rc = virXMLPropInt(node, "ports", 10, VIR_XML_PROP_NONE, &ports, -1)) < 0) + if (virXMLPropInt(node, "ports", 10, VIR_XML_PROP_NONNEGATIVE, &ports, -1) < 0) return NULL; - if ((rc == 1) && ports < 0) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("Invalid ports: %i"), ports); - return NULL; - } switch (def->type) { case VIR_DOMAIN_CONTROLLER_TYPE_VIRTIO_SERIAL: { - if ((rc = virXMLPropInt(node, "vectors", 10, VIR_XML_PROP_NONE, - &def->opts.vioserial.vectors, - def->opts.vioserial.vectors)) < 0) - return NULL; - - if ((rc == 1) && def->opts.vioserial.vectors < 0) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("Invalid vectors: %i"), - def->opts.vioserial.vectors); + if (virXMLPropInt(node, "vectors", 10, VIR_XML_PROP_NONNEGATIVE, + &def->opts.vioserial.vectors, + def->opts.vioserial.vectors) < 0) return NULL; - } def->opts.vioserial.ports = ports; break; @@ -8345,29 +8333,15 @@ virDomainControllerDefParseXML(virDomainXMLOption *xmlopt, break; case VIR_DOMAIN_CONTROLLER_TYPE_XENBUS: { - if ((rc = virXMLPropInt(node, "maxGrantFrames", 10, VIR_XML_PROP_NONE, - &def->opts.xenbusopts.maxGrantFrames, - def->opts.xenbusopts.maxGrantFrames)) < 0) + if (virXMLPropInt(node, "maxGrantFrames", 10, VIR_XML_PROP_NONNEGATIVE, + &def->opts.xenbusopts.maxGrantFrames, + def->opts.xenbusopts.maxGrantFrames) < 0) return NULL; - if ((rc == 1) && def->opts.xenbusopts.maxGrantFrames < 0) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("Invalid maxGrantFrames: %i"), - def->opts.xenbusopts.maxGrantFrames); + if (virXMLPropInt(node, "maxEventChannels", 10, VIR_XML_PROP_NONNEGATIVE, + &def->opts.xenbusopts.maxEventChannels, + def->opts.xenbusopts.maxEventChannels) < 0) return NULL; - } - - if ((rc = virXMLPropInt(node, "maxEventChannels", 10, VIR_XML_PROP_NONE, - &def->opts.xenbusopts.maxEventChannels, - def->opts.xenbusopts.maxEventChannels)) < 0) - return NULL; - - if ((rc == 1) && def->opts.xenbusopts.maxEventChannels < 0) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("Invalid maxEventChannels: %i"), - def->opts.xenbusopts.maxEventChannels); - return NULL; - } break; } -- 2.37.1
'index' is parsed to fit into a signed int but not have negative values. virXMLPropInt can do that internally. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/conf/domain_conf.c | 14 +++----------- .../pci-bridge-negative-index-invalid.err | 2 +- 2 files changed, 4 insertions(+), 12 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 77cb5f1456..70672c83ce 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -8132,7 +8132,6 @@ virDomainControllerDefParseXML(virDomainXMLOption *xmlopt, int ports; VIR_XPATH_NODE_AUTORESTORE(ctxt) int rc; - g_autofree char *idx = NULL; g_autofree char *model = NULL; ctxt->node = node; @@ -8152,16 +8151,9 @@ virDomainControllerDefParseXML(virDomainXMLOption *xmlopt, } } - idx = virXMLPropString(node, "index"); - if (idx) { - unsigned int idxVal; - if (virStrToLong_ui(idx, NULL, 10, &idxVal) < 0 || idxVal > INT_MAX) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("Cannot parse controller index %s"), idx); - return NULL; - } - def->idx = idxVal; - } + if (virXMLPropInt(node, "index", 10, VIR_XML_PROP_NONNEGATIVE, + &def->idx, def->idx) < 0) + return NULL; if ((driver = virXPathNode("./driver", ctxt))) { if (virXMLPropUInt(driver, "queues", 10, VIR_XML_PROP_NONE, diff --git a/tests/qemuxml2argvdata/pci-bridge-negative-index-invalid.err b/tests/qemuxml2argvdata/pci-bridge-negative-index-invalid.err index e258bcbee5..24a7d817e1 100644 --- a/tests/qemuxml2argvdata/pci-bridge-negative-index-invalid.err +++ b/tests/qemuxml2argvdata/pci-bridge-negative-index-invalid.err @@ -1 +1 @@ -internal error: Cannot parse controller index -1 +XML error: Invalid value for attribute 'index' in element 'controller': '-1'. Expected non-negative value -- 2.37.1
Move some checks earlier so that they are not tucked at the back of the block of code doing the actual parsing. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/conf/domain_conf.c | 20 ++++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 70672c83ce..a4384f9d13 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -8181,7 +8181,12 @@ virDomainControllerDefParseXML(virDomainXMLOption *xmlopt, return NULL; } - nmodelNodes = virXPathNodeSet("./model", ctxt, &modelNodes); + if ((nmodelNodes = virXPathNodeSet("./model", ctxt, &modelNodes)) > 1) { + virReportError(VIR_ERR_XML_ERROR, "%s", + _("Multiple <model> elements in controller definition not allowed")); + return NULL; + } + if (nmodelNodes == 1) { if (def->type == VIR_DOMAIN_CONTROLLER_TYPE_PCI) { if (virXMLPropEnum(modelNodes[0], "name", @@ -8190,14 +8195,14 @@ virDomainControllerDefParseXML(virDomainXMLOption *xmlopt, &def->opts.pciopts.modelName) < 0) return NULL; } - } else if (nmodelNodes > 1) { + } + + if ((ntargetNodes = virXPathNodeSet("./target", ctxt, &targetNodes)) > 1) { virReportError(VIR_ERR_XML_ERROR, "%s", - _("Multiple <model> elements in " - "controller definition not allowed")); + _("Multiple <target> elements in controller definition not allowed")); return NULL; } - ntargetNodes = virXPathNodeSet("./target", ctxt, &targetNodes); if (ntargetNodes == 1) { if (def->type == VIR_DOMAIN_CONTROLLER_TYPE_PCI) { if (virXMLPropInt(targetNodes[0], "chassisNr", 0, VIR_XML_PROP_NONE, @@ -8230,11 +8235,6 @@ virDomainControllerDefParseXML(virDomainXMLOption *xmlopt, def->opts.pciopts.targetIndex) < 0) return NULL; } - } else if (ntargetNodes > 1) { - virReportError(VIR_ERR_XML_ERROR, "%s", - _("Multiple <target> elements in " - "controller definition not allowed")); - return NULL; } /* node is parsed differently from target attributes because -- 2.37.1
All of the properties use '-1' as default and the code omits formatting them when the property is '-1'. Additionally subsequent validation code rejects all other negative values anyways. Since we've never formatted '-1' into an XML formatted by libvirt we can make the parser more strict, as we will never fail to parse existing on-disk libvirt-owned XMLs. Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=2121627 Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/conf/domain_conf.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index a4384f9d13..65afebbfde 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -8205,22 +8205,22 @@ virDomainControllerDefParseXML(virDomainXMLOption *xmlopt, if (ntargetNodes == 1) { if (def->type == VIR_DOMAIN_CONTROLLER_TYPE_PCI) { - if (virXMLPropInt(targetNodes[0], "chassisNr", 0, VIR_XML_PROP_NONE, + if (virXMLPropInt(targetNodes[0], "chassisNr", 0, VIR_XML_PROP_NONNEGATIVE, &def->opts.pciopts.chassisNr, def->opts.pciopts.chassisNr) < 0) return NULL; - if (virXMLPropInt(targetNodes[0], "chassis", 0, VIR_XML_PROP_NONE, + if (virXMLPropInt(targetNodes[0], "chassis", 0, VIR_XML_PROP_NONNEGATIVE, &def->opts.pciopts.chassis, def->opts.pciopts.chassis) < 0) return NULL; - if (virXMLPropInt(targetNodes[0], "port", 0, VIR_XML_PROP_NONE, + if (virXMLPropInt(targetNodes[0], "port", 0, VIR_XML_PROP_NONNEGATIVE, &def->opts.pciopts.port, def->opts.pciopts.port) < 0) return NULL; - if (virXMLPropInt(targetNodes[0], "busNr", 0, VIR_XML_PROP_NONE, + if (virXMLPropInt(targetNodes[0], "busNr", 0, VIR_XML_PROP_NONNEGATIVE, &def->opts.pciopts.busNr, def->opts.pciopts.busNr) < 0) return NULL; -- 2.37.1
On a Friday in 2022, Peter Krempa wrote:
A relatively simple-looking bug report (see 10/10) lead me looking and finding few more problems.
Peter Krempa (10): virDomainControllerDefValidate: Un-break lines in error messages virDomainControllerDefFormat: Use proper type for enum virDomainControllerDefFormat: Split out formatting of PCI controller virDomainControllerDefFormatPCI: Refactor formatting of '<target>' subelement virDomainControllerDefParseXML: Fix broken code indentation when parsing PCI contoller target virDomainControllerDefParseXML: Fix broken error reporting when parsing 'index' virDomainControllerDefParseXML: Remove explicit checks for negative value virDomainControllerDefParseXML: Parse 'index' by virXMLPropInt virDomainControllerDefParseXML: Return early if there's unexpectedly many elements virDomainControllerDefParseXML: Reject '-1' for PCI controller target properties
src/conf/domain_conf.c | 286 ++++++++---------- src/conf/domain_validate.c | 24 +- .../pci-bridge-negative-index-invalid.err | 2 +- 3 files changed, 132 insertions(+), 180 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
participants (2)
-
Ján Tomko -
Peter Krempa