8:13 a.m.
On Tue, Aug 27, 2013 at 6:06 PM, Daniel P. Berrange <berrange(a)redhat.com>
wrote:
On Sun, Aug 25, 2013 at 04:45:41AM +0530, Nehal J Wani wrote:
> 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 */
> > +};
>
> s/int/unsigned int/ since there's no reason for prefix to ever
> be negative.
>
> > +
> > +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);
>
> Don't put a space between the function name & opening bracket.
>
> The double pluralization reads oddly to me. I'd suggest we
> just name this 'virDomainInterfaceAddresses'
>
> ie remove the 's' from "Interfaces" here, and in all similarly
> named functions in this series.
>
The double pluralization was chosen because there are multiple
IP Addresses per interface and there are multiple interfaces. If we
choose to use 'virDomainInterfaceAddresses', then it would look
as if this function will report all the IP Addresses related to a single
Interface only.
> > +
> > +void virDomainInterfaceFree (virDomainInterfacePtr iface);
>
> Likewise.
>
> > 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..05e3a03 100644
> > --- a/src/libvirt.c
> > +++ b/src/libvirt.c
> > @@ -8643,6 +8643,94 @@ error:
> > return -1;
> > }
>
> > + /**
> > + * virDomainInterfacesAddresses:
> > + * @dom: domain object
> > + * @ifaces: pointer to an array of pointers pointing interface objects
> > + * @flags: extra flags; not used yet, so callers should always pass 0
> > + *
> > + * Return a pointer to the allocated array of 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.
> + *
> + * 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)
> + * 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:
> + * 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
> +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;
You must following existing practice and call virDispatchError(NULL)
and 'return -1' here immediately.
> + }
> +
> + 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);
This is not safe. VIR_IS_CONNECTED_DOMAIN does more than just
check whether 'dom' is NULL, so you must not dereference
'dom' if that check fails.
> +
> +/**
> + * virDomainInterfaceFree:
> + * @iface: an interface object
> + *
> + * Free the interface object. The data structure is
> + * freed and should not be used thereafter.
> + */
> +void
> +virDomainInterfaceFree(virDomainInterfacePtr iface)
> +{
> + if (iface) {
The usual coding style in libvirt for this is
if (!iface)
return;
> + size_t i;
> + VIR_FREE(iface->name);
> + VIR_FREE(iface->hwaddr);
> + for (i = 0; i < iface->naddrs; i++)
> + VIR_FREE(iface->addrs[i].addr);
> + VIR_FREE(iface->addrs);
> + }
> + VIR_FREE(iface);
> +}
Regards,
Daniel
--
|: http://berrange.com -o- http://www.flickr.com/photos/dberrange/:|
|: http://libvirt.org -o- http://virt-manager.org:|
|: http://autobuild.org -o- http://search.cpan.org/~danberr/:|
|: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc:|
--
Nehal J Wani
UG3, BTech CS+MS(CL)
IIIT-Hyderabad