Re: [libvirt PATCH v5 1/6] conf: Refactor video model parsing

[Ján Tomko <jtomko@xxxxxxxxxx>]

On a Friday in 2022, Jonathon Jongsma wrote:
> Factor out a separate function to parse out the <model> element for
> video devices.
>
> Signed-off-by: Jonathon Jongsma <jjongsma@xxxxxxxxxx>
> ---
> src/conf/domain_conf.c | 95 ++++++++++++++++++++++++++----------------
> 1 file changed, 59 insertions(+), 36 deletions(-)
>
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index fda1c6caa6..2e153db94f 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -12536,17 +12536,13 @@ virDomainVideoDriverDefParseXML(xmlNodePtr node,
>     return g_steal_pointer(&def);
> }
>
> -static virDomainVideoDef *
> -virDomainVideoDefParseXML(virDomainXMLOption *xmlopt,
> -                          xmlNodePtr node,
> -                          xmlXPathContextPtr ctxt,
> -                          unsigned int flags)
> +static int
> +virDomainVideoModelDefParseXML(virDomainVideoDef *def,
> +                               xmlNodePtr node,
> +                               xmlXPathContextPtr ctxt)
> {
> -    g_autoptr(virDomainVideoDef) def = NULL;
> -    xmlNodePtr driver;
>     xmlNodePtr accel_node;
>     xmlNodePtr res_node;
> -    VIR_XPATH_NODE_AUTORESTORE(ctxt)
>     g_autofree char *type = NULL;
>     g_autofree char *heads = NULL;
>     g_autofree char *vram = NULL;
> @@ -12555,81 +12551,108 @@ virDomainVideoDefParseXML(virDomainXMLOption *xmlopt,
>     g_autofree char *vgamem = NULL;
>     g_autofree char *primary = NULL;
>
> -    if (!(def = virDomainVideoDefNew(xmlopt)))
> -        return NULL;
> -
> +    VIR_XPATH_NODE_AUTORESTORE(ctxt);

There's no need to move this declaration/initialization and add a
semicolon to it.

>     ctxt->node = node;

[...]

> -    if ((type = virXPathString("string(./model/@type)", ctxt))) {
> +
> +    if ((type = virXPathString("string(./@type)", ctxt))) {
>         if ((def->type = virDomainVideoTypeFromString(type)) < 0) {
>             virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
>                            _("unknown video model '%s'"), type);
> -            return NULL;
> +            return -1;
>         }
>     } else {
>         def->type = VIR_DOMAIN_VIDEO_TYPE_DEFAULT;
>     }

This branch can be handled by virDomainVideoDefNew. Or by saying that
VIR_DOMAIN_VIDEO_TYPE_DEFAULT = 0.

To the rest:

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

Jano