Re: [PATCH v10 1/4] domifaddr: Implement the public APIs

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 




On 03/11/2015 07:09 AM, Daniel P. Berrange wrote:
> From: Nehal J Wani <nehaljw.kkd1@xxxxxxxxx>
> 
> Define helper function virDomainInterfaceFree, which allows
> the upper layer application to free the domain interface object
> conveniently.
> 
> The API is going to provide multiple methods by flags, e.g.
>   * Query guest agent
>   * Parse DHCP lease file
> 
> include/libvirt/libvirt-domain.h
>   * Define virDomainInterfaceAddresses, virDomainInterfaceFree
>   * Define structs virDomainInterface, virDomainIPAddress
> 
> src/driver-hypervisor.h:
>   * Define domainInterfaceAddresses
> 
> src/libvirt-domain.c:
>   * Implement virDomainInterfaceAddresses
>   * Implement virDomainInterfaceFree
> 
> src/libvirt_public.syms:
>   * Export the new symbols
> 
> Signed-off-by: Nehal J Wani <nehaljw.kkd1@xxxxxxxxx>
> ---
>  include/libvirt/libvirt-domain.h |  32 ++++++++++
>  src/driver-hypervisor.h          |   6 ++
>  src/libvirt-domain.c             | 123 +++++++++++++++++++++++++++++++++++++++
>  src/libvirt_public.syms          |   2 +
>  4 files changed, 163 insertions(+)
> 
> diff --git a/include/libvirt/libvirt-domain.h b/include/libvirt/libvirt-domain.h
> index 9487b80..8cc7fe8 100644
> --- a/include/libvirt/libvirt-domain.h
> +++ b/include/libvirt/libvirt-domain.h
> @@ -3725,5 +3725,37 @@ typedef struct _virTypedParameter virMemoryParameter;
>   */
>  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 */
> +
> +# ifdef VIR_ENUM_SENTINELS
> +    VIR_DOMAIN_INTERFACE_ADDRESSES_SRC_LAST
> +# endif
> +} virDomainInterfaceAddressesSource;
> +
> +typedef struct _virDomainInterfaceIPAddress virDomainIPAddress;
> +typedef virDomainIPAddress *virDomainIPAddressPtr;
> +struct _virDomainInterfaceIPAddress {
> +    int type;                /* virIPAddrType */
> +    char *addr;              /* IP address */
> +    unsigned int prefix;     /* IP address prefix */
> +};
> +
> +typedef struct _virDomainInterface virDomainInterface;
> +typedef virDomainInterface *virDomainInterfacePtr;
> +struct _virDomainInterface {
> +    char *name;                     /* interface name */
> +    char *hwaddr;                   /* hardware address */
> +    unsigned int naddrs;            /* number of items in @addrs */
> +    virDomainIPAddressPtr addrs;    /* array of IP addresses */
> +};
> +
> +int virDomainInterfaceAddresses(virDomainPtr dom,
> +                                virDomainInterfacePtr **ifaces,
> +                                unsigned int source,
> +                                unsigned int flags);
> +
> +void virDomainInterfaceFree(virDomainInterfacePtr iface);
>  
>  #endif /* __VIR_LIBVIRT_DOMAIN_H__ */
> diff --git a/src/driver-hypervisor.h b/src/driver-hypervisor.h
> index c2b4cd8..0e729c4 100644
> --- a/src/driver-hypervisor.h
> +++ b/src/driver-hypervisor.h
> @@ -1178,6 +1178,11 @@ typedef int
>                          unsigned int cellCount,
>                          unsigned int flags);
>  
> +typedef int
> +(*virDrvDomainInterfaceAddresses)(virDomainPtr dom,
> +                                  virDomainInterfacePtr **ifaces,
> +                                  unsigned int source,
> +                                  unsigned int flags);
>  
>  typedef struct _virHypervisorDriver virHypervisorDriver;
>  typedef virHypervisorDriver *virHypervisorDriverPtr;
> @@ -1404,6 +1409,7 @@ struct _virHypervisorDriver {
>      virDrvConnectGetAllDomainStats connectGetAllDomainStats;
>      virDrvNodeAllocPages nodeAllocPages;
>      virDrvDomainGetFSInfo domainGetFSInfo;
> +    virDrvDomainInterfaceAddresses domainInterfaceAddresses;
>  };
>  
>  
> diff --git a/src/libvirt-domain.c b/src/libvirt-domain.c
> index 04545fd..cf36d30 100644
> --- a/src/libvirt-domain.c
> +++ b/src/libvirt-domain.c
> @@ -11338,3 +11338,126 @@ virDomainFSInfoFree(virDomainFSInfoPtr info)
>          VIR_FREE(info->devAlias[i]);
>      VIR_FREE(info->devAlias);
>  }
> +
> +/**
> + * virDomainInterfaceAddresses:
> + * @dom: domain object
> + * @ifaces: pointer to an array of pointers pointing to interface objects
> + * @source: one of the virDomainInterfaceAddressesSource constants
> + * @flags: currently unused, pass zero
> + *
> + * Return a pointer to the allocated array of pointers to interfaces
> + * present in given domain along with their IP and MAC addresses. Note that
> + * single interface can have multiple or even 0 IP addresses.
> + *
> + * This API dynamically allocates the virDomainInterfacePtr struct based on
> + * how many interfaces domain @dom has, usually there's 1:1 correlation. The
> + * count of the interfaces is returned as the return value.
> + *
> + * If @source is VIR_DOMAIN_INTERFACE_ADDRESSES_SRC_LEASE, the DHCP lease
> + * file associated with any virtual networks will be examined to obtain
> + * the interface addresses. This only returns data for interfaces which
> + * are connected to virtual networks managed by libvirt.
> + *
> + * If @source is VIR_DOMAIN_INTERFACE_ADDRESSES_SRC_AGENT, a configured
> + * guest agent is needed for successful return from this API. Moreover, if
> + * guest agent is used then the interface name is the one seen by guest OS.
> + * To match such interface with the one from @dom XML use MAC address or IP
> + * range.
> + *
> + * @ifaces->name and @ifaces->hwaddr are never NULL.
> + *
> + * The caller *must* free @ifaces when no longer needed. Usual use case
> + * looks like this:
> + *
> + *  virDomainInterfacePtr *ifaces = NULL;
> + *  int ifaces_count = 0;
> + *  size_t i, j;
> + *  virDomainPtr dom = ... obtain a domain here ...;
> + *
> + *  if ((ifaces_count = virDomainInterfaceAddresses(dom, &ifaces,
> + *           VIR_DOMAIN_INTERFACE_ADDRESSES_SRC_LEASE)) < 0)
> + *      goto cleanup;
> + *
> + *  ... do something with returned values, for example:
> + *  for (i = 0; i < ifaces_count; i++) {
> + *      printf("name: %s", ifaces[i]->name);
> + *      printf(" hwaddr: %s", ifaces[i]->hwaddr);
> + *
> + *      for (j = 0; j < ifaces[i]->naddrs; j++) {
> + *          virDomainIPAddressPtr ip_addr = ifaces[i]->addrs + j;
> + *          printf("[addr: %s prefix: %d type: %d]",
> + *                 ip_addr->addr, ip_addr->prefix, ip_addr->type);
> + *      }
> + *      printf("\n");
> + *  }
> + *
> + *  cleanup:
> + *      if (ifaces)

since ifaces_count could be negative...

if (ifaces && ifaces_count > 0)

> + *          for (i = 0; i < ifaces_count; i++)
> + *              virDomainInterfaceFree(ifaces[i]);
> + *      free(ifaces);
> + *
> + * Returns the number of interfaces on success, -1 in case of error.
> + */
> +int
> +virDomainInterfaceAddresses(virDomainPtr dom,
> +                            virDomainInterfacePtr **ifaces,
> +                            unsigned int source,
> +                            unsigned int flags)
> +{
> +    virConnectPtr conn;
> +
> +    VIR_DOMAIN_DEBUG(dom, "ifaces=%p, source=%d, flags=%x", ifaces, source, flags);
> +
> +    virResetLastError();
> +
> +    conn = dom->conn;

If 'dom' is NULL, then there's a pointer deref (considering check below)

> +
> +    if (ifaces)
> +        *ifaces = NULL;
> +    virCheckDomainReturn(dom, -1);
> +    virCheckNonNullArgGoto(ifaces, error);
> +    virCheckReadOnlyGoto(conn->flags, error);

I think I ran into this when I was adding the libvirt-perl code - since
this is a get interface - does it really matter if it's readonly?

Also, I'd move both conn = dom->conn and "if (ifaces) *ifaces = NULL;"
to here

> +
> +    if (conn->driver->domainInterfaceAddresses) {
> +        int ret;
> +        ret = conn->driver->domainInterfaceAddresses(dom, ifaces, source, flags);
> +        if (ret < 0)
> +            goto error;
> +        return ret;
> +    }
> +
> +    virReportError(VIR_ERR_NO_SUPPORT, __FUNCTION__);
> +
> + error:
> +    virDispatchError(dom->conn);
> +    return -1;
> +}
> +
> +
> +/**
> + * virDomainInterfaceFree:
> + * @iface: an interface object
> + *
> + * Free the interface object. The data structure is
> + * freed and should not be used thereafter. If @iface
> + * is NULL, then this method has no effect.
> + */
> +void
> +virDomainInterfaceFree(virDomainInterfacePtr iface)
> +{
> +    size_t i;
> +
> +    if (!iface)
> +        return;
> +
> +    VIR_FREE(iface->name);
> +    VIR_FREE(iface->hwaddr);
> +
> +    for (i = 0; i < iface->naddrs; i++)
> +        VIR_FREE(iface->addrs[i].addr);
> +    VIR_FREE(iface->addrs);
> +
> +    VIR_FREE(iface);
> +}
> diff --git a/src/libvirt_public.syms b/src/libvirt_public.syms
> index 34852c1..64c455d 100644
> --- a/src/libvirt_public.syms
> +++ b/src/libvirt_public.syms
> @@ -699,6 +699,8 @@ LIBVIRT_1.2.14 {
>      global:
>          virDomainIOThreadsInfoFree;
>          virDomainGetIOThreadsInfo;
> +        virDomainInterfaceAddresses;
> +        virDomainInterfaceFree;
>  } LIBVIRT_1.2.12;
>  

This will have some merge conflicts, but nothing unusual...

>  # .... define new API here using predicted next version number ....
> 

ACK - with the cleanups

John

--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list




[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]