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

Osier Yang jyang at redhat.com
Fri Aug 23 10:03:58 UTC 2013


On 23/08/13 17:52, Osier Yang wrote:
> On 23/08/13 06:18, nehaljwani wrote:
>> Define a new API virDomainInterfacesAddresses, which returns
>> the address information of a running domain's interfaces(s).
>> If no interface name is specified, it returns the information
>> of all interfaces, otherwise it only returns the information
>> of the specificed interface. The address information includes
>> the MAC and IP addresses.
>>
>> The API is going to provide multiple methods by flags, e.g.
>>
>>    * Query guest agent
>>    * Parse lease file of dnsmasq
>>    * DHCP snooping
>>
>> But at this stage, it will only work with guest agent, and flags
>> won't be supported.
>>
>> include/libvirt/libvirt.h.in:
>>    * Define virDomainInterfacesAddresses
>>    * Define structs virDomainInterface, virDomainIPAddress
>>
>> python/generator.py:
>>    * Skip the auto-generation for virDomainInterfacesAddresses
>>
>> src/driver.h:
>>    * Define domainInterfacesAddresses
>>
>> src/libvirt.c:
>>    * Implement virDomainInterfacesAddresses
>>
>> src/libvirt_public.syms:
>>    * Export the new symbol
>
> You introduced virDomainInterfaceFree in the new series,
> the commit log should be changed to be consistent.
>
>>
>> ---
>>   include/libvirt/libvirt.h.in |  33 +++++++++++++
>>   python/generator.py          |   3 ++
>>   src/driver.h                 |   6 +++
>>   src/libvirt.c                | 110 
>> +++++++++++++++++++++++++++++++++++++++++++
>>   src/libvirt_public.syms      |   6 +++
>>   5 files changed, 158 insertions(+)
>>
>> diff --git a/include/libvirt/libvirt.h.in b/include/libvirt/libvirt.h.in
>> index a47e33c..deb1e1f 100644
>> --- a/include/libvirt/libvirt.h.in
>> +++ b/include/libvirt/libvirt.h.in
>> @@ -2044,6 +2044,39 @@ 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 naddrs;            /* number of items in @addrs */
>> +    virDomainIPAddressPtr addrs;    /* array of IP addresses */
>> +};
>> +
>> +int virDomainInterfacesAddresses (virDomainPtr dom,
>> +                                  virDomainInterfacePtr **ifaces,
>> +                                  unsigned int flags);
>> +
>> +void virDomainInterfaceFree (virDomainInterfacePtr iface);
>> +
>>   /* Management of domain block devices */
>>     int                     virDomainBlockPeek (virDomainPtr dom,
>> diff --git a/python/generator.py b/python/generator.py
>> index 427cebc..4f3c8ee 100755
>> --- a/python/generator.py
>> +++ b/python/generator.py
>> @@ -458,6 +458,7 @@ skip_impl = (
>>       'virNodeGetMemoryParameters',
>>       'virNodeSetMemoryParameters',
>>       'virNodeGetCPUMap',
>> +    'virDomainInterfacesAddresses',
>>       'virDomainMigrate3',
>>       'virDomainMigrateToURI3',
>>   )
>> @@ -560,6 +561,8 @@ skip_function = (
>>       "virTypedParamsGetString",
>>       "virTypedParamsGetUInt",
>>       "virTypedParamsGetULLong",
>> +
>> +    "virDomainInterfaceFree", #Only for convenience in C
>
> Consider changing the comment into "# Only useful in C".
>
>>   )
>>     lxc_skip_function = (
>> diff --git a/src/driver.h b/src/driver.h
>> index be64333..8b6182e 100644
>> --- a/src/driver.h
>> +++ b/src/driver.h
>> @@ -518,6 +518,11 @@ typedef int
>>                                         unsigned int flags);
>>     typedef int
>> +(*virDrvDomainInterfacesAddresses)(virDomainPtr dom,
>> +                                   virDomainInterfacePtr **ifaces,
>> +                                   unsigned int flags);
>> +
>> +typedef int
>>   (*virDrvDomainMemoryStats)(virDomainPtr domain,
>>                              struct _virDomainMemoryStat *stats,
>>                              unsigned int nr_stats,
>> @@ -1238,6 +1243,7 @@ struct _virDriver {
>>       virDrvDomainInterfaceStats domainInterfaceStats;
>>       virDrvDomainSetInterfaceParameters domainSetInterfaceParameters;
>>       virDrvDomainGetInterfaceParameters domainGetInterfaceParameters;
>> +    virDrvDomainInterfacesAddresses domainInterfacesAddresses;
>>       virDrvDomainMemoryStats domainMemoryStats;
>>       virDrvDomainBlockPeek domainBlockPeek;
>>       virDrvDomainMemoryPeek domainMemoryPeek;
>> diff --git a/src/libvirt.c b/src/libvirt.c
>> index 07a3fd5..2d4b0bf 100644
>> --- a/src/libvirt.c
>> +++ b/src/libvirt.c
>> @@ -8643,6 +8643,97 @@ error:
>>       return -1;
>>   }
>>   + /**
>> + * virDomainInterfacesAddresses:
>> + * @dom: domain object
>> + * @ifaces: array of @dom interfaces
>
> Not correct. Note that you changed the API interface in this series.
>
>> + * @flags: extra flags; not used yet, so callers should always pass 0
>> + *
>> + * Return an array of interfaces present in given domain along with
>
> Like above, this should be changed too.
>
>> + * their IP and MAC addresses. Note that single interface can have
>> + * multiple or even 0 IP address.
>> + *
>> + * This API dynamically allocates the virDomainInterfacePtr struct
>> + * based on how many interfaces domain @dom has, usually there's
>> + * 1:1 correlation. The count of elements allocated is stored in
>> + * @ifaces_count.
>
> In this new implementation, there is now @ifaces_count, the count
> of the interfaces is returned as the return value.
>
>> + *
>> + * Note that for some hypervisors, 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 is never NULL, @ifaces->hwaddr might be 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 = virDomainInterfacesAddresses(dom, &ifaces, 
>> 0)) < 0)
>
> The argument type is "virDomainInterfacePtr **" now. So you
> have an error here.
>
>> + *     goto cleanup;
>> + *
>> + * ... do something with returned values, for example:
>> + * for (i = 0; i < ifaces_count; i++) {
>> + *     printf("name: %s", ifaces[i].name);
>
> And you will want ifaces[i]->name instead. Similiar for other memebers
> accessing.
>
>> + *     if (ifaces[i].hwaddr)
>> + *         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:
>> + * for (i = 0; i < ifaces_count; i++) {
>> + *      if(ifaces[i])
>> + *              virDomainInterfaceFree(ifaces[i]);
>
> Indention problem.
>
>> + * }
>> + * free(ifaces);
>> + *
>> + * Returns 0 on success, -1 in case of error.
>> + */
>> +int
>> +virDomainInterfacesAddresses(virDomainPtr dom,
>> +                             virDomainInterfacePtr **ifaces,
>> +                             unsigned int flags)
>> +{
>> +    virConnectPtr conn;
>> +
>> +    VIR_DOMAIN_DEBUG(dom, "ifaces=%p, flags=%x", ifaces, flags);
>> +
>> +    virResetLastError();
>> +
>> +    if (!VIR_IS_CONNECTED_DOMAIN(dom)) {
>> +        virLibDomainError(VIR_ERR_INVALID_DOMAIN, __FUNCTION__);
>> +        goto error;
>> +    }
>> +
>> +    conn = dom->conn;
>> +
>> +    virCheckNonNullArgGoto(ifaces, error);
>> +
>> +    if (conn->driver->domainInterfacesAddresses) {
>> +        int ret;
>> +        ret = conn->driver->domainInterfacesAddresses(dom, ifaces, 
>> flags);
>> +        if (ret < 0)
>> +            goto error;
>> +        return ret;
>> +    }
>> +
>> +    virLibConnError(VIR_ERR_NO_SUPPORT, __FUNCTION__);
>> +
>> +error:
>> +    virDispatchError(dom ? dom->conn : NULL);
>> +    return -1;
>> +}
>> +
>>   /**
>>    * virDomainMemoryStats:
>>    * @dom: pointer to the domain object
>> @@ -21961,3 +22052,22 @@ 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.
>> + *
>> + */
>> +void
>> +virDomainInterfaceFree(virDomainInterfacePtr iface)
>> +{
>> +    size_t i;
>> +    VIR_FREE((*iface).name);
>
> Why not "iface->name"? also you need to check if "iface" is NULL
> or not, otherwise it could cause the crash, unless the user checking
> it himself before calling virDomainInterfaceFree, but that says you
> introduce a bad api then.
>
>> +    VIR_FREE((*iface).hwaddr);
>> +    for (i = 0; i < (*iface).naddrs; i++)
>> +        VIR_FREE((*iface).addrs[i].addr);
>> +    VIR_FREE((*iface).addrs);

Also you need to free "iface" itself.

>> +}
>> diff --git a/src/libvirt_public.syms b/src/libvirt_public.syms
>> index bbdf78a..df1c166 100644
>> --- a/src/libvirt_public.syms
>> +++ b/src/libvirt_public.syms
>> @@ -634,4 +634,10 @@ LIBVIRT_1.1.1 {
>>           virDomainSetMemoryStatsPeriod;
>>   } LIBVIRT_1.1.0;
>>   +LIBVIRT_1.1.2 {
>> +    global:
>> +        virDomainInterfacesAddresses;
>> +        virDomainInterfaceFree;
>> +} LIBVIRT_1.1.1;
>> +
>>   # .... define new API here using predicted next version number ....
>
> -- 
> libvir-list mailing list
> libvir-list at redhat.com
> https://www.redhat.com/mailman/listinfo/libvir-list




More information about the libvir-list mailing list