[libvirt PATCH 00/10] Refactor more XML parsing boilerplate code, part VI
For background, see https://listman.redhat.com/archives/libvir-list/2021-April/msg00668.html Tim Wiederhake (10): virDomainGraphicsDefParseXMLSDL: Use virXMLProp* virDomainGraphicsDefParseXMLDesktop: Use virXMLProp* virDomainGraphicsDefParseXMLSpice: Use virXMLProp* virDomainGraphicsDefParseXML: Use virXMLProp* virDomainSoundDef: Change type of model to virDomainSoundModel virDomainSoundDefParseXML: Use virXMLProp* virDomainDef: Change type of hyperv_stimer_direct to virTristateSwitch virDomainDef: Change type of xen_passthrough_mode to virDomainXenPassthroughMode virDomainFeaturesDefParse: Use virXMLProp* virDomainAudioSDLParse: Use virXMLProp* src/conf/domain_conf.c | 683 +++++------------- src/conf/domain_conf.h | 6 +- src/qemu/qemu_command.c | 2 +- src/qemu/qemu_domain_address.c | 2 +- src/qemu/qemu_validate.c | 2 +- .../qemuxml2argvdata/aarch64-gic-invalid.err | 2 +- 6 files changed, 206 insertions(+), 491 deletions(-) -- 2.26.3
Signed-off-by: Tim Wiederhake <twiederh@redhat.com> --- src/conf/domain_conf.c | 35 ++++++++--------------------------- 1 file changed, 8 insertions(+), 27 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index a72d58f488..5b0e90f234 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -12813,42 +12813,23 @@ virDomainGraphicsDefParseXMLSDL(virDomainGraphicsDef *def, xmlXPathContextPtr ctxt) { VIR_XPATH_NODE_AUTORESTORE(ctxt) - int enableVal; xmlNodePtr glNode; - g_autofree char *fullscreen = virXMLPropString(node, "fullscreen"); - g_autofree char *enable = NULL; + virTristateBool fullscreen = VIR_TRISTATE_BOOL_NO; ctxt->node = node; - if (fullscreen != NULL) { - if (virStringParseYesNo(fullscreen, &def->data.sdl.fullscreen) < 0) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("unknown fullscreen value '%s'"), fullscreen); - return -1; - } - } else { - def->data.sdl.fullscreen = false; - } + if (virXMLPropTristateBool(node, "fullscreen", VIR_XML_PROP_NONE, + &fullscreen) < 0) + return -1; + def->data.sdl.fullscreen = fullscreen == VIR_TRISTATE_BOOL_YES; def->data.sdl.xauth = virXMLPropString(node, "xauth"); def->data.sdl.display = virXMLPropString(node, "display"); - glNode = virXPathNode("./gl", ctxt); - if (glNode) { - enable = virXMLPropString(glNode, "enable"); - if (!enable) { - virReportError(VIR_ERR_XML_ERROR, "%s", - _("sdl gl element missing enable")); + if ((glNode = virXPathNode("./gl", ctxt))) { + if (virXMLPropTristateBool(glNode, "enable", VIR_XML_PROP_REQUIRED, + &def->data.sdl.gl) < 0) return -1; - } - - enableVal = virTristateBoolTypeFromString(enable); - if (enableVal < 0) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, - _("unknown enable value '%s'"), enable); - return -1; - } - def->data.sdl.gl = enableVal; } return 0; -- 2.26.3
Signed-off-by: Tim Wiederhake <twiederh@redhat.com> --- src/conf/domain_conf.c | 15 +++++---------- 1 file changed, 5 insertions(+), 10 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 5b0e90f234..d57450b3c0 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -12885,18 +12885,13 @@ static int virDomainGraphicsDefParseXMLDesktop(virDomainGraphicsDef *def, xmlNodePtr node) { - g_autofree char *fullscreen = virXMLPropString(node, "fullscreen"); + virTristateBool fullscreen = VIR_TRISTATE_BOOL_NO; - if (fullscreen != NULL) { - if (virStringParseYesNo(fullscreen, &def->data.desktop.fullscreen) < 0) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("unknown fullscreen value '%s'"), fullscreen); - return -1; - } - } else { - def->data.desktop.fullscreen = false; - } + if (virXMLPropTristateBool(node, "fullscreen", VIR_XML_PROP_NONE, + &fullscreen) < 0) + return -1; + def->data.desktop.fullscreen = fullscreen == VIR_TRISTATE_BOOL_YES; def->data.desktop.display = virXMLPropString(node, "display"); return 0; -- 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 d57450b3c0..9aba2edf0a 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_REQUIRED, &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_REQUIRED, &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_REQUIRED, + &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, &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 Fri, Apr 23, 2021 at 17:39:16 +0200, 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 d57450b3c0..9aba2edf0a 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -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;
I'm not entirely sure that all static analyzers will be able to see that 'name' and '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)
... will not be uninitalized if virXMLPropEnum returns since VIR_XML_PROP_REQUIRED is used, but the code is correct.
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_REQUIRED, &compression) < 0)
The removed function was checking '<= 0' thus VIR_XML_PROP_NONZERO is missing.
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_REQUIRED, &compression) < 0) return -1;
Here too.
- }
- 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)
... and here too.
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_REQUIRED, + &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, &mode) < 0)
... and here as well.
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");
Don't break the line here. 80 cols is not a hard limit.
- 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
Signed-off-by: Tim Wiederhake <twiederh@redhat.com> --- src/conf/domain_conf.c | 16 ++-------------- 1 file changed, 2 insertions(+), 14 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 9aba2edf0a..f599d1afe7 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -13097,25 +13097,13 @@ virDomainGraphicsDefParseXML(virDomainXMLOption *xmlopt, unsigned int flags) { virDomainGraphicsDef *def; - int typeVal; - g_autofree char *type = NULL; if (!(def = virDomainGraphicsDefNew(xmlopt))) return NULL; - type = virXMLPropString(node, "type"); - if (!type) { - virReportError(VIR_ERR_INTERNAL_ERROR, - "%s", _("missing graphics device type")); - goto error; - } - - if ((typeVal = virDomainGraphicsTypeFromString(type)) < 0) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, - _("unknown graphics device type '%s'"), type); + if (virXMLPropEnum(node, "type", virDomainGraphicsTypeFromString, + VIR_XML_PROP_REQUIRED, &def->type) < 0) goto error; - } - def->type = typeVal; switch (def->type) { case VIR_DOMAIN_GRAPHICS_TYPE_VNC: -- 2.26.3
Signed-off-by: Tim Wiederhake <twiederh@redhat.com> --- src/conf/domain_conf.c | 4 +++- src/conf/domain_conf.h | 2 +- src/qemu/qemu_command.c | 2 +- src/qemu/qemu_domain_address.c | 2 +- src/qemu/qemu_validate.c | 2 +- 5 files changed, 7 insertions(+), 5 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index f599d1afe7..50ddb293ed 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -13174,17 +13174,19 @@ 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 ((def->model = virDomainSoundModelTypeFromString(model)) < 0) { + if ((modelval = virDomainSoundModelTypeFromString(model)) < 0) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("unknown sound model '%s'"), model); goto error; } + def->model = modelval; if (virDomainSoundModelSupportsCodecs(def)) { int ncodecs; diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 4838687edf..ede80ac322 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -1449,7 +1449,7 @@ struct _virDomainSoundCodecDef { }; struct _virDomainSoundDef { - int model; + virDomainSoundModel model; virDomainDeviceInfo info; size_t ncodecs; diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index be93182092..d7f1c715b6 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -4133,7 +4133,7 @@ qemuBuildSoundDevStr(const virDomainDef *def, const char *model = NULL; /* Hack for devices with different names in QEMU and libvirt */ - switch ((virDomainSoundModel) sound->model) { + switch (sound->model) { case VIR_DOMAIN_SOUND_MODEL_ES1370: model = "ES1370"; break; diff --git a/src/qemu/qemu_domain_address.c b/src/qemu/qemu_domain_address.c index a73f30ddcb..e66efb3d1f 100644 --- a/src/qemu/qemu_domain_address.c +++ b/src/qemu/qemu_domain_address.c @@ -759,7 +759,7 @@ qemuDomainDeviceCalculatePCIConnectFlags(virDomainDeviceDef *dev, } case VIR_DOMAIN_DEVICE_SOUND: - switch ((virDomainSoundModel) dev->data.sound->model) { + switch (dev->data.sound->model) { case VIR_DOMAIN_SOUND_MODEL_ES1370: case VIR_DOMAIN_SOUND_MODEL_AC97: case VIR_DOMAIN_SOUND_MODEL_ICH6: diff --git a/src/qemu/qemu_validate.c b/src/qemu/qemu_validate.c index 255d653118..774426bceb 100644 --- a/src/qemu/qemu_validate.c +++ b/src/qemu/qemu_validate.c @@ -4385,7 +4385,7 @@ qemuValidateDomainDeviceDefSound(virDomainSoundDef *sound, { size_t i; - switch ((virDomainSoundModel) sound->model) { + switch (sound->model) { case VIR_DOMAIN_SOUND_MODEL_USB: if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_OBJECT_USB_AUDIO)) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", -- 2.26.3
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 50ddb293ed..867a83a605 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 Fri, Apr 23, 2021 at 17:39:19 +0200, Tim Wiederhake wrote:
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 50ddb293ed..867a83a605 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);
This also fixes use of NULL string with printf's %s modifier.
+ 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)
The commit message lacks justification of removing the support for wrapping of negative numbers.
goto error; - } }
if (virDomainDeviceInfoParseXML(xmlopt, node, ctxt, &def->info, flags) < 0) -- 2.26.3
Signed-off-by: Tim Wiederhake <twiederh@redhat.com> --- src/conf/domain_conf.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index ede80ac322..61f2d10d89 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -2755,7 +2755,7 @@ struct _virDomainDef { int xen_features[VIR_DOMAIN_XEN_LAST]; int xen_passthrough_mode; unsigned int hyperv_spinlocks; - int hyperv_stimer_direct; + virTristateSwitch hyperv_stimer_direct; virGICVersion gic_version; virDomainHPTResizing hpt_resizing; unsigned long long hpt_maxpagesize; /* Stored in KiB */ -- 2.26.3
Signed-off-by: Tim Wiederhake <twiederh@redhat.com> --- src/conf/domain_conf.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 61f2d10d89..85c318d027 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -2753,7 +2753,7 @@ struct _virDomainDef { int kvm_features[VIR_DOMAIN_KVM_LAST]; int msrs_features[VIR_DOMAIN_MSRS_LAST]; int xen_features[VIR_DOMAIN_XEN_LAST]; - int xen_passthrough_mode; + virDomainXenPassthroughMode xen_passthrough_mode; unsigned int hyperv_spinlocks; virTristateSwitch hyperv_stimer_direct; virGICVersion gic_version; -- 2.26.3
Signed-off-by: Tim Wiederhake <twiederh@redhat.com> --- src/conf/domain_conf.c | 333 +++++++----------- .../qemuxml2argvdata/aarch64-gic-invalid.err | 2 +- 2 files changed, 119 insertions(+), 216 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 867a83a605..bff17057af 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -17858,6 +17858,7 @@ virDomainFeaturesDefParse(virDomainDef *def, g_autofree xmlNodePtr *nodes = NULL; size_t i; int n; + int rc; if ((n = virXPathNodeSet("./features/*", ctxt, &nodes)) < 0) return -1; @@ -17895,76 +17896,67 @@ 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; + + def->features[val] = VIR_TRISTATE_SWITCH_ABSENT; + + if ((rc = virXMLPropEnum(nodes[i], "policy", + virDomainCapabilitiesPolicyTypeFromString, + VIR_XML_PROP_NONE, &policy)) < 0) + return -1; + + if (rc == 1) + def->features[val] = policy; break; + } case VIR_DOMAIN_FEATURE_VMCOREINFO: case VIR_DOMAIN_FEATURE_HAP: 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; + + def->features[val] = VIR_TRISTATE_SWITCH_ON; + + if ((rc = virXMLPropTristateSwitch(nodes[i], "state", + VIR_XML_PROP_NONE, &state)) < 0) + return -1; + + if (rc == 1) + def->features[val] = state; break; + } 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; - } def->features[val] = VIR_TRISTATE_SWITCH_ON; + + if (virXMLPropEnum(nodes[i], "version", + virGICVersionTypeFromString, + VIR_XML_PROP_NONZERO, &def->gic_version) < 0) + return -1; 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; + + if ((rc = virXMLPropEnum(nodes[i], "driver", + virDomainIOAPICTypeFromString, + VIR_XML_PROP_NONZERO, &driver)) < 0) + return -1; + + if (rc == 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, @@ -17986,64 +17978,57 @@ 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; - } + case VIR_DOMAIN_FEATURE_CFPC: { + virDomainCFPC value; + + if ((rc = virXMLPropEnum(nodes[i], "value", + virDomainCFPCTypeFromString, + VIR_XML_PROP_NONZERO, &value)) < 0) + return -1; + + if (rc == 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; - } + case VIR_DOMAIN_FEATURE_SBBC: { + virDomainSBBC value; + + if ((rc = virXMLPropEnum(nodes[i], "value", + virDomainSBBCTypeFromString, + VIR_XML_PROP_NONZERO, &value)) < 0) + return -1; + + if (rc == 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; - } + case VIR_DOMAIN_FEATURE_IBS: { + virDomainIBS value; + + if ((rc = virXMLPropEnum(nodes[i], "value", + virDomainIBSTypeFromString, + VIR_XML_PROP_NONZERO, &value)) < 0) + return -1; + + if (rc == 1) def->features[val] = value; - } break; + } 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 +18039,12 @@ virDomainFeaturesDefParse(virDomainDef *def, if (def->features[VIR_DOMAIN_FEATURE_HYPERV] == VIR_TRISTATE_SWITCH_ON) { int feature; - int value; xmlNodePtr node = ctxt->node; if ((n = virXPathNodeSet("./features/hyperv/*", ctxt, &nodes)) < 0) return -1; for (i = 0; i < n; i++) { - g_autofree char *tmp = NULL; + virTristateSwitch value; feature = virDomainHypervTypeFromString((const char *)nodes[i]->name); if (feature < 0) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, @@ -18071,21 +18055,9 @@ virDomainFeaturesDefParse(virDomainDef *def, 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; @@ -18108,12 +18080,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", @@ -18161,13 +18131,10 @@ virDomainFeaturesDefParse(virDomainDef *def, } 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"), @@ -18175,33 +18142,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; if ((n = virXPathNodeSet("./features/kvm/*", ctxt, &nodes)) < 0) return -1; for (i = 0; i < n; i++) { - g_autofree char *tmp = NULL; + virTristateSwitch value; + int feature; feature = virDomainKVMTypeFromString((const char *)nodes[i]->name); if (feature < 0) { @@ -18215,21 +18170,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); + if (virXMLPropTristateSwitch(nodes[i], "state", + VIR_XML_PROP_REQUIRED, + &value) < 0) return -1; - } - - if ((value = virTristateSwitchTypeFromString(tmp)) < 0) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, - _("invalid value of state argument " - "for KVM feature '%s'"), - nodes[i]->name); - return -1; - } def->kvm_features[feature] = value; break; @@ -18243,15 +18187,12 @@ virDomainFeaturesDefParse(virDomainDef *def, } if (def->features[VIR_DOMAIN_FEATURE_XEN] == VIR_TRISTATE_SWITCH_ON) { - int feature; - int 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; + virTristateSwitch value; + int feature; feature = virDomainXenTypeFromString((const char *)nodes[i]->name); if (feature < 0) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, @@ -18260,21 +18201,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; @@ -18286,25 +18215,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] */ @@ -18329,31 +18244,23 @@ 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) 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, @@ -18361,16 +18268,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; 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 Fri, Apr 23, 2021 at 17:39:22 +0200, Tim Wiederhake wrote: Function-level granularity for a function this massive seems to be too coarse.
Signed-off-by: Tim Wiederhake <twiederh@redhat.com> --- src/conf/domain_conf.c | 333 +++++++----------- .../qemuxml2argvdata/aarch64-gic-invalid.err | 2 +- 2 files changed, 119 insertions(+), 216 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 867a83a605..bff17057af 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -17858,6 +17858,7 @@ virDomainFeaturesDefParse(virDomainDef *def, g_autofree xmlNodePtr *nodes = NULL; size_t i; int n; + int rc;
if ((n = virXPathNodeSet("./features/*", ctxt, &nodes)) < 0) return -1; @@ -17895,76 +17896,67 @@ 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; + + def->features[val] = VIR_TRISTATE_SWITCH_ABSENT;
While faithfully reimplementing the code, this doesn't make sense. You should initialize 'policy' properly to the default value ...
+ + if ((rc = virXMLPropEnum(nodes[i], "policy", + virDomainCapabilitiesPolicyTypeFromString, + VIR_XML_PROP_NONE, &policy)) < 0) + return -1;
... and then assign it here to def->features[val] without any check. It makes no sense to check 'rc' at all, since virXMLPropEnum doesn't touch the value stored in the pointer passed in the last argument if the property is missing.
+ + if (rc == 1) + def->features[val] = policy; break; + }
case VIR_DOMAIN_FEATURE_VMCOREINFO: case VIR_DOMAIN_FEATURE_HAP: 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; + + def->features[val] = VIR_TRISTATE_SWITCH_ON; + + if ((rc = virXMLPropTristateSwitch(nodes[i], "state", + VIR_XML_PROP_NONE, &state)) < 0) + return -1; + + if (rc == 1) + def->features[val] = state; break;
Same as above here and in all other refactored VIR_DOMAIN_FEATURE_* below ... (snipped). [...]
@@ -18054,13 +18039,12 @@ virDomainFeaturesDefParse(virDomainDef *def,
if (def->features[VIR_DOMAIN_FEATURE_HYPERV] == VIR_TRISTATE_SWITCH_ON) { int feature; - int value; xmlNodePtr node = ctxt->node; if ((n = virXPathNodeSet("./features/hyperv/*", ctxt, &nodes)) < 0) return -1;
for (i = 0; i < n; i++) { - g_autofree char *tmp = NULL; + virTristateSwitch value; feature = virDomainHypervTypeFromString((const char *)nodes[i]->name); if (feature < 0) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, @@ -18071,21 +18055,9 @@ virDomainFeaturesDefParse(virDomainDef *def,
ctxt->node = nodes[i];
[1] (see below)
- 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;
@@ -18108,12 +18080,10 @@ virDomainFeaturesDefParse(virDomainDef *def, if (value != VIR_TRISTATE_SWITCH_ON) break;
- if (virXPathUInt("string(./@retries)", ctxt, - &def->hyperv_spinlocks) < 0) {
This removes the only piece of code which uses the XPath context (ctxt) for parsing in this whole loop, so [1] and the resetting of ctxt at the end of the loop should be removed too.
- 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",
[...]
This strictens the parser to disallow negative values (interpreted as `UINT_MAX + value + 1`) for attribute `bufferCount`. 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 bff17057af..12bdc12f54 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 Fri, Apr 23, 2021 at 17:39:23 +0200, Tim Wiederhake wrote:
This strictens the parser to disallow negative values (interpreted as `UINT_MAX + value + 1`) for attribute `bufferCount`.
A change like this requires justification, but you've provided only an explanation.
Signed-off-by: Tim Wiederhake <twiederh@redhat.com> --- src/conf/domain_conf.c | 10 ++-------- 1 file changed, 2 insertions(+), 8 deletions(-)
On Fri, Apr 23, 2021 at 05:39:23PM +0200, Tim Wiederhake wrote:
This strictens the parser to disallow negative values (interpreted as `UINT_MAX + value + 1`) for attribute `bufferCount`.
I don't get what's different here - we were already using virStrToLong_ui to get positive values.
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 bff17057af..12bdc12f54 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; }
Why is this only modifying parsing of this one attribute for the SDL audio backend, and not the other identical code patterns for other audi backends Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
On Mon, Apr 26, 2021 at 12:48:47 +0100, Daniel P. Berrangé wrote:
On Fri, Apr 23, 2021 at 05:39:23PM +0200, Tim Wiederhake wrote:
This strictens the parser to disallow negative values (interpreted as `UINT_MAX + value + 1`) for attribute `bufferCount`.
I don't get what's different here - we were already using virStrToLong_ui to get positive values.
virStrToLong_ui accepts -1 as valid input and wraps around to the max value. virStrToLong_uip is a version that doesn't have this weird behaviour. We actually document and rely on the wraparound in certain cases as shortcuts for MAX value, thus any change from _ui to _uip equivalent must be properly justified as not making sense/not being documented.
On Fri, Apr 23, 2021 at 17:39:13 +0200, Tim Wiederhake wrote:
For background, see https://listman.redhat.com/archives/libvir-list/2021-April/msg00668.html
Tim Wiederhake (10): virDomainGraphicsDefParseXMLSDL: Use virXMLProp* virDomainGraphicsDefParseXMLDesktop: Use virXMLProp* virDomainGraphicsDefParseXMLSpice: Use virXMLProp* virDomainGraphicsDefParseXML: Use virXMLProp* virDomainSoundDef: Change type of model to virDomainSoundModel virDomainSoundDefParseXML: Use virXMLProp* virDomainDef: Change type of hyperv_stimer_direct to virTristateSwitch virDomainDef: Change type of xen_passthrough_mode to virDomainXenPassthroughMode virDomainFeaturesDefParse: Use virXMLProp* virDomainAudioSDLParse: Use virXMLProp*
Patches which I've didn't reply to have been Reviewed-by: Peter Krempa <pkrempa@redhat.com> and will be pushed shortly.
participants (3)
-
Daniel P. Berrangé -
Peter Krempa -
Tim Wiederhake