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

[Osier Yang]

On 15/08/13 17:30, Daniel P. Berrange wrote:
> 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.

Hm, I forgot the previous discussion on virConnectListAllDomains,
agreed that returning an array of object pointers is better. For the
one who is interested in what the story is:

https://www.redhat.com/archives/libvir-list/2012-May/msg01049.html

>
>> +                                  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:

Agreed.

>
>
>> + * 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.

All the API does is reading, no writing action is involved. So no RO
checking is needed. Any problem of the unpriviledge client gets
IP addrs of its own guests?

>> +    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