[libvirt] [PATCH v2 2/4] qemu: introduce qemuARPGetInterfaces to get IP from host's arp table

[Michal Privoznik]

On 01/24/2018 05:09 PM, Chen Hanxiao wrote:
> From: Chen Hanxiao <chenhanxiao at gmail.com>
> 
> introduce VIR_DOMAIN_INTERFACE_ADDRESSES_SRC_ARP to get ip address
> of VM from the output of /proc/net/arp
> 
> Signed-off-by: Chen Hanxiao <chenhanxiao at gmail.com>
> ---
>  include/libvirt/libvirt-domain.h |  1 +
>  src/qemu/qemu_driver.c           | 75 ++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 76 insertions(+)
> 
> diff --git a/include/libvirt/libvirt-domain.h b/include/libvirt/libvirt-domain.h
> index 4048acf38..38e2d9a3e 100644
> --- a/include/libvirt/libvirt-domain.h
> +++ b/include/libvirt/libvirt-domain.h
> @@ -4665,6 +4665,7 @@ typedef virMemoryParameter *virMemoryParameterPtr;
>  typedef enum {
>      VIR_DOMAIN_INTERFACE_ADDRESSES_SRC_LEASE = 0, /* Parse DHCP lease file */
>      VIR_DOMAIN_INTERFACE_ADDRESSES_SRC_AGENT = 1, /* Query qemu guest agent */
> +    VIR_DOMAIN_INTERFACE_ADDRESSES_SRC_ARP = 2, /* Query ARP tables */

You should document the flag in src/libvirt-domain.c in
virDomainInterfaceAddresses documentation and also mention all the
limitations.

>  
>  # ifdef VIR_ENUM_SENTINELS
>      VIR_DOMAIN_INTERFACE_ADDRESSES_SRC_LAST
> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> index a203c9297..e31a74261 100644
> --- a/src/qemu/qemu_driver.c
> +++ b/src/qemu/qemu_driver.c
> @@ -160,6 +160,9 @@ static int qemuGetDHCPInterfaces(virDomainPtr dom,
>                                   virDomainObjPtr vm,
>                                   virDomainInterfacePtr **ifaces);
>  
> +static int qemuARPGetInterfaces(virDomainObjPtr vm,
> +                                virDomainInterfacePtr **ifaces);
> +
>  static virQEMUDriverPtr qemu_driver;
>  
>  
> @@ -20384,6 +20387,10 @@ qemuDomainInterfaceAddresses(virDomainPtr dom,
>  
>          break;
>  
> +    case VIR_DOMAIN_INTERFACE_ADDRESSES_SRC_ARP:
> +        ret = qemuARPGetInterfaces(vm, ifaces);
> +        break;
> +
>      default:
>          virReportError(VIR_ERR_ARGUMENT_UNSUPPORTED,
>                         _("Unknown IP address data source %d"),
> @@ -20494,6 +20501,74 @@ qemuGetDHCPInterfaces(virDomainPtr dom,
>  }
>  
>  
> +static int
> +qemuARPGetInterfaces(virDomainObjPtr vm,
> +                     virDomainInterfacePtr **ifaces)
> +{
> +    size_t i, j;
> +    size_t ifaces_count = 0;
> +    int ret = -1;
> +    char macaddr[VIR_MAC_STRING_BUFLEN];
> +    virDomainInterfacePtr *ifaces_ret = NULL;
> +    virDomainInterfacePtr iface = NULL;
> +
> +    virArpTablePtr table;
> +    if (VIR_ALLOC(table) < 0)
> +        goto error;

The empty line should go between block of variable declaration and the
code block.

> +    if (virGetArpTable(&table) < 0)
> +        goto cleanup;
> +
> +    for (i = 0; i < vm->def->nnets; i++) {
> +        if (vm->def->nets[i]->type != VIR_DOMAIN_NET_TYPE_NETWORK)
> +            continue;
> +
> +        virMacAddrFormat(&(vm->def->nets[i]->mac), macaddr);
> +        virArpTableEntry entry;
> +        for (j = 0; j < table->n; j++) {
> +            entry = table->t[j];
> +            if (STREQ(entry.mac, macaddr)) {
> +                    if (VIR_EXPAND_N(ifaces_ret, ifaces_count, 1) < 0)
> +                        goto error;
> +
> +                    if (VIR_ALLOC(ifaces_ret[ifaces_count - 1]) < 0)
> +                        goto error;
> +
> +                    iface = ifaces_ret[ifaces_count - 1];
> +                    iface->naddrs = 1;
> +                    if (VIR_ALLOC_N(iface->addrs, iface->naddrs) < 0)
> +                        goto error;
> +
> +                    if (VIR_STRDUP(iface->name, vm->def->nets[i]->ifname) < 0)
> +                        goto cleanup;

Interesting. If VIR_ALLOC_N() fails you jump to error, vir STRDUP fails
you jump to cleanup. That is not right. @iface is leaked if this STRDUP
fails. Just drop the error label and move it to cleanup. In case of
success @ifaces_ret is NULL so virDomainInterfaceFree() is not called
(btw might reset ifaces_count to zero).

> +
> +                    if (VIR_STRDUP(iface->hwaddr, macaddr) < 0)
> +                        goto cleanup;
> +
> +                    if (VIR_STRDUP(iface->addrs->addr, entry.ipaddr) < 0)
> +                        goto cleanup;

I wonder if there's something we can do with prefix. I mean, the arp
table has no knowledge of it so probably it's okay to leave it as is (=
set to 0). Just wanted to point this fact out.

> +            }
> +        }
> +    }
> +
> +    *ifaces = ifaces_ret;
> +    ifaces_ret = NULL;
> +    ret = ifaces_count;
> +
> + cleanup:
> +    virArpTableFree(table);
> +    return ret;
> +
> + error:
> +    if (ifaces_ret) {
> +        for (i = 0; i < ifaces_count; i++)
> +            virDomainInterfaceFree(ifaces_ret[i]);
> +    }
> +    VIR_FREE(ifaces_ret);
> +
> +    goto cleanup;
> +}
> +
> +

Michal