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.

+ +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 :|

On Tue, Aug 27, 2013 at 06:43:06PM +0530, Nehal J Wani wrote:

On Tue, Aug 27, 2013 at 6:06 PM, Daniel P. Berrange <berrange@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.

I don't think that interpretation is really a problem. The double pluralization is worse imho. 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 :|

On 25/08/13 07:15, Nehal J Wani wrote:

Implement RPC calls for virDomainInterfacesAddresses

daemon/remote.c * Define remoteSerializeDomainInterfacePtr, remoteDispatchDomainInterfacesAddresses

src/remote/remote_driver.c * Define remoteDomainInterfacesAddresses

src/remote/remote_protocol.x * New RPC procedure: REMOTE_PROC_DOMAIN_INTERFACES_ADDRESSES * Define structs remote_domain_ip_addr, remote_domain_interface, remote_domain_interfaces_addresses_args, remote_domain_interfaces_addresses_ret

src/remote_protocol-structs * New structs added

--- daemon/remote.c | 127 +++++++++++++++++++++++++++++++++++++++++++ src/remote/remote_driver.c | 85 +++++++++++++++++++++++++++++ src/remote/remote_protocol.x | 41 +++++++++++++- src/remote_protocol-structs | 24 ++++++++ 4 files changed, 276 insertions(+), 1 deletion(-)

diff --git a/daemon/remote.c b/daemon/remote.c index 03d5557..44d7ff2 100644 --- a/daemon/remote.c +++ b/daemon/remote.c @@ -5025,7 +5025,134 @@ cleanup: return rv; }

+static int +remoteSerializeDomainInterfacePtr(virDomainInterfacePtr *ifaces, + unsigned int ifaces_count, + remote_domain_interfaces_addresses_ret *ret) +{ + size_t i, j; + + if (ifaces_count > REMOTE_DOMAIN_INTERFACE_MAX) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Number of interfaces exceeds max limit!")); + return -1; + } + + if (VIR_ALLOC_N(ret->ifaces.ifaces_val, ifaces_count) < 0) + return -1; + + ret->ifaces.ifaces_len = ifaces_count; + + for (i = 0; i < ifaces_count; i++) { + virDomainInterfacePtr iface = ifaces[i]; + remote_domain_interface *iface_ret = &(ret->ifaces.ifaces_val[i]);

+ if ((VIR_STRDUP(iface_ret->name, iface->name)) < 0) + goto cleanup; + + if (iface->hwaddr) { + char **hwaddr_p = NULL; + if (VIR_ALLOC(hwaddr_p) < 0) + goto cleanup; + if (VIR_STRDUP(*hwaddr_p, iface->hwaddr) < 0) { + VIR_FREE(hwaddr_p); + goto cleanup; + } + + iface_ret->hwaddr = hwaddr_p; + } + + if (iface->naddrs > REMOTE_DOMAIN_IP_ADDR_MAX) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Number of IP addresses exceeds max limit!")); + goto cleanup; + } + + if (VIR_ALLOC_N(iface_ret->addrs.addrs_val, + iface->naddrs) < 0) + goto cleanup; + + iface_ret->addrs.addrs_len = iface->naddrs; + + for (j = 0; j < iface->naddrs; j++) { + virDomainIPAddressPtr ip_addr = &(iface->addrs[j]); + remote_domain_ip_addr *ip_addr_ret = + &(iface_ret->addrs.addrs_val[j]); + + if (VIR_STRDUP(ip_addr_ret->addr, ip_addr->addr) < 0) + goto cleanup; + + ip_addr_ret->prefix = ip_addr->prefix; + ip_addr_ret->type = ip_addr->type; + } + } + + return 0; + +cleanup: + if (ret->ifaces.ifaces_val) { + for (i = 0; i < ifaces_count; i++) { + remote_domain_interface *iface_ret = &(ret->ifaces.ifaces_val[i]); + VIR_FREE(iface_ret->name); + VIR_FREE(iface_ret->hwaddr); + for (j = 0; j < iface_ret->addrs.addrs_len; j++) { + remote_domain_ip_addr *ip_addr = + &(iface_ret->addrs.addrs_val[j]); + VIR_FREE(ip_addr->addr); + } + VIR_FREE(iface_ret); + } + VIR_FREE(ret->ifaces.ifaces_val); + } + + return -1; +} + +static int +remoteDispatchDomainInterfacesAddresses( + virNetServerPtr server ATTRIBUTE_UNUSED, + virNetServerClientPtr client, + virNetMessagePtr msg ATTRIBUTE_UNUSED, + virNetMessageErrorPtr rerr, + remote_domain_interfaces_addresses_args *args, + remote_domain_interfaces_addresses_ret *ret) +{ + int rv = -1; + virDomainPtr dom = NULL; + virDomainInterfacePtr *ifaces = NULL; + int ifaces_count = 0; + struct daemonClientPrivate *priv = + virNetServerClientGetPrivateData(client); + + if (!priv->conn) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("connection not open")); + goto cleanup; + } + + if (!(dom = get_nonnull_domain(priv->conn, args->dom))) + goto cleanup; + + if ((ifaces_count = virDomainInterfacesAddresses(dom, &ifaces, args->flags)) < 0) + goto cleanup; + + if (remoteSerializeDomainInterfacePtr(ifaces, ifaces_count, ret) < 0) + goto cleanup; + + rv = 0; + +cleanup: + if (rv < 0) + virNetMessageSaveError(rerr); + if (dom) + virDomainFree(dom);

As I explained in previous series, no need for the checking.

+ if (ifaces) { + size_t i; + for (i = 0; i < ifaces_count; i++) + virDomainInterfaceFree(ifaces[i]); + VIR_FREE(ifaces); + } + return rv; +}

/*----- Helpers. -----*/

diff --git a/src/remote/remote_driver.c b/src/remote/remote_driver.c index 71d0034..94eb63d 100644 --- a/src/remote/remote_driver.c +++ b/src/remote/remote_driver.c @@ -6477,6 +6477,90 @@ done: return rv; }

+static int +remoteDomainInterfacesAddresses(virDomainPtr dom, + virDomainInterfacePtr **ifaces, + unsigned int flags) +{ + int rv = -1; + size_t i, j; + + virDomainInterfacePtr *ifaces_ret = NULL; + remote_domain_interfaces_addresses_args args; + remote_domain_interfaces_addresses_ret ret; + + struct private_data *priv = dom->conn->privateData; + + args.flags = flags; + make_nonnull_domain(&args.dom, dom); + + remoteDriverLock(priv); + + memset(&ret, 0, sizeof(ret)); + + if (call(dom->conn, priv, 0, REMOTE_PROC_DOMAIN_INTERFACES_ADDRESSES, + (xdrproc_t)xdr_remote_domain_interfaces_addresses_args, + (char *)&args, + (xdrproc_t)xdr_remote_domain_interfaces_addresses_ret, + (char *)&ret) == -1) { + goto done; + } + + if (ret.ifaces.ifaces_len && + VIR_ALLOC_N(ifaces_ret, ret.ifaces.ifaces_len) < 0) + goto cleanup; + + for (i = 0; i < ret.ifaces.ifaces_len; i++) { + if (VIR_ALLOC(ifaces_ret[i]) < 0) + goto cleanup; + + virDomainInterfacePtr iface = ifaces_ret[i]; + remote_domain_interface *iface_ret = &(ret.ifaces.ifaces_val[i]); + + if (VIR_STRDUP(iface->name, iface_ret->name) < 0) + goto cleanup; + + if (iface_ret->hwaddr && + VIR_STRDUP(iface->hwaddr, *iface_ret->hwaddr) < 0) + goto cleanup; + + iface->naddrs = iface_ret->addrs.addrs_len; + + if (iface->naddrs) { + if (VIR_ALLOC_N(iface->addrs, iface->naddrs) < 0) + goto cleanup; + + for (j = 0; j < iface->naddrs; j++) { + virDomainIPAddressPtr ip_addr = &(iface->addrs[j]); + remote_domain_ip_addr *ip_addr_ret = + &(iface_ret->addrs.addrs_val[j]); + + if (VIR_STRDUP(ip_addr->addr, ip_addr_ret->addr) < 0) { + goto cleanup; + }

No need for the {}.

+ ip_addr->prefix = ip_addr_ret->prefix; + ip_addr->type = ip_addr_ret->type; + } + } + } + *ifaces = ifaces_ret; + ifaces_ret = NULL; + + rv = ret.ifaces.ifaces_len; + +cleanup: + if (ifaces_ret) { + for (i = 0; i < ret.ifaces.ifaces_len; i++) + virDomainInterfaceFree(ifaces_ret[i]); + VIR_FREE(ifaces_ret); + } + xdr_free((xdrproc_t)xdr_remote_domain_interfaces_addresses_ret, + (char *) &ret); +done: + remoteDriverUnlock(priv); + return rv; +} + static void remoteDomainEventQueue(struct private_data *priv, virDomainEventPtr event) { @@ -6811,6 +6895,7 @@ static virDriver remote_driver = { .domainMigratePerform3Params = remoteDomainMigratePerform3Params, /* 1.1.0 */ .domainMigrateFinish3Params = remoteDomainMigrateFinish3Params, /* 1.1.0 */ .domainMigrateConfirm3Params = remoteDomainMigrateConfirm3Params, /* 1.1.0 */ + .domainInterfacesAddresses = remoteDomainInterfacesAddresses, /* 1.1.2 */ };

static virNetworkDriver network_driver = { diff --git a/src/remote/remote_protocol.x b/src/remote/remote_protocol.x index 7cfebdf..86d4da8 100644 --- a/src/remote/remote_protocol.x +++ b/src/remote/remote_protocol.x @@ -237,6 +237,17 @@ const REMOTE_NODE_MEMORY_PARAMETERS_MAX = 64; /* UUID. VIR_UUID_BUFLEN definition comes from libvirt.h */ typedef opaque remote_uuid[VIR_UUID_BUFLEN];

+/* + * Upper limit on number of interfaces per domain + */ +const REMOTE_DOMAIN_INTERFACE_MAX = 32; + +/* + * Upper limit on number of ip addresses per interface

s/ip/IP/,

+ */ +const REMOTE_DOMAIN_IP_ADDR_MAX = 16;

hm, are these 2 limits too small? I don't believe one host could only have up to 32 interfaces. Any evidence for you to descrease them in new series? Osier

On Sun, Aug 25, 2013 at 04:45:42AM +0530, Nehal J Wani wrote:

diff --git a/daemon/remote.c b/daemon/remote.c index 03d5557..44d7ff2 100644 --- a/daemon/remote.c +++ b/daemon/remote.c @@ -5025,7 +5025,134 @@ cleanup: return rv; }

+static int +remoteSerializeDomainInterfacePtr(virDomainInterfacePtr *ifaces, + unsigned int ifaces_count, + remote_domain_interfaces_addresses_ret *ret) +{ + size_t i, j; + + if (ifaces_count > REMOTE_DOMAIN_INTERFACE_MAX) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Number of interfaces exceeds max limit!"));

When reporting this error, include the actual 'ifaces_count' value and the limit in the error message. No need for an '!' either.

+ if (VIR_ALLOC_N(ret->ifaces.ifaces_val, ifaces_count) < 0) + return -1; + + ret->ifaces.ifaces_len = ifaces_count; + + for (i = 0; i < ifaces_count; i++) { + virDomainInterfacePtr iface = ifaces[i]; + remote_domain_interface *iface_ret = &(ret->ifaces.ifaces_val[i]);

+ if ((VIR_STRDUP(iface_ret->name, iface->name)) < 0) + goto cleanup; + + if (iface->hwaddr) { + char **hwaddr_p = NULL; + if (VIR_ALLOC(hwaddr_p) < 0) + goto cleanup; + if (VIR_STRDUP(*hwaddr_p, iface->hwaddr) < 0) { + VIR_FREE(hwaddr_p); + goto cleanup; + } + + iface_ret->hwaddr = hwaddr_p; + } + + if (iface->naddrs > REMOTE_DOMAIN_IP_ADDR_MAX) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Number of IP addresses exceeds max limit!"));

Again, include the actual values checked, and remove the '!'.

+ goto cleanup; + } + + if (VIR_ALLOC_N(iface_ret->addrs.addrs_val, + iface->naddrs) < 0) + goto cleanup; + + iface_ret->addrs.addrs_len = iface->naddrs; + + for (j = 0; j < iface->naddrs; j++) { + virDomainIPAddressPtr ip_addr = &(iface->addrs[j]); + remote_domain_ip_addr *ip_addr_ret = + &(iface_ret->addrs.addrs_val[j]); + + if (VIR_STRDUP(ip_addr_ret->addr, ip_addr->addr) < 0) + goto cleanup; + + ip_addr_ret->prefix = ip_addr->prefix; + ip_addr_ret->type = ip_addr->type; + } + } + + return 0; + +cleanup: + if (ret->ifaces.ifaces_val) { + for (i = 0; i < ifaces_count; i++) { + remote_domain_interface *iface_ret = &(ret->ifaces.ifaces_val[i]); + VIR_FREE(iface_ret->name); + VIR_FREE(iface_ret->hwaddr); + for (j = 0; j < iface_ret->addrs.addrs_len; j++) { + remote_domain_ip_addr *ip_addr = + &(iface_ret->addrs.addrs_val[j]); + VIR_FREE(ip_addr->addr); + } + VIR_FREE(iface_ret); + } + VIR_FREE(ret->ifaces.ifaces_val); + } + + return -1; +} + +static int +remoteDispatchDomainInterfacesAddresses( + virNetServerPtr server ATTRIBUTE_UNUSED, + virNetServerClientPtr client, + virNetMessagePtr msg ATTRIBUTE_UNUSED, + virNetMessageErrorPtr rerr, + remote_domain_interfaces_addresses_args *args, + remote_domain_interfaces_addresses_ret *ret) +{ + int rv = -1; + virDomainPtr dom = NULL; + virDomainInterfacePtr *ifaces = NULL; + int ifaces_count = 0; + struct daemonClientPrivate *priv = + virNetServerClientGetPrivateData(client); + + if (!priv->conn) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("connection not open")); + goto cleanup; + } + + if (!(dom = get_nonnull_domain(priv->conn, args->dom))) + goto cleanup; + + if ((ifaces_count = virDomainInterfacesAddresses(dom, &ifaces, args->flags)) < 0) + goto cleanup; + + if (remoteSerializeDomainInterfacePtr(ifaces, ifaces_count, ret) < 0) + goto cleanup; + + rv = 0; + +cleanup: + if (rv < 0) + virNetMessageSaveError(rerr); + if (dom) + virDomainFree(dom); + if (ifaces) { + size_t i; + for (i = 0; i < ifaces_count; i++) + virDomainInterfaceFree(ifaces[i]); + VIR_FREE(ifaces); + } + return rv; +}

/*----- Helpers. -----*/

diff --git a/src/remote/remote_driver.c b/src/remote/remote_driver.c index 71d0034..94eb63d 100644 --- a/src/remote/remote_driver.c +++ b/src/remote/remote_driver.c @@ -6477,6 +6477,90 @@ done: return rv; }

+static int +remoteDomainInterfacesAddresses(virDomainPtr dom, + virDomainInterfacePtr **ifaces, + unsigned int flags) +{ + int rv = -1; + size_t i, j; + + virDomainInterfacePtr *ifaces_ret = NULL; + remote_domain_interfaces_addresses_args args; + remote_domain_interfaces_addresses_ret ret; + + struct private_data *priv = dom->conn->privateData; + + args.flags = flags; + make_nonnull_domain(&args.dom, dom); + + remoteDriverLock(priv); + + memset(&ret, 0, sizeof(ret)); + + if (call(dom->conn, priv, 0, REMOTE_PROC_DOMAIN_INTERFACES_ADDRESSES, + (xdrproc_t)xdr_remote_domain_interfaces_addresses_args, + (char *)&args, + (xdrproc_t)xdr_remote_domain_interfaces_addresses_ret, + (char *)&ret) == -1) { + goto done; + } +

You must check ret.ifaces.ifaces_len against REMOTE_DOMAIN_INTERFACE_MAX here before allocating the array.

+ if (ret.ifaces.ifaces_len && + VIR_ALLOC_N(ifaces_ret, ret.ifaces.ifaces_len) < 0) + goto cleanup; + + for (i = 0; i < ret.ifaces.ifaces_len; i++) { + if (VIR_ALLOC(ifaces_ret[i]) < 0) + goto cleanup; + + virDomainInterfacePtr iface = ifaces_ret[i]; + remote_domain_interface *iface_ret = &(ret.ifaces.ifaces_val[i]); + + if (VIR_STRDUP(iface->name, iface_ret->name) < 0) + goto cleanup; + + if (iface_ret->hwaddr && + VIR_STRDUP(iface->hwaddr, *iface_ret->hwaddr) < 0) + goto cleanup; + + iface->naddrs = iface_ret->addrs.addrs_len;

And here you must validate addrs_len against its limit too.

+ + if (iface->naddrs) { + if (VIR_ALLOC_N(iface->addrs, iface->naddrs) < 0) + goto cleanup; + + for (j = 0; j < iface->naddrs; j++) { + virDomainIPAddressPtr ip_addr = &(iface->addrs[j]); + remote_domain_ip_addr *ip_addr_ret = + &(iface_ret->addrs.addrs_val[j]); + + if (VIR_STRDUP(ip_addr->addr, ip_addr_ret->addr) < 0) { + goto cleanup; + } + ip_addr->prefix = ip_addr_ret->prefix; + ip_addr->type = ip_addr_ret->type; + } + } + } + *ifaces = ifaces_ret; + ifaces_ret = NULL; + + rv = ret.ifaces.ifaces_len; + +cleanup: + if (ifaces_ret) { + for (i = 0; i < ret.ifaces.ifaces_len; i++) + virDomainInterfaceFree(ifaces_ret[i]); + VIR_FREE(ifaces_ret); + } + xdr_free((xdrproc_t)xdr_remote_domain_interfaces_addresses_ret, + (char *) &ret); +done: + remoteDriverUnlock(priv); + return rv; +}

@@ -2837,6 +2848,27 @@ struct remote_domain_event_device_removed_msg { remote_nonnull_string devAlias; };

+struct remote_domain_ip_addr { + int type; + remote_nonnull_string addr; + int prefix;

Lets make this 'unsigned int' to match the public API change i suggested.

+struct remote_domain_interface { + remote_nonnull_string name; + remote_string hwaddr; + remote_domain_ip_addr addrs<REMOTE_DOMAIN_IP_ADDR_MAX>; +}; + +struct remote_domain_interfaces_addresses_args { + remote_nonnull_domain dom; + unsigned int flags; +}; + +struct remote_domain_interfaces_addresses_ret { + remote_domain_interface ifaces<REMOTE_DOMAIN_INTERFACE_MAX>; +};

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 :|

On 25/08/13 07:15, Nehal J Wani wrote:

By querying the qemu guest agent with the QMP command "guest-network-get-interfaces" and converting the received JSON output to structured objects.

src/qemu/qemu_agent.h: * Define qemuAgentGetInterfaces

src/qemu/qemu_agent.c: * Implement qemuAgentGetInterface

src/qemu/qemu_driver.c: * New function qemuDomainInterfacesAddresses

src/remote_protocol-sructs: * Define new structs

tests/qemuagenttest.c: * Add new test: testQemuAgentGetInterfaces

--- src/qemu/qemu_agent.c | 153 +++++++++++++++++++++++++++++++++++++++++++++++++ src/qemu/qemu_agent.h | 3 + src/qemu/qemu_driver.c | 55 ++++++++++++++++++ tests/qemuagenttest.c | 113 ++++++++++++++++++++++++++++++++++++ 4 files changed, 324 insertions(+)

diff --git a/src/qemu/qemu_agent.c b/src/qemu/qemu_agent.c index 2cd0ccc..11f5467 100644 --- a/src/qemu/qemu_agent.c +++ b/src/qemu/qemu_agent.c @@ -1319,6 +1319,159 @@ cleanup: return ret; }

+ +int +qemuAgentGetInterfaces(qemuAgentPtr mon, + virDomainInterfacePtr **ifaces) +{ + int ret = -1; + size_t i, j; + int size = -1; + virJSONValuePtr cmd = NULL; + virJSONValuePtr reply = NULL; + virJSONValuePtr ret_array = NULL; + int ifaces_count = 0; + virDomainInterfacePtr *ifaces_ret = NULL; + + if (!(cmd = qemuAgentMakeCommand("guest-network-get-interfaces", NULL))) + return -1; + + if (qemuAgentCommand(mon, cmd, &reply, VIR_DOMAIN_QEMU_AGENT_COMMAND_BLOCK) < 0 || + qemuAgentCheckError(cmd, reply) < 0) { + goto cleanup; + } + + if (!(ret_array = virJSONValueObjectGet(reply, "return"))) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("qemu agent didn't provide 'return' field")); + goto cleanup; + } + + if ((size = virJSONValueArraySize(ret_array)) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("qemu agent didn't return an array of interfaces")); + goto cleanup; + } + + ifaces_count = (unsigned int) size; + + if (VIR_ALLOC_N(ifaces_ret, size) < 0) + goto cleanup; + + for (i = 0; i < size; i++) { + virJSONValuePtr tmp_iface = virJSONValueArrayGet(ret_array, i); + virJSONValuePtr ip_addr_arr = NULL; + const char *name, *hwaddr; + int ip_addr_arr_size; + + if (VIR_ALLOC(ifaces_ret[i]) < 0) + goto cleanup; + + /* Shouldn't happen but doesn't hurt to check neither */ + if (!tmp_iface) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("something has went really wrong")); + goto error; + } + + /* interface name is required to be presented */ + name = virJSONValueObjectGetString(tmp_iface, "name"); + if (!name) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("qemu agent didn't provide 'name' field")); + goto error; + } + + if (VIR_STRDUP(ifaces_ret[i]->name, name) < 0) + goto error; + + /* hwaddr might be omitted */ + hwaddr = virJSONValueObjectGetString(tmp_iface, "hardware-address"); + if (hwaddr && VIR_STRDUP(ifaces_ret[i]->hwaddr, hwaddr) < 0) + goto error; + + /* as well as IP address which - moreover - + * can be presented multiple times */ + ip_addr_arr = virJSONValueObjectGet(tmp_iface, "ip-addresses"); + if (!ip_addr_arr) + continue; + + if ((ip_addr_arr_size = virJSONValueArraySize(ip_addr_arr)) < 0) + /* Mmm, empty 'ip-address'? */ + continue; + + (*(ifaces_ret)[i]).naddrs = (unsigned int) ip_addr_arr_size; + + if (VIR_ALLOC_N((*(ifaces_ret)[i]).addrs, ip_addr_arr_size) < 0) + goto error; + + for (j = 0; j < ip_addr_arr_size; j++) { + virJSONValuePtr ip_addr_obj = virJSONValueArrayGet(ip_addr_arr, j); + virDomainIPAddressPtr ip_addr = &ifaces_ret[i]->addrs[j]; + const char *type, *addr; + + /* Shouldn't happen but doesn't hurt to check neither */ + if (!ip_addr_obj) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("something has went really wrong")); + goto error; + } + + type = virJSONValueObjectGetString(ip_addr_obj, "ip-address-type"); + if (!type) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("qemu agent didn't provide 'ip-address-type'" + " field for interface '%s'"), name); + goto error; + } else if (STREQ(type, "ipv4")) { + ip_addr->type = VIR_IP_ADDR_TYPE_IPV4; + } else if (STREQ(type, "ipv6")) { + ip_addr->type = VIR_IP_ADDR_TYPE_IPV6; + } else { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("unknown ip address type '%s'"), + type); + goto error; + } + + addr = virJSONValueObjectGetString(ip_addr_obj, "ip-address"); + if (!addr) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("qemu agent didn't provide 'ip-address'" + " field for interface '%s'"), name); + goto error; + } + if (VIR_STRDUP(ip_addr->addr, addr) < 0) + goto error; + + if (virJSONValueObjectGetNumberInt(ip_addr_obj, "prefix", + &ip_addr->prefix) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("malformed 'prefix' field")); + goto error; + } + } + } + + *ifaces = ifaces_ret; + ifaces_ret = NULL; + ret = ifaces_count; + +cleanup: + virJSONValueFree(cmd); + virJSONValueFree(reply); + return ret; + +error: + if (ifaces_ret) { + for (i = 0; i < ifaces_count; i++) + virDomainInterfaceFree(ifaces_ret[i]); + } + VIR_FREE(ifaces_ret); + goto cleanup; +} + + /* * qemuAgentFSThaw: * @mon: Agent diff --git a/src/qemu/qemu_agent.h b/src/qemu/qemu_agent.h index 5fbacdb..58b56fd 100644 --- a/src/qemu/qemu_agent.h +++ b/src/qemu/qemu_agent.h @@ -76,6 +76,9 @@ int qemuAgentFSThaw(qemuAgentPtr mon); int qemuAgentSuspend(qemuAgentPtr mon, unsigned int target);

+int qemuAgentGetInterfaces(qemuAgentPtr mon, + virDomainInterfacePtr **ifaces); + int qemuAgentArbitraryCommand(qemuAgentPtr mon, const char *cmd, char **result, diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 0a8e518..0cd266f 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -16035,6 +16035,60 @@ qemuNodeSuspendForDuration(virConnectPtr conn, return nodeSuspendForDuration(target, duration, flags); }

+static int +qemuDomainInterfacesAddresses(virDomainPtr dom, + virDomainInterfacePtr **ifaces, + unsigned int flags) +{ + virQEMUDriverPtr driver = dom->conn->privateData; + qemuDomainObjPrivatePtr priv = NULL; + virDomainObjPtr vm = NULL; + int ret = -1; + + virCheckFlags(0, -1); + + if (!(vm = qemuDomObjFromDomain(dom))) + goto cleanup; + + if (!virDomainObjIsActive(vm)) { + virReportError(VIR_ERR_OPERATION_INVALID, "%s", + _("domain is not running")); + goto cleanup; + } + + priv = vm->privateData; + + if (virDomainInterfacesAddressesEnsureACL(dom->conn, vm->def) < 0) + goto cleanup; + + if (priv->agentError) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("QEMU guest agent is not " + "available due to an error")); + goto cleanup; + } + + if (!priv->agent) { + virReportError(VIR_ERR_ARGUMENT_UNSUPPORTED, "%s", + _("QEMU guest agent is not configured")); + goto cleanup; + } + + if (qemuDomainObjBeginJob(driver, vm, QEMU_JOB_QUERY) < 0) + goto cleanup; + + qemuDomainObjEnterAgent(vm); + ret = qemuAgentGetInterfaces(priv->agent, ifaces); + qemuDomainObjExitAgent(vm); + + if (qemuDomainObjEndJob(driver, vm) == 0) + vm = NULL; + +cleanup: + if (vm) + virObjectUnlock(vm); + return ret; +}

static virDriver qemuDriver = { .no = VIR_DRV_QEMU, @@ -16130,6 +16184,7 @@ static virDriver qemuDriver = { .domainMemoryPeek = qemuDomainMemoryPeek, /* 0.4.4 */ .domainGetBlockInfo = qemuDomainGetBlockInfo, /* 0.8.1 */ .nodeGetCPUStats = qemuNodeGetCPUStats, /* 0.9.3 */ + .domainInterfacesAddresses = qemuDomainInterfacesAddresses, /* 1.1.2 */ .nodeGetMemoryStats = qemuNodeGetMemoryStats, /* 0.9.3 */ .nodeGetCellsFreeMemory = qemuNodeGetCellsFreeMemory, /* 0.4.4 */ .nodeGetFreeMemory = qemuNodeGetFreeMemory, /* 0.4.4 */ diff --git a/tests/qemuagenttest.c b/tests/qemuagenttest.c index 4e27981..71e7949 100644 --- a/tests/qemuagenttest.c +++ b/tests/qemuagenttest.c @@ -271,6 +271,7 @@ cleanup: }

+

Meaningless new blank line.

static int testQemuAgentShutdown(const void *data) { @@ -576,6 +577,117 @@ cleanup: return ret; }

+static const char testQemuAgentGetInterfacesResponse[] = + "{\"return\": " + " [" + " {\"name\":\"lo\"," + " \"ip-addresses\":" + " [" + " {\"ip-address-type\":\"ipv4\"," + " \"ip-address\":\"127.0.0.1\"," + " \"prefix\":8" + " }," + " {\"ip-address-type\":\"ipv6\"," + " \"ip-address\":\"::1\"," + " \"prefix\":128" + " }" + " ]," + " \"hardware-address\":\"00:00:00:00:00:00\"" + " }," + " {\"name\":\"eth0\"," + " \"ip-addresses\":" + " [" + " {\"ip-address-type\":\"ipv4\"," + " \"ip-address\":\"192.168.102.142\"," + " \"prefix\":24" + " }," + " {\"ip-address-type\":\"ipv6\"," + " \"ip-address\":\"fe80::5054:ff:fe89:ad35\"," + " \"prefix\":64" + " }" + " ]," + " \"hardware-address\":\"52:54:00:89:ad:35\"" + " }," + " {\"name\":\"eth1\"," + " \"ip-addresses\":" + " [" + " {\"ip-address-type\":\"ipv4\"," + " \"ip-address\":\"192.168.103.83\"," + " \"prefix\":24" + " }," + " {\"ip-address-type\":\"ipv6\"," + " \"ip-address\":\"fe80::5054:ff:fed3:39ee\"," + " \"prefix\":64" + " }" + " ]," + " \"hardware-address\":\"52:54:00:d3:39:ee\"" + " }" + " ]" + "}"; + +static int +testQemuAgentGetInterfaces(const void *data) +{ + virDomainXMLOptionPtr xmlopt = (virDomainXMLOptionPtr)data; + qemuMonitorTestPtr test = qemuMonitorTestNewAgent(xmlopt); + int ret = -1; + int ifaces_count = 0; + virDomainInterfacePtr *ifaces; + + if (!test) + return -1; + + if (qemuMonitorTestAddAgentSyncResponse(test) < 0) + goto cleanup; + + if (qemuMonitorTestAddItem(test, "guest-network-get-interfaces", + testQemuAgentGetInterfacesResponse) < 0) + goto cleanup; + + if ((ifaces_count = qemuAgentGetInterfaces(qemuMonitorTestGetAgent(test), + &ifaces)) < 0) { + goto cleanup; + }

No need for the {}. [1]

+ if (STRNEQ(ifaces[0]->name, "lo") || + STRNEQ(ifaces[0]->addrs[0].addr, "127.0.0.1") || + ifaces[0]->addrs[1].prefix != 128) {

Better to check "naddrs" of ifaces[i] before indexing them, assuming that ifaces[0] only has 1 "addrs" here. I'd like write the tests like:

+ virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + "unexpected return values for interface: lo"); + goto cleanup; + } + + if (STRNEQ(ifaces[1]->hwaddr, "52:54:00:89:ad:35") || + ifaces[1]->addrs[0].prefix != 24 || + ifaces[1]->addrs[1].type != VIR_IP_ADDR_TYPE_IPV6) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + "unexpected return values for interface: eth0"); + goto cleanup; + } + + if (ifaces[2]->naddrs != 2 || + ifaces[2]->addrs[0].type != VIR_IP_ADDR_TYPE_IPV4 || + STRNEQ(ifaces[2]->addrs[1].addr, "fe80::5054:ff:fed3:39ee")) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + "unexpected return values for interface: eth1"); + goto cleanup; + } + + if (ifaces_count != 3) { + virReportError(VIR_ERR_INTERNAL_ERROR, + "expected 3 interfaces, got %d", ret); + goto cleanup; + } +

Not too necessary, since it's the test, but still better to move this checking to [1], assuming that qemuAgentGetInterfaces returns 0, it will lead to crash. So I'd like write the tests like: if (ifaces_count != 3) { virReportError(...); goto cleanup; } if (ifaces[0]->naddrs != 2 || ifaces[1]->naddrs != 2 || ifaces[2]->naddrs !=2) { virReportError(...); goto cleanup; } if (STRNEQ(ifaces[0]->name, "lo") || STRNEQ(ifaces[0]->addrs[0].addr, "127.0.0.1") || ifaces[0]->addrs[1].prefix != 128) { virReportError(...); goto cleanup; } if (STRNEQ(ifaces[1]->hwaddr, "52:54:00:89:ad:35") || ifaces[1]->addrs[0].prefix != 24 || ifaces[1]->addrs[1].type != VIR_IP_ADDR_TYPE_IPV6) { virReportError(...); goto cleanup; } if (ifaces[2]->addrs[0].type != VIR_IP_ADDR_TYPE_IPV4 || STRNEQ(ifaces[2]->addrs[1].addr, "fe80::5054:ff:fed3:39ee")) { virReportError(...); goto cleanup; }

+ ret = 0; + +cleanup: + qemuMonitorTestFree(test); + size_t i;

Generally we do this in the beginning of function.

+ for (i = 0; i < ifaces_count; i++) + virDomainInterfaceFree(ifaces[i]); + VIR_FREE(ifaces); + return ret; +}

static int mymain(void) @@ -605,6 +717,7 @@ mymain(void) DO_TEST(Shutdown); DO_TEST(CPU); DO_TEST(ArbitraryCommand); + DO_TEST(GetInterfaces);

DO_TEST(Timeout); /* Timeout should always be called last */

On Sun, Aug 25, 2013 at 04:45:44AM +0530, Nehal J Wani wrote:

Use virDomainInterfacesAddresses in virsh

tools/virsh-domain-monitor.c * Introduce new command : domifaddr virsh # domifaddr f18 Name MAC address IPv4 address IPv6 address ------------------------------------------------------------------------------- lo 00:00:00:00:00:00 127.0.0.1/8 ::1/128 eth0 52:54:00:89:4e:97 192.168.101.130/24 fe80::5054:ff:fe89:4e97/64 eth0:1 52:54:00:89:4e:97 192.168.101.133/24 eth0:2 52:54:00:89:4e:97 192.168.101.132/24 eth1 52:54:00:89:ad:35 192.168.102.142/24 fe80::5054:ff:fe89:ad35/64 eth1:1 52:54:00:89:ad:35 192.168.102.143/24 eth2 52:54:00:d3:39:ee 192.168.103.183/24 fe80::5054:ff:fed3:39ee/64 eth2:0 52:54:00:d3:39:ee 192.168.103.184/24 eth2:1 52:54:00:d3:39:ee 192.168.103.185/24 eth3 52:54:00:fe:4c:4f 192.168.101.197/24 fe80::5054:ff:fefe:4c4f/64 eth3:1 52:54:00:fe:4c:4f 192.168.101.198/24

This formatting of IP addrs is broken. We should not expose interface aliases 'eth0:1', 'eth0:2', etc. If QEMU agent is returning such names, either we should fix the agent, or strip the ":1" suffixes in libvirt. The aliased names are an artifact of the legacy linux IP config tools. The new 'ip' command does not use these - it just shows 'eth0' with multiple IPv4 and multiple IPv6 addresses, which is also how libvirt/netcf report physical device names & config. Our display format must allow for NICs having arbitrarily many addresses of either type, so displaying IPv4/IPv6 side by side will not work. I think we need a display format like: virsh domifaddr f18 Name MAC address Protocol Address ------------------------------------------------------------------------------- lo 00:00:00:00:00:00 ipv4 127.0.0.1/8 - - ipv6 ::1/128 eth0 52:54:00:89:4e:97 ipv4 192.168.101.130/24 - - ipv4 192.168.101.133/24 - - ipv4 192.168.101.132/24 - - ipv6 fe80::5054:ff:fe89:4e97/64 eth1 52:54:00:89:ad:35 ipv4 192.168.102.142/24 - - ipv4 192.168.102.143/24 - - ipv6 fe80::5054:ff:fe89:ad35/64 With option to fully display all fields to make life easier for scripts: virsh domifaddr --full f18 Name MAC address Protocol Address ------------------------------------------------------------------------------- lo 00:00:00:00:00:00 ipv4 127.0.0.1/8 lo 00:00:00:00:00:00 ipv6 ::1/128 eth0 52:54:00:89:4e:97 ipv4 192.168.101.130/24 eth0 52:54:00:89:4e:97 ipv4 192.168.101.133/24 eth0 52:54:00:89:4e:97 ipv4 192.168.101.132/24 eth0 52:54:00:89:4e:97 ipv6 fe80::5054:ff:fe89:4e97/64 eth1 52:54:00:89:ad:35 ipv4 192.168.102.142/24 eth1 52:54:00:89:ad:35 ipv4 192.168.102.143/24 eth1 52:54:00:89:ad:35 ipv6 fe80::5054:ff:fe89:ad35/64

+ + for (i = 0; i < ifaces_count; i++) { + virDomainInterfacePtr iface = ifaces[i]; + virBuffer buf = VIR_BUFFER_INITIALIZER; + const char *hwaddr = ""; + const char *ip_addr_str = NULL; + + if (interface && STRNEQ(interface, iface->name)) { + virBufferFreeAndReset(&buf); + continue; + } + + if (iface->hwaddr) + hwaddr = iface->hwaddr; + + for (j = 0; j < iface->naddrs; j++) { + if (j) + virBufferAsprintf(&buf, "%25s/%d", + iface->addrs[j].addr, + iface->addrs[j].prefix); + else + virBufferAsprintf(&buf, "%s/%d", + iface->addrs[j].addr, + iface->addrs[j].prefix);

This logic is very broken not allowing for multiple addrs per device

+ } + + if (virBufferError(&buf)) { + virBufferFreeAndReset(&buf); + virReportOOMError(); + return ret; + } + + ip_addr_str = virBufferContentAndReset(&buf); + + if (!ip_addr_str) + ip_addr_str = ""; + + vshPrintExtra(ctl, " %-10s %-17s %s\n", + iface->name, hwaddr, ip_addr_str); + + virBufferFreeAndReset(&buf); + } + + ret = true; + +cleanup: + for (i = 0; i < ifaces_count; i++) + virDomainInterfaceFree(ifaces[i]); + VIR_FREE(ifaces); + + virDomainFree(dom); + return ret; +} + const vshCmdDef domMonitoringCmds[] = { {.name = "domblkerror", .handler = cmdDomBlkError, @@ -1944,5 +2039,11 @@ const vshCmdDef domMonitoringCmds[] = { .info = info_list, .flags = 0 }, + {.name = "domifaddr", + .handler = cmdDomIfAddr, + .opts = opts_domifaddr, + .info = info_domifaddr, + .flags = 0 + }, {.name = NULL} };

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 :|

On Tue, Aug 27, 2013 at 07:28:23PM +0530, Nehal J Wani wrote:

On Tue, Aug 27, 2013 at 6:20 PM, Daniel P. Berrange <berrange@redhat.com> wrote:

...

On Sun, Aug 25, 2013 at 04:45:44AM +0530, Nehal J Wani wrote:

...

Use virDomainInterfacesAddresses in virsh

tools/virsh-domain-monitor.c * Introduce new command : domifaddr virsh # domifaddr f18 Name MAC address IPv4 address IPv6 address

...

...

lo 00:00:00:00:00:00 127.0.0.1/8 ::1/128 eth0 52:54:00:89:4e:97 192.168.101.130/24 fe80::5054:ff:fe89:4e97/64 eth0:1 52:54:00:89:4e:97 192.168.101.133/24 eth0:2 52:54:00:89:4e:97 192.168.101.132/24 eth1 52:54:00:89:ad:35 192.168.102.142/24 fe80::5054:ff:fe89:ad35/64 eth1:1 52:54:00:89:ad:35 192.168.102.143/24 eth2 52:54:00:d3:39:ee 192.168.103.183/24 fe80::5054:ff:fed3:39ee/64 eth2:0 52:54:00:d3:39:ee 192.168.103.184/24 eth2:1 52:54:00:d3:39:ee 192.168.103.185/24 eth3 52:54:00:fe:4c:4f 192.168.101.197/24 fe80::5054:ff:fefe:4c4f/64 eth3:1 52:54:00:fe:4c:4f 192.168.101.198/24

This formatting of IP addrs is broken.

We should not expose interface aliases 'eth0:1', 'eth0:2', etc. If QEMU agent is returning such names, either we should fix the agent, or strip the ":1" suffixes in libvirt. The aliased names are an artifact of the legacy

...

config tools. The new 'ip' command does not use these - it just shows 'eth0' with multiple IPv4 and multiple IPv6 addresses, which is also how

------------------------------------------------------------------------------- linux IP libvirt/netcf

...

report physical device names & config.

Our display format must allow for NICs having arbitrarily many addresses of either type, so displaying IPv4/IPv6 side by side will not work.

I think we need a display format like:

virsh domifaddr f18 Name MAC address Protocol Address

-------------------------------------------------------------------------------

...

lo 00:00:00:00:00:00 ipv4 127.0.0.1/8 - - ipv6 ::1/128 eth0 52:54:00:89:4e:97 ipv4 192.168.101.130/24 - - ipv4 192.168.101.133/24 - - ipv4 192.168.101.132/24 - - ipv6 fe80::5054:ff:fe89:4e97/64 eth1 52:54:00:89:ad:35 ipv4 192.168.102.142/24 - - ipv4 192.168.102.143/24 - - ipv6 fe80::5054:ff:fe89:ad35/64

With option to fully display all fields to make life easier for scripts:

virsh domifaddr --full f18 Name MAC address Protocol Address

-------------------------------------------------------------------------------

...

lo 00:00:00:00:00:00 ipv4 127.0.0.1/8 lo 00:00:00:00:00:00 ipv6 ::1/128 eth0 52:54:00:89:4e:97 ipv4 192.168.101.130/24 eth0 52:54:00:89:4e:97 ipv4 192.168.101.133/24 eth0 52:54:00:89:4e:97 ipv4 192.168.101.132/24 eth0 52:54:00:89:4e:97 ipv6 fe80::5054:ff:fe89:4e97/64 eth1 52:54:00:89:ad:35 ipv4 192.168.102.142/24 eth1 52:54:00:89:ad:35 ipv4 192.168.102.143/24 eth1 52:54:00:89:ad:35 ipv6 fe80::5054:ff:fe89:ad35/64

...

+ + for (i = 0; i < ifaces_count; i++) { + virDomainInterfacePtr iface = ifaces[i]; + virBuffer buf = VIR_BUFFER_INITIALIZER; + const char *hwaddr = ""; + const char *ip_addr_str = NULL; + + if (interface && STRNEQ(interface, iface->name)) { + virBufferFreeAndReset(&buf); + continue; + } + + if (iface->hwaddr) + hwaddr = iface->hwaddr; + + for (j = 0; j < iface->naddrs; j++) { + if (j) + virBufferAsprintf(&buf, "%25s/%d", + iface->addrs[j].addr, + iface->addrs[j].prefix); + else + virBufferAsprintf(&buf, "%s/%d", + iface->addrs[j].addr, + iface->addrs[j].prefix);

This logic is very broken not allowing for multiple addrs per device

...

+ } + + if (virBufferError(&buf)) { + virBufferFreeAndReset(&buf); + virReportOOMError(); + return ret; + } + + ip_addr_str = virBufferContentAndReset(&buf); + + if (!ip_addr_str) + ip_addr_str = ""; + + vshPrintExtra(ctl, " %-10s %-17s %s\n", + iface->name, hwaddr, ip_addr_str); + + virBufferFreeAndReset(&buf); + } + + ret = true; + +cleanup: + for (i = 0; i < ifaces_count; i++) + virDomainInterfaceFree(ifaces[i]); + VIR_FREE(ifaces); + + virDomainFree(dom); + return ret; +} + const vshCmdDef domMonitoringCmds[] = { {.name = "domblkerror", .handler = cmdDomBlkError, @@ -1944,5 +2039,11 @@ const vshCmdDef domMonitoringCmds[] = { .info = info_list, .flags = 0 }, + {.name = "domifaddr", + .handler = cmdDomIfAddr, + .opts = opts_domifaddr, + .info = info_domifaddr, + .flags = 0 + }, {.name = NULL} };

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:|

-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list

Suppose I have the following network configuration in my guest: [root@localhost ~]# ifconfig eth0: flags=4163<UP,BROADCAST,RUNNING,MULTICAST> mtu 1500 inet 192.168.154.8 netmask 255.255.0.0 broadcast 192.168.255.255 inet6 fe80::5054:ff:fefe:4c4f prefixlen 64 scopeid 0x20<link> inet6 2001:db8:0:f101::2 prefixlen 64 scopeid 0x0<global> inet6 2001:db8:0:f101::1 prefixlen 64 scopeid 0x0<global> ether 52:54:00:fe:4c:4f txqueuelen 1000 (Ethernet) RX packets 1535 bytes 123240 (120.3 KiB) RX errors 0 dropped 0 overruns 0 frame 0 TX packets 1133 bytes 160636 (156.8 KiB) TX errors 0 dropped 0 overruns 0 carrier 0 collisions 0

eth0:0: flags=4163<UP,BROADCAST,RUNNING,MULTICAST> mtu 1500 inet 102.168.168.168 netmask 255.255.0.0 broadcast 192.168.255.255 ether 52:54:00:fe:4c:4f txqueuelen 1000 (Ethernet)

eth0:1: flags=4163<UP,BROADCAST,RUNNING,MULTICAST> mtu 1500 inet 192.168.101.197 netmask 255.255.255.0 broadcast 192.168.101.255 ether 52:54:00:fe:4c:4f txqueuelen 1000 (Ethernet)

lo: flags=73<UP,LOOPBACK,RUNNING> mtu 16436 inet 127.0.0.1 netmask 255.0.0.0 inet6 ::1 prefixlen 128 scopeid 0x10<host> loop txqueuelen 0 (Local Loopback) RX packets 8 bytes 616 (616.0 B) RX errors 0 dropped 0 overruns 0 frame 0 TX packets 8 bytes 616 (616.0 B) TX errors 0 dropped 0 overruns 0 carrier 0 collisions 0

[root@localhost ~]# ip addr 1: lo: <LOOPBACK,UP,LOWER_UP> mtu 16436 qdisc noqueue state UNKNOWN link/loopback 00:00:00:00:00:00 brd 00:00:00:00:00:00 inet 127.0.0.1/8 scope host lo inet6 ::1/128 scope host valid_lft forever preferred_lft forever 2: eth0: <BROADCAST,MULTICAST,UP,LOWER_UP> mtu 1500 qdisc pfifo_fast state UP qlen 1000 link/ether 52:54:00:fe:4c:4f brd ff:ff:ff:ff:ff:ff inet 192.168.154.8/16 brd 192.168.255.255 scope global eth0 inet 192.168.101.197/24 brd 192.168.101.255 scope global eth0:1 inet 102.168.168.168/16 brd 192.168.255.255 scope global eth0:0 inet6 2001:db8:0:f101::2/64 scope global valid_lft forever preferred_lft forever inet6 2001:db8:0:f101::1/64 scope global valid_lft forever preferred_lft forever inet6 fe80::5054:ff:fefe:4c4f/64 scope link valid_lft forever preferred_lft forever [root@localhost ~]#

Now, qemu-guest-agent returns back (after making it pretty): {

"return": [ { "name": "lo", "ip-addresses": [ { "ip-address-type": "ipv4", "ip-address": "127.0.0.1", "prefix": 8 }, { "ip-address-type": "ipv6", "ip-address": "::1", "prefix": 128 } ], "hardware-address": "00:00:00:00:00:00" }, { "name": "eth0", "ip-addresses": [ { "ip-address-type": "ipv4", "ip-address": "192.168.154.8", "prefix": 16 }, { "ip-address-type": "ipv6", "ip-address": "2001:db8:0:f101::2", "prefix": 64 }, { "ip-address-type": "ipv6", "ip-address": "2001:db8:0:f101::1", "prefix": 64 }, { "ip-address-type": "ipv6", "ip-address": "fe80::5054:ff:fefe:4c4f", "prefix": 64 } ], "hardware-address": "52:54:00:fe:4c:4f" }, { "name": "eth0:1", "ip-addresses": [ { "ip-address-type": "ipv4", "ip-address": "192.168.101.197", "prefix": 24 } ], "hardware-address": "52:54:00:fe:4c:4f" }, { "name": "eth0:0", "ip-addresses": [ { "ip-address-type": "ipv4", "ip-address": "102.168.168.168", "prefix": 16 } ], "hardware-address": "52:54:00:fe:4c:4f" } ] }

So, qemu-ga doesn't understand that there can't be more than one device with same MAC addr. So, I think we are left with the following options:

Actually that's wrong. You *can* have 2 completely different physical NICs with the same MAC address.

(i) Modify qemu-guest-agent to return addresses belonging to same MAC address grouped under one interface only.

You would not want to group based on MAC address - you explicitly just want to normalize by stripping the legacy aliases suffixes.

OR (ii) Let the reply be as it is now. Strip the ":0", ":1" from the response of guest agent (Is this really necessary?) . We'll have to parse the JSON multiple times and fill the virDomainInterface structs by grouping them according to the MAC addresses.

I think we need to do (ii) regardless to cope with existing deployed QEMU agent versions. We should also recommend to QEMU developers to fix the agent to not expose these legacy device alias names. 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 :|

On Sun, Aug 25, 2013 at 04:45:45AM +0530, Nehal J Wani wrote:

Expose virDomainInterfacesAddresses to python binding

examples/python/Makefile.am: * Add new file domipaddrs.py

examples/python/README: * Add documentation for the python example

python/libvirt-override-api.xml: * Add new symbol for virDomainInterfacesAddresses

python/libvirt-override.c: * Hand written python api

Example: $ ./run ./examples/python/domipaddrs.py qemu:///system f18 Interface MAC address IPv4 Address IPv6 Address eth3:1 52:54:00:fe:4c:4f 192.168.101.198/24 eth2:1 52:54:00:d3:39:ee 192.168.103.185/24 eth2:0 52:54:00:d3:39:ee 192.168.103.184/24 eth1:2 52:54:00:89:ad:35 192.168.102.143/24 lo 00:00:00:00:00:00 127.0.0.1/8 ::1/128 eth0:2 52:54:00:89:4e:97 192.168.101.132/24 eth0:1 52:54:00:89:4e:97 192.168.101.133/24 eth3 52:54:00:fe:4c:4f 192.168.101.197/24 fe80::5054:ff:fefe:4c4f/64 eth2 52:54:00:d3:39:ee 192.168.103.183/24 fe80::5054:ff:fed3:39ee/64 eth1 52:54:00:89:ad:35 192.168.102.142/24 fe80::5054:ff:fe89:ad35/64 eth0 52:54:00:89:4e:97 192.168.101.130/24 fe80::5054:ff:fe89:4e97/64

--- examples/python/Makefile.am | 2 +- examples/python/README | 1 + examples/python/domipaddrs.py | 49 +++++++++++++++++ python/libvirt-override-api.xml | 8 ++- python/libvirt-override.c | 113 ++++++++++++++++++++++++++++++++++++++++ 5 files changed, 171 insertions(+), 2 deletions(-) create mode 100755 examples/python/domipaddrs.py

diff --git a/examples/python/Makefile.am b/examples/python/Makefile.am index 2cacfa1..d33ee17 100644 --- a/examples/python/Makefile.am +++ b/examples/python/Makefile.am @@ -17,4 +17,4 @@ EXTRA_DIST= \ README \ consolecallback.py \ - dominfo.py domrestore.py domsave.py domstart.py esxlist.py + dominfo.py domrestore.py domsave.py domstart.py esxlist.py domipaddrs.py diff --git a/examples/python/README b/examples/python/README index f4db76c..1285d52 100644 --- a/examples/python/README +++ b/examples/python/README @@ -10,6 +10,7 @@ domsave.py - save all running domU's into a directory domrestore.py - restore domU's from their saved files in a directory esxlist.py - list active domains of an VMware ESX host and print some info. also demonstrates how to use the libvirt.openAuth() method +domipaddrs.py - print domain interfaces along with their MAC and IP addresses

The XML files in this directory are examples of the XML format that libvirt expects, and will have to be adapted for your setup. They are only needed diff --git a/examples/python/domipaddrs.py b/examples/python/domipaddrs.py new file mode 100755 index 0000000..e71e8f6 --- /dev/null +++ b/examples/python/domipaddrs.py @@ -0,0 +1,49 @@ +#!/usr/bin/env python +# domipaddrds - print domain interfaces along with their MAC and IP addresses + +import libvirt +import sys + +def usage(): + print "Usage: %s [URI] DOMAIN" % sys.argv[0] + print " Print domain interfaces along with their MAC and IP addresses" + +uri = None +name = None +args = len(sys.argv) + +if args == 2: + name = sys.argv[1] +elif args == 3: + uri = sys.argv[1] + name = sys.argv[2] +else: + usage() + sys.exit(2) + +conn = libvirt.openReadOnly(uri) +if conn == None: + print "Unable to open connection to libvirt" + sys.exit(1) + +try: + dom = conn.lookupByName(name) +except libvirt.libvirtError: + print "Domain %s not found" % name + sys.exit(0) + +ifaces = dom.interfacesAddresses(0) +if (ifaces == None): + print "Failed to get domain interfaces" + sys.exit(0) + +print " {0:10} {1:20} {2:15} {3}".format("Interface", "MAC address", "IPv4 Address", "IPv6 Address") + +for (name, val) in ifaces.iteritems(): + print " {0:10} {1:17}".format(name, val['hwaddr']), + + if (val['ip_addrs'] <> None): + print " ", + for addr in val['ip_addrs']: + print "{0}/{1} ".format(addr['addr'], addr['prefix']), + print diff --git a/python/libvirt-override-api.xml b/python/libvirt-override-api.xml index 9a88215..f5c6e83 100644 --- a/python/libvirt-override-api.xml +++ b/python/libvirt-override-api.xml @@ -602,5 +602,11 @@ <arg name='conn' type='virConnectPtr' info='pointer to the hypervisor connection'/> <arg name='flags' type='int' info='unused, pass 0'/> </function> - </symbols> + <function name='virDomainInterfacesAddresses' file='python'> + <info>returns a dictionary of domain interfaces along with their MAC and IP addresses</info> + <arg name='dom' type='virDomainPtr' info='pointer to the domain'/> + <arg name='flags' type='unsigned int' info='extra flags; not used yet, so callers should always pass 0'/> + <return type='virDomainInterfacePtr' info="dictionary of domain interfaces along with their MAC and IP addresses"/> + </function> +</symbols> </api> diff --git a/python/libvirt-override.c b/python/libvirt-override.c index d16b9a2..9eae9fb 100644 --- a/python/libvirt-override.c +++ b/python/libvirt-override.c @@ -4761,6 +4761,118 @@ cleanup: return py_retval; }

+ +static PyObject * +libvirt_virDomainInterfacesAddresses(PyObject *self ATTRIBUTE_UNUSED, + PyObject *args) +{ + PyObject *py_retval = VIR_PY_NONE; + virDomainPtr domain; + PyObject *pyobj_domain; + unsigned int flags; + virDomainInterfacePtr *ifaces = NULL; + int ifaces_count = 0; + size_t i, j; + int ret; + bool full_free = true; + + if (!PyArg_ParseTuple(args, (char *) "Oi:virDomainInterfacePtr", + &pyobj_domain, &flags)) + return NULL; + + domain = (virDomainPtr) PyvirDomain_Get(pyobj_domain); + + LIBVIRT_BEGIN_ALLOW_THREADS; + ifaces_count = virDomainInterfacesAddresses(domain, &ifaces, flags); + ret = ifaces_count; + LIBVIRT_END_ALLOW_THREADS; + if (ret < 0) + goto cleanup; + + if (!(py_retval = PyDict_New())) + goto no_memory; + + for (i = 0; i < ifaces_count; i++) { + virDomainInterfacePtr iface = ifaces[i]; + PyObject *py_ip_addrs = NULL; + PyObject *py_iface = NULL; + + if (!(py_iface = PyDict_New())) + goto no_memory; + + if (iface->naddrs) { + if (!(py_ip_addrs = PyList_New(iface->naddrs))) { + Py_DECREF(py_iface); + goto no_memory; + } + } else { + py_ip_addrs = VIR_PY_NONE; + } + + for (j = 0; j < iface->naddrs; j++) { + virDomainIPAddressPtr ip_addr = &(iface->addrs[j]); + PyObject *py_addr = PyDict_New(); + const char *type; + + if (!py_addr) { + Py_DECREF(py_iface); + Py_DECREF(py_ip_addrs); + goto no_memory; + } + + switch (ip_addr->type) { + case VIR_IP_ADDR_TYPE_IPV4: + type = "ipv4";

I'd prefer to have libvirt.VIR_IP_ADDR_TYPE_IPV4 instead of a text representation here but that's probably a matter of taste.

+ break; + case VIR_IP_ADDR_TYPE_IPV6: + type = "ipv6"; + break

Same here.

+ default: + type = "unknown"; + break;

Same here.

+ } + + PyDict_SetItem(py_addr, libvirt_constcharPtrWrap("addr"), + PyString_FromString(ip_addr->addr)); + PyDict_SetItem(py_addr, libvirt_constcharPtrWrap("prefix"), + libvirt_intWrap(ip_addr->prefix)); + PyDict_SetItem(py_addr, libvirt_constcharPtrWrap("type"), + PyString_FromString(type)); + + PyList_SetItem(py_ip_addrs, j, py_addr); + } + + PyDict_SetItem(py_iface, libvirt_constcharPtrWrap("ip_addrs"), + py_ip_addrs); + PyDict_SetItem(py_iface, libvirt_constcharPtrWrap("hwaddr"), + libvirt_charPtrWrap(iface->hwaddr)); + + PyDict_SetItem(py_retval, libvirt_charPtrWrap(iface->name), + py_iface); + } + + full_free = false; + +cleanup: + for (i = 0; i < ifaces_count; i++) { + /* + * We don't want to free values we've just shared with python variables unless + * there was an error and hence we are returning PY_NONE or equivalent + */ + if (full_free) + virDomainInterfaceFree(ifaces[i]);

Wouldn't it be nicer to move the conditional outside the loop or even: for (i = 0; full_free && i < ifaces_count; i++) { Cheers, -- Guido

+ } + VIR_FREE(ifaces); + + return py_retval; + +no_memory: + Py_XDECREF(py_retval); + py_retval = PyErr_NoMemory(); + goto cleanup; +} + + /******************************************* * Helper functions to avoid importing modules * for every callback @@ -7284,6 +7396,7 @@ static PyMethodDef libvirtMethods[] = { {(char *) "virDomainBlockPeek", libvirt_virDomainBlockPeek, METH_VARARGS, NULL}, {(char *) "virDomainMemoryPeek", libvirt_virDomainMemoryPeek, METH_VARARGS, NULL}, {(char *) "virDomainGetDiskErrors", libvirt_virDomainGetDiskErrors, METH_VARARGS, NULL}, + {(char *) "virDomainInterfacesAddresses", libvirt_virDomainInterfacesAddresses, METH_VARARGS, NULL}, {(char *) "virNodeGetMemoryParameters", libvirt_virNodeGetMemoryParameters, METH_VARARGS, NULL}, {(char *) "virNodeSetMemoryParameters", libvirt_virNodeSetMemoryParameters, METH_VARARGS, NULL}, {(char *) "virNodeGetCPUMap", libvirt_virNodeGetCPUMap, METH_VARARGS, NULL}, -- 1.7.11.7

-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list