1:45 a.m.
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