<div dir="ltr"> On Tue, Aug 27, 2013 at 6:06 PM, Daniel P. Berrange <<a href="mailto:berrange@redhat.com">berrange@redhat.com</a>> wrote: > > On Sun, Aug 25, 2013 at 04:45:41AM +0530, Nehal J Wani wrote: > > diff --git a/include/libvirt/<a href="http://libvirt.h.in">libvirt.h.in</a> b/include/libvirt/<a href="http://libvirt.h.in">libvirt.h.in</a> > > index a47e33c..deb1e1f 100644 > > --- a/include/libvirt/<a href="http://libvirt.h.in">libvirt.h.in</a> > > +++ b/include/libvirt/<a href="http://libvirt.h.in">libvirt.h.in</a> > > @@ -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. ><div> <div>The double pluralization was chosen because there are multiple</div><div>IP Addresses per interface and there are multiple interfaces. If we</div><div>choose to use 'virDomainInterfaceAddresses', then it would look</div> <div>as if this function will report all the IP Addresses related to a single</div><div>Interface only.</div><div> > > + > > +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 > -- > |: <a href="http://berrange.com">http://berrange.com</a> -o- <a href="http://www.flickr.com/photos/dberrange/">http://www.flickr.com/photos/dberrange/</a> :| > |: <a href="http://libvirt.org">http://libvirt.org</a> -o- <a href="http://virt-manager.org">http://virt-manager.org</a> :| > |: <a href="http://autobuild.org">http://autobuild.org</a> -o- <a href="http://search.cpan.org/~danberr/">http://search.cpan.org/~danberr/</a> :| > |: <a href="http://entangle-photo.org">http://entangle-photo.org</a> -o- <a href="http://live.gnome.org/gtk-vnc">http://live.gnome.org/gtk-vnc</a> :| -- Nehal J Wani UG3, BTech CS+MS(CL) IIIT-Hyderabad </div></div></div>