Re: [libvirt PATCH 02/10] virDomainAudioCommonParse: Use virXMLProp*

[Ján Tomko <jtomko@xxxxxxxxxx>]

On a Tuesday in 2021, Tim Wiederhake wrote:
> This strictens the parser to disallow negative values (interpreted as
> `UINT_MAX + value + 1`) for attributes `voices` (typically 1),
> `bufferLength` (measured in milliseconds), `frequency` (in Hz, typically
> 44100), and `channels` (typically 2 for stereo).
>
> None of these properties benefit from or have a sensible use-case for
> wrap-around behavior.
>
> Signed-off-by: Tim Wiederhake <twiederh@xxxxxxxxxx>
> ---
> src/conf/domain_conf.c | 73 ++++++++++++------------------------------
> 1 file changed, 20 insertions(+), 53 deletions(-)
>
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index d8e34e79b0..371a9dead7 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -13419,31 +13419,19 @@ virDomainAudioCommonParse(virDomainAudioIOCommon *def,
>                           xmlNodePtr node,
>                           xmlXPathContextPtr ctxt)
> {
> -    g_autofree char *mixingEngine = virXMLPropString(node, "mixingEngine");
> -    g_autofree char *fixedSettings = virXMLPropString(node, "fixedSettings");
> -    g_autofree char *voices = virXMLPropString(node, "voices");
> -    g_autofree char *bufferLength = virXMLPropString(node, "bufferLength");
>     xmlNodePtr settings;
>     VIR_XPATH_NODE_AUTORESTORE(ctxt);
>
>     ctxt->node = node;
>     settings = virXPathNode("./settings", ctxt);
>
> -    if (mixingEngine &&
> -        ((def->mixingEngine =
> -          virTristateBoolTypeFromString(mixingEngine)) <= 0)) {
> -        virReportError(VIR_ERR_XML_ERROR,
> -                       _("unknown 'mixingEngine' value '%s'"), mixingEngine);
> +    if (virXMLPropTristateBool(node, "mixingEngine", VIR_XML_PROP_NONE,
> +                               &def->mixingEngine) < 0)
>         return -1;
> -    }
>
> -    if (fixedSettings &&
> -        ((def->fixedSettings =
> -          virTristateBoolTypeFromString(fixedSettings)) <= 0)) {
> -        virReportError(VIR_ERR_XML_ERROR,
> -                       _("unknown 'fixedSettings' value '%s'"), fixedSettings);
> +    if (virXMLPropTristateBool(node, "fixedSettings", VIR_XML_PROP_NONE,
> +                               &def->fixedSettings) < 0)
>         return -1;
> -    }
>
>     if (def->fixedSettings == VIR_TRISTATE_BOOL_YES &&
>         def->mixingEngine != VIR_TRISTATE_BOOL_YES) {
> @@ -13452,58 +13440,37 @@ virDomainAudioCommonParse(virDomainAudioIOCommon *def,
>         return -1;
>     }
>
> -    if (voices &&
> -        (virStrToLong_ui(voices, NULL, 10, &def->voices) < 0 ||
> -         !def->voices)) {
> -        virReportError(VIR_ERR_XML_ERROR,
> -                       _("cannot parse 'voices' value '%s'"), voices);
> +    if (virXMLPropUInt(node, "voices", 10,
> +                       VIR_XML_PROP_NONE | VIR_XML_PROP_NONZERO,

VIR_XML_PROP_NONE is a symbolic value to be used instead of 0

This and all the cases below should be just VIR_XML_PROP_NONZERO

With that change:
Reviewed-by: Ján Tomko <jtomko@xxxxxxxxxx>

Jano

Attachment: signature.asc
Description: PGP signature