Re: [libvirt PATCH v2 1/6] virDomainGraphicsDefParseXMLSpice: Use virXMLProp*

[Ján Tomko <jtomko@xxxxxxxxxx>]

On a Tuesday in 2021, Tim Wiederhake wrote:
> Signed-off-by: Tim Wiederhake <twiederh@xxxxxxxxxx>
> ---
> 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@xxxxxxxxxx>

Jano

Attachment: signature.asc
Description: PGP signature