Re: [PATCH v2 16/19] Refactoring virDomainGraphicsDefParseXMLSpice() to use XPath
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(a)redhat.com>
Date: Wed May 5 12:55:48 2021 +0200
virDomainGraphicsDefParseXMLSpice: Use virXMLProp*
Signed-off-by: Tim Wiederhake <twiederh(a)redhat.com>
Reviewed-by: Michal Privoznik <mprivozn(a)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