[libvirt PATCH 09/14] qemu: agent: split out qemuAgentGetInterfaceAddresses

[Jonathon Jongsma]

On Tue,  6 Oct 2020 08:58:45 +0200
Ján Tomko <jtomko at redhat.com> wrote:

> Convert one interface from the "return" array returned by
> "guest-network-get-interfaces" to virDomainInterface.
> 
> Due to the functionality of squashing interface aliases together,
> this is not a pure function - it either:
> * Adds the interface to ifaces_ret, incrementing ifaces_count
>   and adds a pointer to it into the ifaces_store hash table.
> * Adds the additional IP addresses from the interface alias
>   to the existing interface entry, found through the hash table.
>   This does not increment ifaces_count or extend the array.

It seems to me that if we were to factor out this function, this
documentation would belong within the code rather than just in the
commit message. Without it, it's not very obvious what this
function does with its arguments. [1]

The fact that it needs this documentation to be understood and it is
only called from a single place makes me think that it probably isn't
worthwhile to factor it out into a separate function. I think it
actually makes the code a bit harder to understand.


[1] I was going to suggest simply removing the 'ifaces_ret' and
'ifaces_count' arguments from this function and doing all of the
processing with just the hash table for storage. Then the calling
function could construct the return array from the hash table after
everything is processed. Unfortunately this won't necessarily maintain
the ordering of the returned interfaces. I don't know if the order is
important to maintain or not...


> 
> Signed-off-by: Ján Tomko <jtomko at redhat.com>
> ---
>  src/qemu/qemu_agent.c | 141
> +++++++++++++++++++++++------------------- 1 file changed, 79
> insertions(+), 62 deletions(-)
> 
> diff --git a/src/qemu/qemu_agent.c b/src/qemu/qemu_agent.c
> index 7a4f5cd6db..b314c610e7 100644
> --- a/src/qemu/qemu_agent.c
> +++ b/src/qemu/qemu_agent.c
> @@ -2104,6 +2104,82 @@
> qemuAgentGetInterfaceOneAddress(virDomainIPAddressPtr ip_addr, }
>  
>  
> +static int
> +qemuAgentGetInterfaceAddresses(virDomainInterfacePtr **ifaces_ret,
> +                               size_t *ifaces_count,
> +                               virHashTablePtr ifaces_store,
> +                               virJSONValuePtr tmp_iface)
> +{
> +    virJSONValuePtr ip_addr_arr = NULL;
> +    const char *hwaddr, *ifname_s, *name = NULL;
> +    virDomainInterfacePtr iface = NULL;
> +    g_auto(GStrv) ifname = NULL;
> +    size_t addrs_count = 0;
> +    size_t j;
> +
> +    /* interface name is required to be presented */
> +    name = virJSONValueObjectGetString(tmp_iface, "name");
> +    if (!name) {
> +        virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> +                       _("qemu agent didn't provide 'name' field"));
> +        return -1;
> +    }
> +
> +    /* Handle interface alias (<ifname>:<alias>) */
> +    ifname = virStringSplit(name, ":", 2);
> +    ifname_s = ifname[0];
> +
> +    iface = virHashLookup(ifaces_store, ifname_s);
> +
> +    /* If the hash table doesn't contain this iface, add it */
> +    if (!iface) {
> +        if (VIR_EXPAND_N(*ifaces_ret, *ifaces_count, 1) < 0)
> +            return -1;
> +
> +        iface = g_new0(virDomainInterface, 1);
> +        (*ifaces_ret)[*ifaces_count - 1] = iface;
> +
> +        if (virHashAddEntry(ifaces_store, ifname_s, iface) < 0)
> +            return -1;
> +
> +        iface->naddrs = 0;
> +        iface->name = g_strdup(ifname_s);
> +
> +        hwaddr = virJSONValueObjectGetString(tmp_iface,
> "hardware-address");
> +        iface->hwaddr = g_strdup(hwaddr);
> +    }
> +
> +    /* as well as IP address which - moreover -
> +     * can be presented multiple times */
> +    ip_addr_arr = virJSONValueObjectGet(tmp_iface, "ip-addresses");
> +    if (!ip_addr_arr)
> +        return 0;
> +
> +    if (!virJSONValueIsArray(ip_addr_arr)) {
> +        virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> +                       _("Malformed ip-addresses array"));
> +        return -1;
> +    }
> +
> +    /* If current iface already exists, continue with the count */
> +    addrs_count = iface->naddrs;
> +
> +    if (VIR_EXPAND_N(iface->addrs, addrs_count,
> +                     virJSONValueArraySize(ip_addr_arr))  < 0)
> +        return -1;
> +
> +    for (j = 0; j < virJSONValueArraySize(ip_addr_arr); j++) {
> +        virJSONValuePtr ip_addr_obj =
> virJSONValueArrayGet(ip_addr_arr, j);
> +        virDomainIPAddressPtr ip_addr = iface->addrs +
> iface->naddrs++; +
> +        if (qemuAgentGetInterfaceOneAddress(ip_addr, ip_addr_obj,
> name) < 0)
> +            return -1;
> +    }
> +
> +    return 0;
> +}
> +
> +
>  /*
>   * qemuAgentGetInterfaces:
>   * @agent: agent object
> @@ -2120,7 +2196,7 @@ qemuAgentGetInterfaces(qemuAgentPtr agent,
>                         virDomainInterfacePtr **ifaces)
>  {
>      int ret = -1;
> -    size_t i, j;
> +    size_t i;
>      virJSONValuePtr cmd = NULL;
>      virJSONValuePtr reply = NULL;
>      virJSONValuePtr ret_array = NULL;
> @@ -2151,70 +2227,11 @@ qemuAgentGetInterfaces(qemuAgentPtr agent,
>  
>      for (i = 0; i < virJSONValueArraySize(ret_array); i++) {
>          virJSONValuePtr tmp_iface = virJSONValueArrayGet(ret_array,
> i);
> -        virJSONValuePtr ip_addr_arr = NULL;
> -        const char *hwaddr, *ifname_s, *name = NULL;
> -        virDomainInterfacePtr iface = NULL;
> -        g_auto(GStrv) ifname = NULL;
> -        size_t addrs_count = 0;
>  
> -        /* interface name is required to be presented */
> -        name = virJSONValueObjectGetString(tmp_iface, "name");
> -        if (!name) {
> -            virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> -                           _("qemu agent didn't provide 'name'
> field"));
> +        if (qemuAgentGetInterfaceAddresses(&ifaces_ret,
> &ifaces_count,
> +                                           ifaces_store, tmp_iface)
> < 0) goto error;
> -        }
>  
> -        /* Handle interface alias (<ifname>:<alias>) */
> -        ifname = virStringSplit(name, ":", 2);
> -        ifname_s = ifname[0];
> -
> -        iface = virHashLookup(ifaces_store, ifname_s);
> -
> -        /* If the hash table doesn't contain this iface, add it */
> -        if (!iface) {
> -            if (VIR_EXPAND_N(ifaces_ret, ifaces_count, 1) < 0)
> -                goto error;
> -
> -            iface = g_new0(virDomainInterface, 1);
> -            ifaces_ret[ifaces_count - 1] = iface;
> -
> -            if (virHashAddEntry(ifaces_store, ifname_s, iface) < 0)
> -                goto error;
> -
> -            iface->naddrs = 0;
> -            iface->name = g_strdup(ifname_s);
> -
> -            hwaddr = virJSONValueObjectGetString(tmp_iface,
> "hardware-address");
> -            iface->hwaddr = g_strdup(hwaddr);
> -        }
> -
> -        /* as well as IP address which - moreover -
> -         * can be presented multiple times */
> -        ip_addr_arr = virJSONValueObjectGet(tmp_iface,
> "ip-addresses");
> -        if (!ip_addr_arr)
> -            continue;
> -
> -        if (!virJSONValueIsArray(ip_addr_arr)) {
> -            virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> -                           _("Malformed ip-addresses array"));
> -            goto error;
> -        }
> -
> -        /* If current iface already exists, continue with the count
> */
> -        addrs_count = iface->naddrs;
> -
> -        if (VIR_EXPAND_N(iface->addrs, addrs_count,
> -                         virJSONValueArraySize(ip_addr_arr))  < 0)
> -            goto error;
> -
> -        for (j = 0; j < virJSONValueArraySize(ip_addr_arr); j++) {
> -            virJSONValuePtr ip_addr_obj =
> virJSONValueArrayGet(ip_addr_arr, j);
> -            virDomainIPAddressPtr ip_addr = iface->addrs +
> iface->naddrs++; -
> -            if (qemuAgentGetInterfaceOneAddress(ip_addr,
> ip_addr_obj, name) < 0)
> -                goto error;
> -        }
>      }
>  
>      *ifaces = g_steal_pointer(&ifaces_ret);