7:19 a.m.
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
7:19 a.m.
New subject: [PATCH 01/10] virDomainControllerDefValidate: Un-break lines in error messages
Signed-off-by: Peter Krempa <pkrempa(a)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
7:19 a.m.
New subject: [PATCH 02/10] virDomainControllerDefFormat: Use proper type for enum
Typecast the controller type variable and add all cases to the switch
statement.
Signed-off-by: Peter Krempa <pkrempa(a)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
7:19 a.m.
New subject: [PATCH 03/10] virDomainControllerDefFormat: Split out formatting of PCI controller
Move the PCI controller code into virDomainControllerDefFormatPCI.
Signed-off-by: Peter Krempa <pkrempa(a)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
7:19 a.m.
New subject: [PATCH 04/10] virDomainControllerDefFormatPCI: Refactor formatting of '<target>' subelement
Rewrite the code to use virXMLFormat element so that we can avoid a
bunch of unnecessary checks.
Signed-off-by: Peter Krempa <pkrempa(a)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
7:20 a.m.
New subject: [PATCH 05/10] virDomainControllerDefParseXML: Fix broken code indentation when parsing PCI contoller target
Code was not indented properly for one of the nested conditions.
Signed-off-by: Peter Krempa <pkrempa(a)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
7:20 a.m.
New subject: [PATCH 06/10] virDomainControllerDefParseXML: Fix broken error reporting when parsing 'index'
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(a)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
7:20 a.m.
New subject: [PATCH 07/10] virDomainControllerDefParseXML: Remove explicit checks for negative value
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(a)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
7:20 a.m.
New subject: [PATCH 08/10] virDomainControllerDefParseXML: Parse 'index' by virXMLPropInt
'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(a)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
7:20 a.m.
New subject: [PATCH 09/10] virDomainControllerDefParseXML: Return early if there's unexpectedly many elements
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(a)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
7:20 a.m.
New subject: [PATCH 10/10] virDomainControllerDefParseXML: Reject '-1' for PCI controller target properties
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(a)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
7:53 a.m.
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(a)redhat.com>
Jano
819
days inactive
819
days old
11 comments
2 participants
participants (2)
-
Ján Tomko -
Peter Krempa