Signed-off-by: Kristina Hanicova <khanicov@redhat.com>
---
src/conf/domain_conf.c | 303 ++++++++++++++++-------------------------
1 file changed, 114 insertions(+), 189 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 2511778b15..206816d76f 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -12690,12 +12690,29 @@ virDomainGraphicsDefParseXMLSpice(virDomainGraphicsDef *def,
xmlXPathContextPtr ctxt,
unsigned int flags)
{
- xmlNodePtr cur;
int defaultModeVal;
+ int nameval;
+ g_autofree xmlNodePtr *node_list = NULL;
+ int n = 0;
+ size_t i = 0;
+ int value = 0;
g_autofree char *port = virXMLPropString(node, "port");
g_autofree char *tlsPort = virXMLPropString(node, "tlsPort");
g_autofree char *autoport = virXMLPropString(node, "autoport");
g_autofree char *defaultMode = virXMLPropString(node, "defaultMode");
+ g_autofree char *compression = NULL;
+ g_autofree char *jpeg_compression = NULL;
+ g_autofree char *zlib_compression = NULL;
+ g_autofree char *playback_compression = NULL;
+ g_autofree char *streaming_mode = NULL;
+ g_autofree char *copypaste = NULL;
+ g_autofree char *filetransfer_enable = NULL;
+ g_autofree char *gl_enable = NULL;
+ g_autofree char *mouse_mode = NULL;
+ g_autofree char *rendernode = NULL;
+ VIR_XPATH_NODE_AUTORESTORE(ctxt)
+
+ ctxt->node = node;
if (virDomainGraphicsListensParseXML(def, node, ctxt, flags) < 0)
return -1;
@@ -12751,207 +12768,115 @@ virDomainGraphicsDefParseXMLSpice(virDomainGraphicsDef *def,
def->type) < 0)
return -1;
- cur = node->children;
- while (cur != NULL) {
- if (cur->type == XML_ELEMENT_NODE) {
- if (virXMLNodeNameEqual(cur, "channel")) {
- int nameval, modeval;
- g_autofree char *name = NULL;
- g_autofree char *mode = NULL;
-
- name = virXMLPropString(cur, "name");
- mode = virXMLPropString(cur, "mode");
-
- if (!name || !mode) {
- virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
- _("spice channel missing name/mode"));
- return -1;
- }
-
- if ((nameval = virDomainGraphicsSpiceChannelNameTypeFromString(name)) < 0) {
- virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
- _("unknown spice channel name %s"),
- name);
- return -1;
- }
- if ((modeval = virDomainGraphicsSpiceChannelModeTypeFromString(mode)) < 0) {
- virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
- _("unknown spice channel mode %s"),
- mode);
- return -1;
- }
-
- def->data.spice.channels[nameval] = modeval;
- } else if (virXMLNodeNameEqual(cur, "image")) {
- int compressionVal;
- g_autofree char *compression = virXMLPropString(cur, "compression");
-
- if (!compression) {
- virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
- _("spice image missing compression"));
- return -1;
- }
-
- if ((compressionVal =
- virDomainGraphicsSpiceImageCompressionTypeFromString(compression)) <= 0) {
- virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
- _("unknown spice image compression %s"),
- compression);
- return -1;
- }
-
- def->data.spice.image = compressionVal;
- } else if (virXMLNodeNameEqual(cur, "jpeg")) {
- int compressionVal;
- g_autofree char *compression = virXMLPropString(cur, "compression");
-
- if (!compression) {
- virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
- _("spice jpeg missing compression"));
- return -1;
- }
-
- if ((compressionVal =
- virDomainGraphicsSpiceJpegCompressionTypeFromString(compression)) <= 0) {
- virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
- _("unknown spice jpeg compression %s"),
- compression);
- return -1;
- }
-
- def->data.spice.jpeg = compressionVal;
- } else if (virXMLNodeNameEqual(cur, "zlib")) {
- int compressionVal;
- g_autofree char *compression = virXMLPropString(cur, "compression");
-
- if (!compression) {
- virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
- _("spice zlib missing compression"));
- return -1;
- }
-
- if ((compressionVal =
- virDomainGraphicsSpiceZlibCompressionTypeFromString(compression)) <= 0) {
- virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
- _("unknown spice zlib compression %s"),
- compression);
- return -1;
- }
-
- def->data.spice.zlib = compressionVal;
- } else if (virXMLNodeNameEqual(cur, "playback")) {
- int compressionVal;
- g_autofree char *compression = virXMLPropString(cur, "compression");
-
- if (!compression) {
- virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
- _("spice playback missing compression"));
- return -1;
- }
-
- if ((compressionVal =
- virTristateSwitchTypeFromString(compression)) <= 0) {
- virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
- _("unknown spice playback compression"));
- return -1;
- }
-
- def->data.spice.playback = compressionVal;
- } else if (virXMLNodeNameEqual(cur, "streaming")) {
- int modeVal;
- g_autofree char *mode = virXMLPropString(cur, "mode");
-
- if (!mode) {
- virReportError(VIR_ERR_XML_ERROR, "%s",
- _("spice streaming missing mode"));
- return -1;
- }
- if ((modeVal =
- virDomainGraphicsSpiceStreamingModeTypeFromString(mode)) <= 0) {
- virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
- _("unknown spice streaming mode"));
- return -1;
- }
-
- def->data.spice.streaming = modeVal;
- } else if (virXMLNodeNameEqual(cur, "clipboard")) {
- int copypasteVal;
- g_autofree char *copypaste = virXMLPropString(cur, "copypaste");
-
- if (!copypaste) {
- virReportError(VIR_ERR_XML_ERROR, "%s",
- _("spice clipboard missing copypaste"));
- return -1;
- }
+ if ((n = virXPathNodeSet("./channel", ctxt, &node_list)) < 0)
+ return -1;
- if ((copypasteVal =
- virTristateBoolTypeFromString(copypaste)) <= 0) {
- virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
- _("unknown copypaste value '%s'"), copypaste);
- return -1;
- }
+ for (i = 0; i < n; i++) {
+ g_autofree char *name = virXMLPropString(node_list[i], "name");
+ g_autofree char *mode = virXMLPropString(node_list[i], "mode");
- def->data.spice.copypaste = copypasteVal;
- } else if (virXMLNodeNameEqual(cur, "filetransfer")) {
- int enableVal;
- g_autofree char *enable = virXMLPropString(cur, "enable");
+ if ((nameval = virDomainGraphicsSpiceChannelNameTypeFromString(name)) < 0) {
+ virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+ _("unknown spice channel name %s"), NULLSTR(name));
+ return -1;
+ }
+ if ((value = virDomainGraphicsSpiceChannelModeTypeFromString(mode)) < 0) {
+ virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+ _("unknown spice channel mode %s"), NULLSTR(mode));
+ return -1;
+ }
+ def->data.spice.channels[nameval] = value;
+ }
- if (!enable) {
- virReportError(VIR_ERR_XML_ERROR, "%s",
- _("spice filetransfer missing enable"));
- return -1;
- }
+ if ((compression = virXPathString("string(./image/@compression)", ctxt))) {
+ if ((value =
+ virDomainGraphicsSpiceImageCompressionTypeFromString(compression)) <= 0) {
+ virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+ _("unknown spice image compression %s"), compression);
+ return -1;
+ }
+ def->data.spice.image = value;
+ }
- if ((enableVal =
- virTristateBoolTypeFromString(enable)) <= 0) {
- virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
- _("unknown enable value '%s'"), enable);
- return -1;
- }
+ if ((jpeg_compression = virXPathString("string(./jpeg/@compression)", ctxt))) {
+ if ((value =
+ virDomainGraphicsSpiceJpegCompressionTypeFromString(jpeg_compression)) <= 0) {
+ virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+ _("unknown spice jpeg compression %s"),
+ jpeg_compression);
+ return -1;
+ }
+ def->data.spice.jpeg = value;
+ }
- def->data.spice.filetransfer = enableVal;
- } else if (virXMLNodeNameEqual(cur, "gl")) {
- int enableVal;
- g_autofree char *enable = virXMLPropString(cur, "enable");
- g_autofree char *rendernode = virXMLPropString(cur, "rendernode");
+ if ((zlib_compression = virXPathString("string(./zlib/@compression)", ctxt))) {
+ if ((value =
+ virDomainGraphicsSpiceZlibCompressionTypeFromString(zlib_compression)) <= 0) {
+ virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+ _("unknown spice zlib compression %s"),
+ zlib_compression);
+ return -1;
+ }
+ def->data.spice.zlib = value;
+ }
- if (!enable) {
- virReportError(VIR_ERR_XML_ERROR, "%s",
- _("spice gl element missing enable"));
- return -1;
- }
+ if ((playback_compression = virXPathString("string(./playback/@compression)", ctxt))) {
+ if ((value = virTristateSwitchTypeFromString(playback_compression)) <= 0) {
+ virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
+ _("unknown spice playback compression"));
+ return -1;
+ }
+ def->data.spice.playback = value;
+ }
- if ((enableVal =
- virTristateBoolTypeFromString(enable)) <= 0) {
- virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
- _("unknown enable value '%s'"), enable);
- return -1;
- }
+ if ((streaming_mode = virXPathString("string(./streaming/@mode)", ctxt))) {
+ if ((value =
+ virDomainGraphicsSpiceStreamingModeTypeFromString(streaming_mode)) <= 0) {
+ virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
+ _("unknown spice streaming mode"));
+ return -1;
+ }
+ def->data.spice.streaming = value;
+ }
- def->data.spice.gl = enableVal;
- def->data.spice.rendernode = g_steal_pointer(&rendernode);
+ if ((copypaste = virXPathString("string(./clipboard/@copypaste)", ctxt))) {
+ if ((value = virTristateBoolTypeFromString(copypaste)) <= 0) {
+ virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+ _("unknown copypaste value '%s'"), copypaste);
+ return -1;
+ }
+ def->data.spice.copypaste = value;
+ }
- } else if (virXMLNodeNameEqual(cur, "mouse")) {
- int modeVal;
- g_autofree char *mode = virXMLPropString(cur, "mode");
+ if ((filetransfer_enable = virXPathString("string(./filetransfer/@enable)", ctxt))) {
+ if ((value = virTristateBoolTypeFromString(filetransfer_enable)) <= 0) {
+ virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+ _("unknown enable value '%s'"),
+ filetransfer_enable);
+ return -1;
+ }
+ def->data.spice.filetransfer = value;
+ }
- if (!mode) {
- virReportError(VIR_ERR_XML_ERROR, "%s",
- _("spice mouse missing mode"));
- return -1;
- }
+ if ((gl_enable = virXPathString("string(./gl/@enable)", ctxt))) {
+ rendernode = virXPathString("string(./gl/@rendernode)", ctxt);
- if ((modeVal = virDomainGraphicsSpiceMouseModeTypeFromString(mode)) <= 0) {
- virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
- _("unknown mouse mode value '%s'"),
- mode);
- return -1;
- }
+ if ((value = virTristateBoolTypeFromString(gl_enable)) <= 0) {
+ virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+ _("unknown enable value '%s'"), gl_enable);
+ return -1;
+ }
+ def->data.spice.gl = value;
+ def->data.spice.rendernode = g_steal_pointer(&rendernode);
+ }
- def->data.spice.mousemode = modeVal;
- }
+ if ((mouse_mode = virXPathString("string(./mouse/@mode)", ctxt))) {
+ if ((value = virDomainGraphicsSpiceMouseModeTypeFromString(mouse_mode)) <= 0) {
+ virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+ _("unknown mouse mode value '%s'"), mouse_mode);
+ return -1;
}
- cur = cur->next;
+ def->data.spice.mousemode = value;
}
return 0;
--
2.30.2
On Tue, May 04, 2021 at 01:40:10PM +0200, Kristina Hanicova wrote:
> - } else if (virXMLNodeNameEqual(cur, "playback")) {
> - int compressionVal;
> - g_autofree char *compression = virXMLPropString(cur, "compression");
> -
> - if (!compression) {
> - virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> - _("spice playback missing compression"));
> - return -1;
> - }
> -
> - if ((compressionVal =
> - virTristateSwitchTypeFromString(compression)) <= 0) {
> - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> - _("unknown spice playback compression"));
> - return -1;
> - }
> -
> - def->data.spice.playback = compressionVal;
> + if ((playback_compression = virXPathString("string(./playback/@compression)", ctxt))) {
> + if ((value = virTristateSwitchTypeFromString(playback_compression)) <= 0) {
> + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> + _("unknown spice playback compression"));
> + return -1;
> + }
> + def->data.spice.playback = value;
> + }
Apologies for the thread necromancy :)
If I'm reading the change above correctly, then the presence of the
compression property of the <playback> element has gone from being
required, with an error being raised and the function terminating
early if it's not there, to being parsed if found and ignored
otherwise.
Tim later restored the original behavior in
commit bb94b3d28db909d43d83b3f2ab73aa3f881b5c95
Author: Tim Wiederhake <twiederh@redhat.com>
Date: Wed May 5 12:55:48 2021 +0200
virDomainGraphicsDefParseXMLSpice: Use virXMLProp*
Signed-off-by: Tim Wiederhake <twiederh@redhat.com>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
with the hunk
> - if ((playback_compression = virXPathString("string(./playback/@compression)", ctxt))) {
> - if ((value = virTristateSwitchTypeFromString(playback_compression)) <= 0) {
> - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> - _("unknown spice playback compression"));
> + if ((cur = virXPathNode("./playback", ctxt))) {
> + if (virXMLPropTristateSwitch(cur, "compression",
> + VIR_XML_PROP_REQUIRED,
> + &def->data.spice.playback) < 0)
> return -1;
> - }
> - def->data.spice.playback = value;
> }
and specifically the VIR_XML_PROP_REQUIRED bit, but I can't help
wondering if there are other cases where switching to
virXPathString() in the way seen here might have introduced undesired
changes in behavior.
Thoughts?
--
Andrea Bolognani / Red Hat / Virtualization
On 3/24/22 18:02, Andrea Bolognani wrote: > > but I can't help > wondering if there are other cases where switching to > virXPathString() in the way seen here might have introduced undesired > changes in behavior. > > Thoughts? > Unless you want to audit every virXPath*() call with its history, I say we're good as we didn't get any bug reports. It is unfortunate, but the master was broken only for one day. Michal
On Thu, Mar 24, 2022 at 07:16:28PM +0100, Michal Prívozník wrote: > On 3/24/22 18:02, Andrea Bolognani wrote: > > but I can't help > > wondering if there are other cases where switching to > > virXPathString() in the way seen here might have introduced undesired > > changes in behavior. > > > > Thoughts? > > Unless you want to audit every virXPath*() call with its history, I say > we're good as we didn't get any bug reports. It is unfortunate, but the > master was broken only for one day. I'm not concerned about the brief period of time between Kristina's and Tim's patches were merged, but about any subtle changes in behavior that might still be lingering even after a year. That said, I have just finished auditing all uses of virXMLPropTristate*() and I don't think I can find the strength in me to go through the same process again for virXPath*() - at least not right away. So let's just hope that the lack of bug reports is an accurate indicator of the lack of significant regressions, I guess :) -- Andrea Bolognani / Red Hat / Virtualization
© 2016 - 2026 Red Hat, Inc.