[libvirt PATCH v2 0/6] Refactor more XML parsing boilerplate code, part VI
For background, see https://listman.redhat.com/archives/libvir-list/2021-April/msg00668.html V1: https://listman.redhat.com/archives/libvir-list/2021-April/msg01184.html Changes since V1: * Split up "virDomainFeaturesDefParse" * Added missing VIR_XML_PROP_NONZERO * Some line break changes * Mentioned printf %s NULL fix in commit message * Improved justification for parser strictening Tim Wiederhake (6): virDomainGraphicsDefParseXMLSpice: Use virXMLProp* virDomainSoundDefParseXML: Use virXMLProp* virDomainFeaturesDefParse: Use virXMLPropUInt virDomainFeaturesDefParse: Use virXMLPropTristateSwitch virDomainFeaturesDefParse: Use virXMLPropEnum virDomainAudioSDLParse: Use virXMLProp* src/conf/domain_conf.c | 609 +++++------------- .../qemuxml2argvdata/aarch64-gic-invalid.err | 2 +- 2 files changed, 170 insertions(+), 441 deletions(-) -- 2.26.3
Signed-off-by: Tim Wiederhake <twiederh@redhat.com> --- src/conf/domain_conf.c | 251 ++++++++++------------------------------- 1 file changed, 59 insertions(+), 192 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 9d98f487ea..822426bc4e 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -12905,49 +12905,30 @@ virDomainGraphicsDefParseXMLSpice(virDomainGraphicsDef *def, unsigned int flags) { xmlNodePtr cur; - int defaultModeVal; - 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"); if (virDomainGraphicsListensParseXML(def, node, ctxt, flags) < 0) return -1; - if (port) { - if (virStrToLong_i(port, NULL, 10, &def->data.spice.port) < 0) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("cannot parse spice port %s"), port); - return -1; - } - } else { - def->data.spice.port = 0; - } + def->data.spice.port = 0; + if (virXMLPropInt(node, "port", 10, VIR_XML_PROP_NONE, + &def->data.spice.port) < 0) + return -1; - if (tlsPort) { - if (virStrToLong_i(tlsPort, NULL, 10, &def->data.spice.tlsPort) < 0) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("cannot parse spice tlsPort %s"), tlsPort); - return -1; - } - } else { - def->data.spice.tlsPort = 0; - } + def->data.spice.tlsPort = 0; + if (virXMLPropInt(node, "tlsPort", 10, VIR_XML_PROP_NONE, + &def->data.spice.tlsPort) < 0) + return -1; if (STREQ_NULLABLE(autoport, "yes")) def->data.spice.autoport = true; def->data.spice.defaultMode = VIR_DOMAIN_GRAPHICS_SPICE_CHANNEL_MODE_ANY; - if (defaultMode) { - if ((defaultModeVal = virDomainGraphicsSpiceChannelModeTypeFromString(defaultMode)) < 0) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, - _("unknown default spice channel mode %s"), - defaultMode); - return -1; - } - def->data.spice.defaultMode = defaultModeVal; - } + if (virXMLPropEnum(node, "defaultMode", + virDomainGraphicsSpiceChannelModeTypeFromString, + VIR_XML_PROP_NONE, &def->data.spice.defaultMode) < 0) + return -1; if (def->data.spice.port == -1 && def->data.spice.tlsPort == -1) { /* Legacy compat syntax, used -1 for auto-port */ @@ -12969,200 +12950,86 @@ virDomainGraphicsDefParseXMLSpice(virDomainGraphicsDef *def, 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"); + virDomainGraphicsSpiceChannelName name; + virDomainGraphicsSpiceChannelMode mode; - if (!name || !mode) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("spice channel missing name/mode")); + if (virXMLPropEnum(cur, "name", + virDomainGraphicsSpiceChannelNameTypeFromString, + VIR_XML_PROP_REQUIRED, &name) < 0) 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); + if (virXMLPropEnum(cur, "mode", + virDomainGraphicsSpiceChannelModeTypeFromString, + VIR_XML_PROP_REQUIRED, &mode) < 0) return -1; - } - def->data.spice.channels[nameval] = modeval; + def->data.spice.channels[name] = mode; } else if (virXMLNodeNameEqual(cur, "image")) { - int compressionVal; - g_autofree char *compression = virXMLPropString(cur, "compression"); + virDomainGraphicsSpiceImageCompression compression; - if (!compression) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("spice image missing compression")); + if (virXMLPropEnum(cur, "compression", + virDomainGraphicsSpiceImageCompressionTypeFromString, + VIR_XML_PROP_NONZERO, &compression) < 0) 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; + def->data.spice.image = compression; } else if (virXMLNodeNameEqual(cur, "jpeg")) { - int compressionVal; - g_autofree char *compression = virXMLPropString(cur, "compression"); + virDomainGraphicsSpiceJpegCompression compression; - if (!compression) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("spice jpeg missing compression")); + if (virXMLPropEnum(cur, "compression", + virDomainGraphicsSpiceJpegCompressionTypeFromString, + VIR_XML_PROP_NONZERO, &compression) < 0) 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; + def->data.spice.jpeg = compression; } 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; - } + virDomainGraphicsSpiceZlibCompression compression; - if ((compressionVal = - virDomainGraphicsSpiceZlibCompressionTypeFromString(compression)) <= 0) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, - _("unknown spice zlib compression %s"), - compression); + if (virXMLPropEnum(cur, "compression", + virDomainGraphicsSpiceZlibCompressionTypeFromString, + VIR_XML_PROP_REQUIRED, &compression) < 0) return -1; - } - def->data.spice.zlib = compressionVal; + def->data.spice.zlib = compression; } 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")); + if (virXMLPropTristateSwitch(cur, "compression", + VIR_XML_PROP_NONZERO, + &def->data.spice.playback) < 0) return -1; - } - def->data.spice.playback = compressionVal; } else if (virXMLNodeNameEqual(cur, "streaming")) { - int modeVal; - g_autofree char *mode = virXMLPropString(cur, "mode"); + virDomainGraphicsSpiceStreamingMode 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")); + if (virXMLPropEnum(cur, "mode", + virDomainGraphicsSpiceStreamingModeTypeFromString, + VIR_XML_PROP_REQUIRED | VIR_XML_PROP_NONZERO, + &mode) < 0) return -1; - } - def->data.spice.streaming = modeVal; + def->data.spice.streaming = mode; } 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")); + if (virXMLPropTristateBool(cur, "copypaste", + VIR_XML_PROP_REQUIRED, + &def->data.spice.copypaste) < 0) return -1; - } - - if ((copypasteVal = - virTristateBoolTypeFromString(copypaste)) <= 0) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, - _("unknown copypaste value '%s'"), copypaste); - return -1; - } - - def->data.spice.copypaste = copypasteVal; } else if (virXMLNodeNameEqual(cur, "filetransfer")) { - int enableVal; - g_autofree char *enable = virXMLPropString(cur, "enable"); - - if (!enable) { - virReportError(VIR_ERR_XML_ERROR, "%s", - _("spice filetransfer missing enable")); + if (virXMLPropTristateBool(cur, "enable", + VIR_XML_PROP_REQUIRED, + &def->data.spice.filetransfer) < 0) return -1; - } - - if ((enableVal = - virTristateBoolTypeFromString(enable)) <= 0) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, - _("unknown enable value '%s'"), enable); - return -1; - } - - 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"); + def->data.spice.rendernode = virXMLPropString(cur, "rendernode"); - if (!enable) { - virReportError(VIR_ERR_XML_ERROR, "%s", - _("spice gl element missing enable")); - return -1; - } - - if ((enableVal = - virTristateBoolTypeFromString(enable)) <= 0) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, - _("unknown enable value '%s'"), enable); + if (virXMLPropTristateBool(cur, "enable", + VIR_XML_PROP_REQUIRED, + &def->data.spice.gl) < 0) return -1; - } - - def->data.spice.gl = enableVal; - def->data.spice.rendernode = g_steal_pointer(&rendernode); - } else if (virXMLNodeNameEqual(cur, "mouse")) { - int modeVal; - g_autofree char *mode = virXMLPropString(cur, "mode"); - - if (!mode) { - virReportError(VIR_ERR_XML_ERROR, "%s", - _("spice mouse missing mode")); + if (virXMLPropEnum(cur, "mode", + virDomainGraphicsSpiceMouseModeTypeFromString, + VIR_XML_PROP_REQUIRED | VIR_XML_PROP_NONZERO, + &def->data.spice.mousemode) < 0) return -1; - } - - if ((modeVal = virDomainGraphicsSpiceMouseModeTypeFromString(mode)) <= 0) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, - _("unknown mouse mode value '%s'"), - mode); - return -1; - } - - def->data.spice.mousemode = modeVal; } } cur = cur->next; -- 2.26.3
On a Tuesday in 2021, Tim Wiederhake wrote:
Signed-off-by: Tim Wiederhake <twiederh@redhat.com> --- src/conf/domain_conf.c | 251 ++++++++++------------------------------- 1 file changed, 59 insertions(+), 192 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 9d98f487ea..822426bc4e 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -12969,200 +12950,86 @@ virDomainGraphicsDefParseXMLSpice(virDomainGraphicsDef *def,
[...]
} else if (virXMLNodeNameEqual(cur, "image")) { - int compressionVal; - g_autofree char *compression = virXMLPropString(cur, "compression"); + virDomainGraphicsSpiceImageCompression compression;
- if (!compression) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("spice image missing compression"));
Originally we errored out if the element was missing.
+ if (virXMLPropEnum(cur, "compression", + virDomainGraphicsSpiceImageCompressionTypeFromString, + VIR_XML_PROP_NONZERO, &compression) < 0)
This needs VIR_XML_PROP_REQUIRED
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; + def->data.spice.image = compression; } else if (virXMLNodeNameEqual(cur, "jpeg")) { - int compressionVal; - g_autofree char *compression = virXMLPropString(cur, "compression"); + virDomainGraphicsSpiceJpegCompression compression;
- if (!compression) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("spice jpeg missing compression")); + if (virXMLPropEnum(cur, "compression", + virDomainGraphicsSpiceJpegCompressionTypeFromString, + VIR_XML_PROP_NONZERO, &compression) < 0)
Same here.
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; + def->data.spice.jpeg = compression; } 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; - } + virDomainGraphicsSpiceZlibCompression compression;
- if ((compressionVal = - virDomainGraphicsSpiceZlibCompressionTypeFromString(compression)) <= 0) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, - _("unknown spice zlib compression %s"), - compression); + if (virXMLPropEnum(cur, "compression", + virDomainGraphicsSpiceZlibCompressionTypeFromString, + VIR_XML_PROP_REQUIRED, &compression) < 0)
VIR_XML_PROP_NONZERO - zero was not allowed originally.
return -1; - }
- def->data.spice.zlib = compressionVal; + def->data.spice.zlib = compression; } 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")); + if (virXMLPropTristateSwitch(cur, "compression", + VIR_XML_PROP_NONZERO, + &def->data.spice.playback) < 0)
NONZERO is implied by virXMLPropTristateSwitch. Assuming it is OK in this case where we checked for <= 0 but other places might have accepted the word "default". Since we never generate the default value and we never documented it, it might be okay to remove. But that's out of scope of this patch and this hunk is currently correct with: s/VIR_XML_PROP_NONZERO/VIR_XML_PROP_REQUIRED/
return -1; - }
- def->data.spice.playback = compressionVal;
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
This strictens the parser to disallow negative values (interpreted as `UINT_MAX + value + 1`) for attribute `id`. `id` must be greater than 0 and does not benefit from being referable as e.g. "-7" for host audio backend 4294967289, as this value is distinctly out of range for normal use. Additionally, this patch fixes a use of NULL string with printf's %s modifier if the `model` attribute is absent. Signed-off-by: Tim Wiederhake <twiederh@redhat.com> --- src/conf/domain_conf.c | 25 +++++-------------------- 1 file changed, 5 insertions(+), 20 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 822426bc4e..59630415a4 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -13173,20 +13173,14 @@ virDomainSoundDefParseXML(virDomainXMLOption *xmlopt, { virDomainSoundDef *def; VIR_XPATH_NODE_AUTORESTORE(ctxt) - g_autofree char *model = NULL; - int modelval; xmlNodePtr audioNode; def = g_new0(virDomainSoundDef, 1); ctxt->node = node; - model = virXMLPropString(node, "model"); - if ((modelval = virDomainSoundModelTypeFromString(model)) < 0) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, - _("unknown sound model '%s'"), model); + if (virXMLPropEnum(node, "model", virDomainSoundModelTypeFromString, + VIR_XML_PROP_REQUIRED, &def->model) < 0) goto error; - } - def->model = modelval; if (virDomainSoundModelSupportsCodecs(def)) { int ncodecs; @@ -13215,19 +13209,10 @@ virDomainSoundDefParseXML(virDomainXMLOption *xmlopt, audioNode = virXPathNode("./audio", ctxt); if (audioNode) { - g_autofree char *tmp = NULL; - tmp = virXMLPropString(audioNode, "id"); - if (!tmp) { - virReportError(VIR_ERR_XML_ERROR, "%s", - _("missing audio 'id' attribute")); - goto error; - } - if (virStrToLong_ui(tmp, NULL, 10, &def->audioId) < 0 || - def->audioId == 0) { - virReportError(VIR_ERR_XML_ERROR, - _("Invalid audio 'id' value '%s'"), tmp); + if (virXMLPropUInt(audioNode, "id", 10, + VIR_XML_PROP_REQUIRED | VIR_XML_PROP_NONZERO, + &def->audioId) < 0) goto error; - } } if (virDomainDeviceInfoParseXML(xmlopt, node, ctxt, &def->info, flags) < 0) -- 2.26.3
On a Tuesday in 2021, Tim Wiederhake wrote:
This strictens the parser to disallow negative values (interpreted as `UINT_MAX + value + 1`) for attribute `id`.
`id` must be greater than 0 and does not benefit from being referable as e.g. "-7" for host audio backend 4294967289, as this value is distinctly out of range for normal use.
Additionally, this patch fixes a use of NULL string with printf's %s modifier if the `model` attribute is absent.
Signed-off-by: Tim Wiederhake <twiederh@redhat.com> --- src/conf/domain_conf.c | 25 +++++-------------------- 1 file changed, 5 insertions(+), 20 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
This strictens the parser to disallow negative values (interpreted as `UINT_MAX + value + 1`) for attribute `retries`. UINT_MAX holds no special significance for this attribute and is distinctly out of range for normal use. Signed-off-by: Tim Wiederhake <twiederh@redhat.com> --- src/conf/domain_conf.c | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 59630415a4..b1e5115c7e 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -18108,12 +18108,10 @@ virDomainFeaturesDefParse(virDomainDef *def, if (value != VIR_TRISTATE_SWITCH_ON) break; - if (virXPathUInt("string(./@retries)", ctxt, - &def->hyperv_spinlocks) < 0) { - virReportError(VIR_ERR_XML_ERROR, "%s", - _("invalid HyperV spinlock retry count")); + if (virXMLPropUInt(nodes[i], "retries", 0, + VIR_XML_PROP_REQUIRED, + &def->hyperv_spinlocks) < 0) return -1; - } if (def->hyperv_spinlocks < 0xFFF) { virReportError(VIR_ERR_XML_ERROR, "%s", -- 2.26.3
On a Tuesday in 2021, Tim Wiederhake wrote:
This strictens the parser to disallow negative values (interpreted as `UINT_MAX + value + 1`) for attribute `retries`. UINT_MAX holds no special significance for this attribute and is distinctly out of range for normal use.
Signed-off-by: Tim Wiederhake <twiederh@redhat.com> --- src/conf/domain_conf.c | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
Signed-off-by: Tim Wiederhake <twiederh@redhat.com> --- src/conf/domain_conf.c | 141 +++++++++++------------------------------ 1 file changed, 37 insertions(+), 104 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index b1e5115c7e..4c7affe5af 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -17913,18 +17913,16 @@ virDomainFeaturesDefParse(virDomainDef *def, case VIR_DOMAIN_FEATURE_PMU: case VIR_DOMAIN_FEATURE_PVSPINLOCK: case VIR_DOMAIN_FEATURE_VMPORT: - case VIR_DOMAIN_FEATURE_SMM: - if ((tmp = virXMLPropString(nodes[i], "state"))) { - if ((def->features[val] = virTristateSwitchTypeFromString(tmp)) == -1) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, - _("unknown state attribute '%s' of feature '%s'"), - tmp, virDomainFeatureTypeToString(val)); - return -1; - } - } else { - def->features[val] = VIR_TRISTATE_SWITCH_ON; - } + case VIR_DOMAIN_FEATURE_SMM: { + virTristateSwitch state = VIR_TRISTATE_SWITCH_ON; + + if (virXMLPropTristateSwitch(nodes[i], "state", + VIR_XML_PROP_NONE, &state) < 0) + return -1; + + def->features[val] = state; break; + } case VIR_DOMAIN_FEATURE_GIC: if ((tmp = virXMLPropString(nodes[i], "version"))) { @@ -18030,20 +18028,16 @@ virDomainFeaturesDefParse(virDomainDef *def, case VIR_DOMAIN_FEATURE_HTM: case VIR_DOMAIN_FEATURE_NESTED_HV: - case VIR_DOMAIN_FEATURE_CCF_ASSIST: - if (!(tmp = virXMLPropString(nodes[i], "state"))) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, - _("missing state attribute '%s' of feature '%s'"), - tmp, virDomainFeatureTypeToString(val)); - return -1; - } - if ((def->features[val] = virTristateSwitchTypeFromString(tmp)) < 0) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, - _("unknown state attribute '%s' of feature '%s'"), - tmp, virDomainFeatureTypeToString(val)); + case VIR_DOMAIN_FEATURE_CCF_ASSIST: { + virTristateSwitch state; + + if (virXMLPropTristateSwitch(nodes[i], "state", + VIR_XML_PROP_REQUIRED, &state) < 0) return -1; - } + + def->features[val] = state; break; + } /* coverity[dead_error_begin] */ case VIR_DOMAIN_FEATURE_LAST: @@ -18054,13 +18048,11 @@ virDomainFeaturesDefParse(virDomainDef *def, if (def->features[VIR_DOMAIN_FEATURE_HYPERV] == VIR_TRISTATE_SWITCH_ON) { int feature; - int value; - xmlNodePtr node = ctxt->node; + virTristateSwitch value; if ((n = virXPathNodeSet("./features/hyperv/*", ctxt, &nodes)) < 0) return -1; for (i = 0; i < n; i++) { - g_autofree char *tmp = NULL; feature = virDomainHypervTypeFromString((const char *)nodes[i]->name); if (feature < 0) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, @@ -18069,23 +18061,9 @@ virDomainFeaturesDefParse(virDomainDef *def, return -1; } - ctxt->node = nodes[i]; - - if (!(tmp = virXMLPropString(nodes[i], "state"))) { - virReportError(VIR_ERR_XML_ERROR, - _("missing 'state' attribute for " - "HyperV Enlightenment feature '%s'"), - nodes[i]->name); - return -1; - } - - if ((value = virTristateSwitchTypeFromString(tmp)) < 0) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, - _("invalid value of state argument " - "for HyperV Enlightenment feature '%s'"), - nodes[i]->name); + if (virXMLPropTristateSwitch(nodes[i], "state", + VIR_XML_PROP_REQUIRED, &value) < 0) return -1; - } def->hyperv_features[feature] = value; @@ -18155,17 +18133,13 @@ virDomainFeaturesDefParse(virDomainDef *def, } } VIR_FREE(nodes); - ctxt->node = node; } if (def->hyperv_features[VIR_DOMAIN_HYPERV_STIMER] == VIR_TRISTATE_SWITCH_ON) { - int value; if ((n = virXPathNodeSet("./features/hyperv/stimer/*", ctxt, &nodes)) < 0) return -1; for (i = 0; i < n; i++) { - g_autofree char *tmp = NULL; - if (STRNEQ((const char *)nodes[i]->name, "direct")) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("unsupported Hyper-V stimer feature: %s"), @@ -18173,34 +18147,21 @@ virDomainFeaturesDefParse(virDomainDef *def, return -1; } - if (!(tmp = virXMLPropString(nodes[i], "state"))) { - virReportError(VIR_ERR_XML_ERROR, - _("missing 'state' attribute for " - "Hyper-V stimer '%s' feature"), "direct"); - return -1; - } - - if ((value = virTristateSwitchTypeFromString(tmp)) < 0) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, - _("invalid value of state argument " - "for Hyper-V stimer '%s' feature"), "direct"); + if (virXMLPropTristateSwitch(nodes[i], "state", + VIR_XML_PROP_REQUIRED, + &def->hyperv_stimer_direct) < 0) return -1; - } - - def->hyperv_stimer_direct = value; } VIR_FREE(nodes); } if (def->features[VIR_DOMAIN_FEATURE_KVM] == VIR_TRISTATE_SWITCH_ON) { int feature; - int value; + virTristateSwitch value; if ((n = virXPathNodeSet("./features/kvm/*", ctxt, &nodes)) < 0) return -1; for (i = 0; i < n; i++) { - g_autofree char *tmp = NULL; - feature = virDomainKVMTypeFromString((const char *)nodes[i]->name); if (feature < 0) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, @@ -18213,21 +18174,10 @@ virDomainFeaturesDefParse(virDomainDef *def, case VIR_DOMAIN_KVM_HIDDEN: case VIR_DOMAIN_KVM_DEDICATED: case VIR_DOMAIN_KVM_POLLCONTROL: - if (!(tmp = virXMLPropString(nodes[i], "state"))) { - virReportError(VIR_ERR_XML_ERROR, - _("missing 'state' attribute for " - "KVM feature '%s'"), - nodes[i]->name); - return -1; - } - - if ((value = virTristateSwitchTypeFromString(tmp)) < 0) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, - _("invalid value of state argument " - "for KVM feature '%s'"), - nodes[i]->name); + if (virXMLPropTristateSwitch(nodes[i], "state", + VIR_XML_PROP_REQUIRED, + &value) < 0) return -1; - } def->kvm_features[feature] = value; break; @@ -18242,14 +18192,13 @@ virDomainFeaturesDefParse(virDomainDef *def, if (def->features[VIR_DOMAIN_FEATURE_XEN] == VIR_TRISTATE_SWITCH_ON) { int feature; - int value; + virTristateSwitch value; g_autofree char *ptval = NULL; if ((n = virXPathNodeSet("./features/xen/*", ctxt, &nodes)) < 0) return -1; for (i = 0; i < n; i++) { - g_autofree char *tmp = NULL; feature = virDomainXenTypeFromString((const char *)nodes[i]->name); if (feature < 0) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, @@ -18258,21 +18207,9 @@ virDomainFeaturesDefParse(virDomainDef *def, return -1; } - if (!(tmp = virXMLPropString(nodes[i], "state"))) { - virReportError(VIR_ERR_XML_ERROR, - _("missing 'state' attribute for " - "Xen feature '%s'"), - nodes[i]->name); - return -1; - } - - if ((value = virTristateSwitchTypeFromString(tmp)) < 0) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, - _("invalid value of state argument " - "for Xen feature '%s'"), - nodes[i]->name); + if (virXMLPropTristateSwitch(nodes[i], "state", + VIR_XML_PROP_REQUIRED, &value) < 0) return -1; - } def->xen_features[feature] = value; @@ -18351,7 +18288,7 @@ virDomainFeaturesDefParse(virDomainDef *def, return -1; for (i = 0; i < n; i++) { - g_autofree char *tmp = NULL; + virTristateSwitch state = VIR_TRISTATE_SWITCH_ON; int val = virDomainProcessCapsFeatureTypeFromString((const char *)nodes[i]->name); if (val < 0) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, @@ -18359,16 +18296,12 @@ virDomainFeaturesDefParse(virDomainDef *def, return -1; } - if ((tmp = virXMLPropString(nodes[i], "state"))) { - if ((def->caps_features[val] = virTristateSwitchTypeFromString(tmp)) == -1) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, - _("unknown state attribute '%s' of feature capability '%s'"), - tmp, virDomainProcessCapsFeatureTypeToString(val)); - return -1; - } - } else { - def->caps_features[val] = VIR_TRISTATE_SWITCH_ON; - } + + if (virXMLPropTristateSwitch(nodes[i], "state", VIR_XML_PROP_NONE, + &state) < 0) + return -1; + + def->caps_features[val] = state; } VIR_FREE(nodes); return 0; -- 2.26.3
On a Tuesday in 2021, Tim Wiederhake wrote:
Signed-off-by: Tim Wiederhake <twiederh@redhat.com> --- src/conf/domain_conf.c | 141 +++++++++++------------------------------ 1 file changed, 37 insertions(+), 104 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
Signed-off-by: Tim Wiederhake <twiederh@redhat.com> --- src/conf/domain_conf.c | 174 +++++++----------- .../qemuxml2argvdata/aarch64-gic-invalid.err | 2 +- 2 files changed, 64 insertions(+), 112 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 4c7affe5af..145b898adb 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -17895,18 +17895,17 @@ virDomainFeaturesDefParse(virDomainDef *def, def->features[val] = VIR_TRISTATE_SWITCH_ON; break; - case VIR_DOMAIN_FEATURE_CAPABILITIES: - if ((tmp = virXMLPropString(nodes[i], "policy"))) { - if ((def->features[val] = virDomainCapabilitiesPolicyTypeFromString(tmp)) == -1) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, - _("unknown policy attribute '%s' of feature '%s'"), - tmp, virDomainFeatureTypeToString(val)); - return -1; - } - } else { - def->features[val] = VIR_TRISTATE_SWITCH_ABSENT; - } + case VIR_DOMAIN_FEATURE_CAPABILITIES: { + virDomainCapabilitiesPolicy policy = VIR_DOMAIN_CAPABILITIES_POLICY_DEFAULT; + + if (virXMLPropEnum(nodes[i], "policy", + virDomainCapabilitiesPolicyTypeFromString, + VIR_XML_PROP_NONE, &policy) < 0) + return -1; + + def->features[val] = policy; break; + } case VIR_DOMAIN_FEATURE_VMCOREINFO: case VIR_DOMAIN_FEATURE_HAP: @@ -17925,44 +17924,29 @@ virDomainFeaturesDefParse(virDomainDef *def, } case VIR_DOMAIN_FEATURE_GIC: - if ((tmp = virXMLPropString(nodes[i], "version"))) { - int gic_version = virGICVersionTypeFromString(tmp); - if (gic_version < 0 || gic_version == VIR_GIC_VERSION_NONE) { - virReportError(VIR_ERR_XML_ERROR, - _("malformed gic version: %s"), tmp); - return -1; - } - def->gic_version = gic_version; - } + if (virXMLPropEnum(nodes[i], "version", virGICVersionTypeFromString, + VIR_XML_PROP_NONZERO, &def->gic_version) < 0) + return -1; + def->features[val] = VIR_TRISTATE_SWITCH_ON; break; - case VIR_DOMAIN_FEATURE_IOAPIC: - tmp = virXMLPropString(nodes[i], "driver"); - if (tmp) { - int value = virDomainIOAPICTypeFromString(tmp); - if (value < 0 || value == VIR_DOMAIN_IOAPIC_NONE) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, - _("Unknown driver mode: %s"), - tmp); - return -1; - } - def->features[val] = value; - } + case VIR_DOMAIN_FEATURE_IOAPIC: { + virDomainIOAPIC driver = VIR_DOMAIN_IOAPIC_NONE; + + if (virXMLPropEnum(nodes[i], "driver", virDomainIOAPICTypeFromString, + VIR_XML_PROP_NONZERO, &driver) < 0) + return -1; + + def->features[val] = driver; break; + } case VIR_DOMAIN_FEATURE_HPT: - tmp = virXMLPropString(nodes[i], "resizing"); - if (tmp) { - int value = virDomainHPTResizingTypeFromString(tmp); - if (value < 0 || value == VIR_DOMAIN_HPT_RESIZING_NONE) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, - _("Unknown HPT resizing setting: %s"), - tmp); - return -1; - } - def->hpt_resizing = (virDomainHPTResizing) value; - } + if (virXMLPropEnum(nodes[i], "resizing", + virDomainHPTResizingTypeFromString, + VIR_XML_PROP_NONZERO, &def->hpt_resizing) < 0) + return -1; if (virParseScaledValue("./features/hpt/maxpagesize", NULL, @@ -17984,47 +17968,38 @@ virDomainFeaturesDefParse(virDomainDef *def, } break; - case VIR_DOMAIN_FEATURE_CFPC: - tmp = virXMLPropString(nodes[i], "value"); - if (tmp) { - int value = virDomainCFPCTypeFromString(tmp); - if (value < 0 || value == VIR_DOMAIN_CFPC_NONE) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, - _("Unknown value: %s"), - tmp); - return -1; - } - def->features[val] = value; - } + case VIR_DOMAIN_FEATURE_CFPC: { + virDomainCFPC value = VIR_DOMAIN_CFPC_NONE; + + if (virXMLPropEnum(nodes[i], "value", virDomainCFPCTypeFromString, + VIR_XML_PROP_NONZERO, &value) < 0) + return -1; + + def->features[val] = value; break; + } - case VIR_DOMAIN_FEATURE_SBBC: - tmp = virXMLPropString(nodes[i], "value"); - if (tmp) { - int value = virDomainSBBCTypeFromString(tmp); - if (value < 0 || value == VIR_DOMAIN_SBBC_NONE) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, - _("Unknown value: %s"), - tmp); - return -1; - } - def->features[val] = value; - } + case VIR_DOMAIN_FEATURE_SBBC: { + virDomainSBBC value = VIR_DOMAIN_SBBC_NONE; + + if (virXMLPropEnum(nodes[i], "value", virDomainSBBCTypeFromString, + VIR_XML_PROP_NONZERO, &value) < 0) + return -1; + + def->features[val] = value; break; + } - case VIR_DOMAIN_FEATURE_IBS: - tmp = virXMLPropString(nodes[i], "value"); - if (tmp) { - int value = virDomainIBSTypeFromString(tmp); - if (value < 0 || value == VIR_DOMAIN_IBS_NONE) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, - _("Unknown value: %s"), - tmp); - return -1; - } - def->features[val] = value; - } + case VIR_DOMAIN_FEATURE_IBS: { + virDomainIBS value = VIR_DOMAIN_IBS_NONE; + + if (virXMLPropEnum(nodes[i], "value", virDomainIBSTypeFromString, + VIR_XML_PROP_NONZERO, &value) < 0) + return -1; + + def->features[val] = value; break; + } case VIR_DOMAIN_FEATURE_HTM: case VIR_DOMAIN_FEATURE_NESTED_HV: @@ -18193,7 +18168,6 @@ virDomainFeaturesDefParse(virDomainDef *def, if (def->features[VIR_DOMAIN_FEATURE_XEN] == VIR_TRISTATE_SWITCH_ON) { int feature; virTristateSwitch value; - g_autofree char *ptval = NULL; if ((n = virXPathNodeSet("./features/xen/*", ctxt, &nodes)) < 0) return -1; @@ -18221,25 +18195,11 @@ virDomainFeaturesDefParse(virDomainDef *def, if (value != VIR_TRISTATE_SWITCH_ON) break; - if ((ptval = virXMLPropString(nodes[i], "mode"))) { - int mode = virDomainXenPassthroughModeTypeFromString(ptval); - - if (mode < 0) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, - _("unsupported mode '%s' for Xen passthrough feature"), - ptval); - return -1; - } - - if (mode != VIR_DOMAIN_XEN_PASSTHROUGH_MODE_SYNC_PT && - mode != VIR_DOMAIN_XEN_PASSTHROUGH_MODE_SHARE_PT) { - virReportError(VIR_ERR_XML_ERROR, "%s", - _("'mode' attribute for Xen feature " - "'passthrough' must be 'sync_pt' or 'share_pt'")); - return -1; - } - def->xen_passthrough_mode = mode; - } + if (virXMLPropEnum(nodes[i], "mode", + virDomainXenPassthroughModeTypeFromString, + VIR_XML_PROP_NONZERO, + &def->xen_passthrough_mode) < 0) + return -1; break; /* coverity[dead_error_begin] */ @@ -18264,24 +18224,16 @@ virDomainFeaturesDefParse(virDomainDef *def, } if (def->features[VIR_DOMAIN_FEATURE_MSRS] == VIR_TRISTATE_SWITCH_ON) { - g_autofree char *tmp = NULL; + virDomainMsrsUnknown unknown; xmlNodePtr node = NULL; if ((node = virXPathNode("./features/msrs", ctxt)) == NULL) return -1; - if (!(tmp = virXMLPropString(node, "unknown"))) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, - _("missing 'unknown' attribute for feature '%s'"), - virDomainFeatureTypeToString(VIR_DOMAIN_FEATURE_MSRS)); + if (virXMLPropEnum(node, "unknown", virDomainMsrsUnknownTypeFromString, + VIR_XML_PROP_REQUIRED, &unknown) < 0) return -1; - } - if ((def->msrs_features[VIR_DOMAIN_MSRS_UNKNOWN] = virDomainMsrsUnknownTypeFromString(tmp)) < 0) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, - _("unknown 'unknown' value '%s'"), - tmp); - return -1; - } + def->msrs_features[VIR_DOMAIN_MSRS_UNKNOWN] = unknown; } if ((n = virXPathNodeSet("./features/capabilities/*", ctxt, &nodes)) < 0) diff --git a/tests/qemuxml2argvdata/aarch64-gic-invalid.err b/tests/qemuxml2argvdata/aarch64-gic-invalid.err index c2e9f4aa3f..18de19f660 100644 --- a/tests/qemuxml2argvdata/aarch64-gic-invalid.err +++ b/tests/qemuxml2argvdata/aarch64-gic-invalid.err @@ -1 +1 @@ -XML error: malformed gic version: none +XML error: Invalid value for attribute 'version' in element 'gic': 'none'. -- 2.26.3
On a Tuesday in 2021, Tim Wiederhake wrote:
Signed-off-by: Tim Wiederhake <twiederh@redhat.com> --- src/conf/domain_conf.c | 174 +++++++----------- .../qemuxml2argvdata/aarch64-gic-invalid.err | 2 +- 2 files changed, 64 insertions(+), 112 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
This strictens the parser to disallow negative values (interpreted as `UINT_MAX + value + 1`) for attribute `bufferCount`. `bufferCount` does not benefit from being referable as e.g. "-7" for requesting 4294967289 buffers, as this value is distinctly out of range for normal use. Signed-off-by: Tim Wiederhake <twiederh@redhat.com> --- src/conf/domain_conf.c | 10 ++-------- 1 file changed, 2 insertions(+), 8 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 145b898adb..66217289d1 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -13455,15 +13455,9 @@ static int virDomainAudioSDLParse(virDomainAudioIOSDL *def, xmlNodePtr node) { - g_autofree char *bufferCount = virXMLPropString(node, "bufferCount"); - - if (bufferCount && - virStrToLong_ui(bufferCount, NULL, 10, - &def->bufferCount) < 0) { - virReportError(VIR_ERR_XML_ERROR, - _("cannot parse 'bufferCount' value '%s'"), bufferCount); + if (virXMLPropUInt(node, "bufferCount", 10, VIR_XML_PROP_NONE, + &def->bufferCount) < 0) return -1; - } return 0; } -- 2.26.3
On a Tuesday in 2021, Tim Wiederhake wrote:
This strictens the parser to disallow negative values (interpreted as `UINT_MAX + value + 1`) for attribute `bufferCount`.
`bufferCount` does not benefit from being referable as e.g. "-7" for requesting 4294967289 buffers, as this value is distinctly out of range for normal use.
Signed-off-by: Tim Wiederhake <twiederh@redhat.com> --- src/conf/domain_conf.c | 10 ++-------- 1 file changed, 2 insertions(+), 8 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
On a Tuesday in 2021, Tim Wiederhake wrote:
For background, see https://listman.redhat.com/archives/libvir-list/2021-April/msg00668.html
V1: https://listman.redhat.com/archives/libvir-list/2021-April/msg01184.html
Changes since V1: * Split up "virDomainFeaturesDefParse" * Added missing VIR_XML_PROP_NONZERO * Some line break changes * Mentioned printf %s NULL fix in commit message * Improved justification for parser strictening
Tim Wiederhake (6): virDomainGraphicsDefParseXMLSpice: Use virXMLProp*
virDomainSoundDefParseXML: Use virXMLProp* virDomainFeaturesDefParse: Use virXMLPropUInt virDomainFeaturesDefParse: Use virXMLPropTristateSwitch virDomainFeaturesDefParse: Use virXMLPropEnum virDomainAudioSDLParse: Use virXMLProp*
I've pushed patches 2-6. Jano
src/conf/domain_conf.c | 609 +++++------------- .../qemuxml2argvdata/aarch64-gic-invalid.err | 2 +- 2 files changed, 170 insertions(+), 441 deletions(-)
-- 2.26.3
participants (2)
-
Ján Tomko -
Tim Wiederhake