Re: [PATCH 1/5] conf: virNetDevVPortProfileParse refactor

[Ján Tomko via Devel <devel@xxxxxxxxxxxxxxxxx>]

On a Monday in 2025, Kirill Shchetiniuk via Devel wrote:
> From: Kirill Shchetiniuk <kshcheti@xxxxxxxxxx>
>
> Refactored the virNetDevVPortProfileParse function to use the appropriate
> virXMLProp* functions to parse input configuration XML.
>
> Signed-off-by: Kirill Shchetiniuk <kshcheti@xxxxxxxxxx>
> ---
> src/conf/netdev_vport_profile_conf.c | 120 +++++++++++----------------
> 1 file changed, 48 insertions(+), 72 deletions(-)
>
> diff --git a/src/conf/netdev_vport_profile_conf.c b/src/conf/netdev_vport_profile_conf.c
> index 032a3147d7..9be19de808 100644
> --- a/src/conf/netdev_vport_profile_conf.c
> +++ b/src/conf/netdev_vport_profile_conf.c
> @@ -29,12 +29,6 @@ virNetDevVPortProfile *
> virNetDevVPortProfileParse(xmlNodePtr node, unsigned int flags)
> {
>     g_autofree char *virtPortType = NULL;
> -    g_autofree char *virtPortManagerID = NULL;
> -    g_autofree char *virtPortTypeID = NULL;
> -    g_autofree char *virtPortTypeIDVersion = NULL;
> -    g_autofree char *virtPortInstanceID = NULL;
> -    g_autofree char *virtPortProfileID = NULL;
> -    g_autofree char *virtPortInterfaceID = NULL;
>     g_autofree virNetDevVPortProfile *virtPort = NULL;
>     xmlNodePtr parameters;
>
> @@ -55,88 +49,70 @@ virNetDevVPortProfileParse(xmlNodePtr node, unsigned int flags)
>     }
>
>     if ((parameters = virXMLNodeGetSubelement(node, "parameters"))) {
> -        virtPortManagerID = virXMLPropString(parameters, "managerid");
> -        virtPortTypeID = virXMLPropString(parameters, "typeid");
> -        virtPortTypeIDVersion = virXMLPropString(parameters, "typeidversion");
> -        virtPortInstanceID = virXMLPropString(parameters, "instanceid");
> -        virtPortProfileID = virXMLPropString(parameters, "profileid");
> -        virtPortInterfaceID = virXMLPropString(parameters, "interfaceid");
> -    }
> +        int ret;

Generally, we use 'ret' as the value that will be returned by the
current function, and 'rc' for storing temporary values of the functions
we call.

> +        unsigned int tempUInt;

There's no need to encode the type in the variable name, the old 'val'
was just fine.

> +        g_autofree char *virtPortProfileID = virXMLPropString(parameters, "profileid");
>
> -    if (virtPortManagerID) {
> -        unsigned int val;
> +        if ((ret = virXMLPropUInt(parameters, "managerid", 10, VIR_XML_PROP_NONE, &tempUInt))) {

Omitting the != 0 should be avoided according to our coding style.
https://libvirt.org/coding-style.html#conditional-expressions

Moreover, writing this as:

if ((rc = virXMLPropUint(...)) < 0)
    return NULL;
if (rc > 0) {
    ....
}

is easier to read, IMO

> +            if (ret < 0)
> +                return NULL;

Jano

Attachment: signature.asc
Description: PGP signature