[libvirt] [PATCHv2 1/5] domifaddr: Implement the public API

[Daniel P. Berrange]

On Thu, Aug 15, 2013 at 02:24:26AM +0530, nehaljwani wrote:
> diff --git a/include/libvirt/libvirt.h.in b/include/libvirt/libvirt.h.in
> index 52ac95d..fa49e70 100644
> --- a/include/libvirt/libvirt.h.in
> +++ b/include/libvirt/libvirt.h.in
> @@ -2044,6 +2044,38 @@ int                     virDomainGetInterfaceParameters (virDomainPtr dom,
>                                                           virTypedParameterPtr params,
>                                                           int *nparams, unsigned int flags);
>  
> +typedef enum {
> +    VIR_IP_ADDR_TYPE_IPV4,
> +    VIR_IP_ADDR_TYPE_IPV6,
> +
> +#ifdef VIR_ENUM_SENTINELS
> +    VIR_IP_ADDR_TYPE_LAST
> +#endif
> +} virIPAddrType;
> +
> +
> +typedef struct _virDomainInterfaceIPAddress virDomainIPAddress;
> +typedef virDomainIPAddress *virDomainIPAddressPtr;
> +struct _virDomainInterfaceIPAddress {
> +    int type;       /* virIPAddrType */
> +    char *addr;     /* IP address */
> +    int prefix;     /* IP address prefix */
> +};
> +
> +typedef struct _virDomainInterface virDomainInterface;
> +typedef virDomainInterface *virDomainInterfacePtr;
> +struct _virDomainInterface {
> +    char *name;                     /* interface name */
> +    char *hwaddr;                   /* hardware address */
> +    unsigned int ip_addrs_count;    /* number of items in @ip_addr */

Lets rename this to  'naddrs'

> +    virDomainIPAddressPtr ip_addrs; /* array of IP addresses */

And just 'addrs'

> +int virDomainInterfacesAddresses (virDomainPtr dom,
> +                                  virDomainInterfacePtr *ifaces,

Hmm, so this reasons an array of interface objects. I wonder if it
is better to return an array of pointers to interface objects. Using
an array pointers makes for more natural code design when free'ing
the pointers, and is what we do with most other APIs.

> +                                  unsigned int *ifaces_count,

Since 'ifaces' is allocated by this API and not the caller, the
'ifaces_count' parameter is pointless. Just use the return value
of the function to tell the caller how many elements were allocated.
This is the model we use for similar APIs such as
virConnectListAllDomains:


> + * virDomainInterfacePtr ifaces = NULL;
> + * unsigned int ifaces_count = 0;
> + * size_t i, j;
> + * virDomainPtr dom = ... obtain a domain here ...;
> + *
> + * if (virDomainInterfacesAddresses(dom, &ifaces, &ifaces_count, 0) < 0)
> + *     goto cleanup;
> + *
> + * ... do something with returned values, for example:
> + * for (i = 0; i < ifaces_count; i++) {
> + *     printf("name: %s", ifaces[i].name);
> + *     if (ifaces[i].hwaddr)
> + *         printf(" hwaddr: %s", ifaces[i].hwaddr);
> + *
> + *     for (j = 0; j < ifaces[i].ip_addrs_count; j++) {
> + *         virDomainIPAddressPtr ip_addr = ifaces[i].ip_addrs + j;
> + *         printf("[addr: %s prefix: %d type: %d]",
> + *                ip_addr->addr, ip_addr->prefix, ip_addr->type);
> + *     }
> + *     printf("\n");
> + * }
> + *
> + * cleanup:
> + * for (i = 0; i < ifaces_count; i++) {
> + *     free(ifaces[i].name);
> + *     free(ifaces[i].hwaddr);
> + *     for (j = 0; j < ifaces[i].ip_addrs_count; j++)
> + *         free(ifaces[i].ip_addrs[j].addr);
> + *     free(ifaces[i].ip_addrs);
> + * }
> + * free(ifaces);

This is pretty awful cleanup code for apps. We should provide
them with a virDomainIntefaceFree(virDomainInterfacePtr iface)
function to free. eg so they can do

    for (i = 0; i < ifaces_count; i++) {
        virDomainInterfaceFree(ifaces[i]);
    }
    free(ifaces);

> + *
> + * Returns 0 on success, -1 in case of error.
> + */
> +int
> +virDomainInterfacesAddresses(virDomainPtr dom,
> +                             virDomainInterfacePtr *ifaces,
> +                             unsigned int *ifaces_count,
> +                             unsigned int flags)
> +{
> +    virConnectPtr conn;
> +
> +    VIR_DOMAIN_DEBUG(dom, "ifaces=%p, ifaces_count=%p, flags=%x",
> +                     ifaces, ifaces_count, flags);
> +
> +    virResetLastError();
> +
> +    if (!VIR_IS_CONNECTED_DOMAIN(dom)) {
> +        virLibDomainError(VIR_ERR_INVALID_DOMAIN, __FUNCTION__);
> +        goto error;
> +    }
> +

I wonder if we need to add a check for VIR_CONNECT_RO in this method.
Not sure whether is a good idea to expose the list of IP addrs to an
unprivileged client or not.

> +    conn = dom->conn;
> +
> +    virCheckNonNullArgGoto(ifaces, error);
> +    virCheckNonNullArgGoto(ifaces_count, error);
> +
> +    if (conn->driver->domainInterfacesAddresses) {
> +        int ret;
> +        ret = conn->driver->domainInterfacesAddresses(dom, ifaces,
> +                                                      ifaces_count, flags);
> +        if (ret < 0)
> +            goto error;
> +        return ret;
> +    }
> +
> +    virLibConnError(VIR_ERR_NO_SUPPORT, __FUNCTION__);
> +
> +error:
> +    virDispatchError(dom ? dom->conn : NULL);
> +    return -1;
> +}
> +

Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|