[PATCH v2 16/19] Refactoring virDomainGraphicsDefParseXMLSpice() to use XPath

[Andrea Bolognani]

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 at redhat.com>
  Date:   Wed May 5 12:55:48 2021 +0200

    virDomainGraphicsDefParseXMLSpice: Use virXMLProp*

    Signed-off-by: Tim Wiederhake <twiederh at redhat.com>
    Reviewed-by: Michal Privoznik <mprivozn at 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