[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