[libvirt] [PATCHv6 1/5] domifaddr: Implement the public APIs

Osier Yang jyang at redhat.com
Sun Sep 8 06:45:27 UTC 2013


On 08/09/13 14:43, Osier Yang wrote:
> On 06/09/13 23:18, Nehal J Wani wrote:
>
>
> The documentation for virDomainInterfaceAddresses is removed in this
> version.
>
>> 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 lease file of dnsmasq
>>    * DHCP snooping
>>
>> At this stage, it will only work with guest agent. Passing other
>> flags will result in error: "Method hasn't been implemented yet"
>
> Hm, not sure if I like this.
>
>>
>> include/libvirt/libvirt.h.in:
>>    * Define virDomainInterfaceAddresses, virDomainInterfaceFree
>>    * Define structs virDomainInterface, virDomainIPAddress
>>
>> python/generator.py:
>>    * Skip the auto-generation for virDomainInterfaceAddresses
>>      and virDomainInterfaceFree
>>
>> src/driver.h:
>>    * Define domainInterfaceAddresses
>>
>> src/libvirt.c:
>>    * Implement virDomainInterfaceAddresses
>>    * Implement virDomainInterfaceFree
>>
>> src/libvirt_public.syms:
>>    * Export the new symbols
>>
>> ---
>>   include/libvirt/libvirt.h.in |  38 ++++++++++++
>>   python/generator.py          |   3 +
>>   src/driver.h                 |   6 ++
>>   src/libvirt.c                | 135 
>> +++++++++++++++++++++++++++++++++++++++++++
>>   src/libvirt_public.syms      |   6 ++
>>   5 files changed, 188 insertions(+)
>>
>> diff --git a/include/libvirt/libvirt.h.in b/include/libvirt/libvirt.h.in
>> index a47e33c..0995080 100644
>> --- a/include/libvirt/libvirt.h.in
>> +++ b/include/libvirt/libvirt.h.in
>> @@ -2044,6 +2044,44 @@ int virDomainGetInterfaceParameters 
>> (virDomainPtr dom,
>> virTypedParameterPtr params,
>>                                                            int 
>> *nparams, unsigned int flags);
>>   +typedef enum {
>> +    VIR_DOMAIN_INTERFACE_ADDRESSES_SNOOP = (1 << 0), /* Snoop 
>> traffic */
>> +    VIR_DOMAIN_INTERFACE_ADDRESSES_LEASE = (1 << 1), /* Parse DHCP 
>> lease file */
>> +    VIR_DOMAIN_INTERFACE_ADDRESSES_AGENT = (1 << 2), /* Query qemu 
>> guest agent */
>
> I don't see a good reason for expose the flags which are not supported 
> yet. It will
> keep biting us until the implementations are done. Especially when you 
> have
> documentations for them in virsh, API comments. (I see you exposed 
> them as
> options in 4/5).
>
> It's the experience of a storage volume command "virsh vol-resize", 
> which exposed
> the options "--shrink" and "--allocate" when the volResize API was 
> introduced, but
> the implementations for them were done after a long time. And as said, 
> it bugged us.
>
> Even if we can get the other 2 methods implemented very soon, I guess 
> it won't be
> achieved in the same release of the API itself.
>
> Although what you did is a bit better than volResize (you have a more 
> sensible error
> "Method has not been implemented yet"), I'm still not fan of exposing 
> flags not
> supported yet.
>
>> +} virDomainInterfaceAddressesFlags;
>> +
>> +typedef enum {
>> +    VIR_IP_ADDR_TYPE_IPV4 = 0,
>> +    VIR_IP_ADDR_TYPE_IPV6 = 1,
>> +
>> +#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 */
>> +    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 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 fb321c6..50f779b 100755
>> --- a/python/generator.py
>> +++ b/python/generator.py
>> @@ -458,6 +458,7 @@ skip_impl = (
>>       'virNodeGetMemoryParameters',
>>       'virNodeSetMemoryParameters',
>>       'virNodeGetCPUMap',
>> +    'virDomainInterfaceAddresses',
>>       'virDomainMigrate3',
>>       'virDomainMigrateToURI3',
>>   )
>> @@ -560,6 +561,8 @@ skip_function = (
>>       "virTypedParamsGetString",
>>       "virTypedParamsGetUInt",
>>       "virTypedParamsGetULLong",
>> +
>> +    "virDomainInterfaceFree", # Only useful in C
>>   )
>>     lxc_skip_function = (
>> diff --git a/src/driver.h b/src/driver.h
>> index be64333..210a910 100644
>> --- a/src/driver.h
>> +++ b/src/driver.h
>> @@ -518,6 +518,11 @@ typedef int
>>                                         unsigned int flags);
>>     typedef int
>> +(*virDrvDomainInterfaceAddresses)(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;
>> +    virDrvDomainInterfaceAddresses domainInterfaceAddresses;
>>       virDrvDomainMemoryStats domainMemoryStats;
>>       virDrvDomainBlockPeek domainBlockPeek;
>>       virDrvDomainMemoryPeek domainMemoryPeek;
>> diff --git a/src/libvirt.c b/src/libvirt.c
>> index 07a3fd5..7a5759b 100644
>> --- a/src/libvirt.c
>> +++ b/src/libvirt.c
>> @@ -8643,6 +8643,116 @@ error:
>>       return -1;
>>   }
>>   + /**
>> + * virDomainInterfaceAddresses:
>> + * @dom: domain object
>> + * @ifaces: pointer to an array of pointers pointing to interface 
>> objects
>> + * @flags: bitwise-OR of virDomainInterfaceAddressesFlags
>> + *
>> + * Return a pointer to the allocated array of pointers pointing to 
>> interfaces
>> + * present in given domain along with 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 the interfaces is returned as the return value.
>> + *
>> + * In case @flags includes VIR_DOMAIN_INTERFACE_ADDRESSES_AGENT, a 
>> configured
>
> [1], because the AGENT flag will be exclusive with the other two flags,
> you should s/includes/is/,
>
>> + * guest agent is needed for successful return from this API. 
>> Moreover, if
>
> s/successful return/successfully returning/,
>
>
>> + * guest agent is used then the interface name is the one seen by 
>> guest OS.
>
> I think "if guest is used" can be removed, since the "flags is _AGENT"
> already implied it.
>
>> + * To match such interface with the one from @dom XML use MAC 
>> address or IP
>> + * range.
>
> s/XML use/XML, use/,
>
>> + *
>> + * Both the methods, pasrsing leases file and traffic snooping will 
>> return
>
> s/pasrsing/parsing/,
>
>> + * the interface name of the form "vnetN", which is different from 
>> what guest
>> + * agent returns (like ethN or emN).
>
> Guest OS could returns "vnetN", if it has a virtual network interface 
> named
> like that, so may be just using "(the interface name seen by guest 
> OS)" is
> better.
>
>> And since the MAC address from guest agent
>> + * might be different with what @dom XML specifies, we have no way 
>> to convert it
>> + * into the names present in @dom config. Hence, it is not 
>> recommended to mix
>
> To be consistent, please use either "@dom XML" or "@dom config".
>
>> + * the flag ..._AGENT with ..._LEASE or ..._SNOOP as it may lead to 
>> ambiguous
>
> Not good to have strings like "...._AGENT" in documents.
>
>> + * results because we cannot be sure if the name came from the agent 
>> or from
>> + * the other method.
>
> That says _AGENT flag is exclusive with the other two flags. See [1]
>
> But anyway, since I think we shouldn't expose the flags not 
> implemented yet,
> above comments are not needed then.
>
>> + *
>> + * If 0 is passed as @flags, libvirt will choose the best way, and 
>> won't
>> + * include agent in it.
>
> I'm thinking if we don't need to be so complicated, by defaulting to one
> of _LEASE or _SNOOP explicitly.
>
> That says (conclusion of all of above), I'm wondering if define the 
> enum like
> blow is better.
>
>
> typedef enum {
>     VIR_DOMAIN_INTERFACE_ADDRESSES_AGENT = (1 << 1), /* Query qemu 
> guest agent */

Btw, the API might work for other guest agent too, not only *qemu* guest 
agent. So
s/qemu guest/guest/,

> }
>
> I.E. 0 and (1 << 0) is reserved for lease and snooping methods.
>
>> + *
>> + * @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, 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]->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)
>> + *         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 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__);
>> +        virDispatchError(NULL);
>> +        return -1;
>> +    }
>> +
>> +    conn = dom->conn;
>> +
>> +    virCheckNonNullArgGoto(ifaces, error);
>> +
>> +    if ((flags & VIR_DOMAIN_INTERFACE_ADDRESSES_AGENT)
>> +        && (conn->flags & VIR_CONNECT_RO)) {
>> +        virLibConnError(VIR_ERR_OPERATION_DENIED, "%s",
>> +                        _("Cannot query guest agent in readonly 
>> mode"));
>
> Follow what other APIs do:
>
>         virLibDomainError(VIR_ERR_OPERATION_DENIED, __FUNCTION__);
>         goto error;
>
> Osier




More information about the libvir-list mailing list