On Sun, Sep 1, 2013 at 7:13 PM, Nehal J Wani <nehaljw.kkd1@gmail.com> wrote:

Define a new API virDomainInterfaceAddresses, which returns the address information of a running domain's interfaces(s). If no interface name is specified, it returns the information of all interfaces, otherwise it only returns the information of the specificed interface. The address information includes the MAC and IP addresses.

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

But at this stage, it will only work with guest agent, and flags won't be supported.

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 | 32 ++++++++++++ python/generator.py | 3 ++ src/driver.h | 6 +++ src/libvirt.c | 115 +++++++++++++++++++++++++++++++++++++++++++ src/libvirt_public.syms | 6 +++ 5 files changed, 162 insertions(+)

diff --git a/include/libvirt/libvirt.h.in b/include/libvirt/libvirt.h.in index a47e33c..1a34d02 100644 --- a/include/libvirt/libvirt.h.in +++ b/include/libvirt/libvirt.h.in @@ -2044,6 +2044,38 @@ 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 */ + 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..eb4927b 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..82c117f 100644 --- a/src/libvirt.c +++ b/src/libvirt.c @@ -8643,6 +8643,96 @@ error: return -1; }

+ /** + * virDomainInterfaceAddresses: + * @dom: domain object + * @ifaces: pointer to an array of pointers pointing to interface objects + * @flags: extra flags; not used yet, so callers should always pass 0 + * + * 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. + * + * 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 = 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]); + * VIR_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 (conn->driver->domainInterfaceAddresses) { + int ret; + ret = conn->driver->domainInterfaceAddresses(dom, ifaces, flags); + if (ret < 0) + goto error; + return ret; + } + + virLibConnError(VIR_ERR_NO_SUPPORT, __FUNCTION__); + +error: + virDispatchError(dom->conn); + return -1; +} + /** * virDomainMemoryStats: * @dom: pointer to the domain object @@ -21961,3 +22051,28 @@ error: virDispatchError(dom->conn); return -1; } + +/** + * virDomainInterfaceFree: + * @iface: an interface object + * + * Free the interface object. The data structure is + * freed and should not be used thereafter. + */ +void +virDomainInterfaceFree(virDomainInterfacePtr iface) +{ + size_t i; + + if (!iface) + return; + + 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); +} diff --git a/src/libvirt_public.syms b/src/libvirt_public.syms index bbdf78a..35f3a3e 100644 --- a/src/libvirt_public.syms +++ b/src/libvirt_public.syms @@ -634,4 +634,10 @@ LIBVIRT_1.1.1 { virDomainSetMemoryStatsPeriod; } LIBVIRT_1.1.0;

+LIBVIRT_1.1.3 { + global: + virDomainInterfaceAddresses; + virDomainInterfaceFree; +} LIBVIRT_1.1.1; + # .... define new API here using predicted next version number .... -- 1.7.11.7

In agreement with Guido's suggestion on 5/5 of the previous version (https://www.redhat.com/archives/libvir-list/2013-August/msg01271.html), the enums are also to exposed to python bindings, for which, /include/libvirt/libvirt.h.in will have to be modified a bit. Diff has been attached. -- Nehal J Wani

On 09/01/2013 07:43 AM, Nehal J Wani wrote:

Define a new API virDomainInterfaceAddresses, which returns the address information of a running domain's interfaces(s). If no interface name is specified, it returns the information of all interfaces, otherwise it only returns the information of the specificed interface. The address information includes the MAC and IP addresses.

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

But at this stage, it will only work with guest agent, and flags won't be supported.

That worries me a bit. Ultimately, we want our interfaces to behave as sane as possible when flags==0; rather than making the behavior be that 'flags==0' implies 'only guest agent probe', I'd rather introduce a flag right away up front that says 'include guest agent probe in the set of attempted methods', and then document that 'flags==0 is shorthand for letting the hypervisor choose the best method(s) out of supported possibilities)'. In other words, I want to make sure that this API will be similar to virDomainShutdownFlags, where a flags of 0 lets the hypervisor choose between methods, a single explicit flag forces the hypervisor to use that method alone, and more than one flag can be OR'd together to let the hypervisor choose among that subset of flags.

+ char *addr; /* IP address */ + unsigned int prefix; /* IP address prefix */

Do we ever intend to support non-CIDR IPv4 addresses (where the mask is not contiguous bits)? Or are we intentionally documenting that the prefix will always be possible because we always require the mask to be a contiguous prefix?

@@ -1238,6 +1243,7 @@ struct _virDriver { virDrvDomainInterfaceStats domainInterfaceStats; virDrvDomainSetInterfaceParameters domainSetInterfaceParameters; virDrvDomainGetInterfaceParameters domainGetInterfaceParameters; + virDrvDomainInterfaceAddresses domainInterfaceAddresses;

Spacing is off; only one space needed.

+ * + * cleanup: + * if (ifaces) + * for (i = 0; i < ifaces_count; i++) + * virDomainInterfaceFree(ifaces[i]); + * VIR_FREE(ifaces);

VIR_FREE() is not a public function; this comment should instead mention free(); because it is in a comment, it will not trigger our syntax check rules.

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

Putting on my security hat - anything that requires guest interaction should probably not be permitted from a read-only connection (because a malicious guest coupled with a read-only caller purposefully exploiting the guest's intentional bad behavior might open up a denial of service against read-write clients). Again, all the more reason why I think you should require non-zero flags before permitting guest agent interaction, so that we can then add a security check here (if you pass flags=0, you get the default set of actions that are safe for a read-only client; but if you pass flags=..._AGENT, then we can prevent the call from succeeding on a read-only client). Overall, I'm happy with what we finally settled on for the API, and my questions now only involve whether we need a non-zero flag before allowing agent interaction. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org

On 04/09/13 03:07, Eric Blake wrote:

...

Define a new API virDomainInterfaceAddresses, which returns the address information of a running domain's interfaces(s). If no interface name is specified, it returns the information of all interfaces, otherwise it only returns the information of the specificed interface. The address information includes the MAC and IP addresses.

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

But at this stage, it will only work with guest agent, and flags won't be supported. That worries me a bit. Ultimately, we want our interfaces to behave as sane as possible when flags==0; rather than making the behavior be that 'flags==0' implies 'only guest agent probe', I'd rather introduce a flag right away up front that says 'include guest agent probe in the set of attempted methods', and then document that 'flags==0 is shorthand for letting the hypervisor choose the best method(s) out of supported

On 09/01/2013 07:43 AM, Nehal J Wani wrote: possibilities)'. In other words, I want to make sure that this API will be similar to virDomainShutdownFlags, where a flags of 0 lets the hypervisor choose between methods, a single explicit flag forces the hypervisor to use that method alone, and more than one flag can be OR'd together to let the hypervisor choose among that subset of flags.

I can't find any reason to say this is not good. Except that I'm wondering which the upper layer developer prefer more, the more straightforward style we used for the flags of old APIs (flags==0 defaults to a specific flag), or the style like this (flags==0 defaults to letting the hypervisor choosing it, and even choosing between subset of the OR'ed flags)?

...

+ char *addr; /* IP address */ + unsigned int prefix; /* IP address prefix */ Do we ever intend to support non-CIDR IPv4 addresses (where the mask is not contiguous bits)? Or are we intentionally documenting that the prefix will always be possible because we always require the mask to be a contiguous prefix?

We don't have the way to require it, it's returned either from guest agent, leases file, and traffic snooping, unless we reject if they returns non-contiguous bits for the subnet mask, but that doesn't make sense. So I'm wondering if we should introduce a new member for the netmask, though qemu guest agent doesn't return anything for the netmask (I guess it always assumes the netmask bits are contiguous), for the other 2 methods, we still might faces the non-contiguous netmask bits.

...

@@ -1238,6 +1243,7 @@ struct _virDriver { virDrvDomainInterfaceStats domainInterfaceStats; virDrvDomainSetInterfaceParameters domainSetInterfaceParameters; virDrvDomainGetInterfaceParameters domainGetInterfaceParameters; + virDrvDomainInterfaceAddresses domainInterfaceAddresses; Spacing is off; only one space needed.

...

+ * + * cleanup: + * if (ifaces) + * for (i = 0; i < ifaces_count; i++) + * virDomainInterfaceFree(ifaces[i]); + * VIR_FREE(ifaces); VIR_FREE() is not a public function; this comment should instead mention free(); because it is in a comment, it will not trigger our syntax check rules.

...

+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; Putting on my security hat - anything that requires guest interaction should probably not be permitted from a read-only connection (because a malicious guest coupled with a read-only caller purposefully exploiting the guest's intentional bad behavior might open up a denial of service against read-write clients). Again, all the more reason why I think you should require non-zero flags before permitting guest agent interaction, so that we can then add a security check here (if you pass flags=0, you get the default set of actions that are safe for a read-only client; but if you pass flags=..._AGENT, then we can prevent the call from succeeding on a read-only client).

Overall, I'm happy with what we finally settled on for the API, and my questions now only involve whether we need a non-zero flag before allowing agent interaction.

On Sun, Sep 01, 2013 at 07:13:32PM +0530, Nehal J Wani wrote:

daemon/remote.c * Define remoteSerializeDomainInterface, remoteDispatchDomainInterfaceAddresses

src/remote/remote_driver.c * Define remoteDomainInterfaceAddresses

src/remote/remote_protocol.x * New RPC procedure: REMOTE_PROC_DOMAIN_INTERFACE_ADDRESSES * Define structs remote_domain_ip_addr, remote_domain_interface, remote_domain_interfaces_addresse_args, remote_domain_interface_addresses_ret * Introduce upper bounds (to handle DDoS attacks): REMOTE_DOMAIN_INTERFACE_MAX = 2048 REMOTE_DOMAIN_IP_ADDR_MAX = 2048 Restrictions on the maximum number of aliases per interface were removed after kernel v2.0, and theoretically, at present, there are no upper limits on number of interfaces per virtual machine and on the number of IP addresses per interface.

src/remote_protocol-structs * New structs added

--- daemon/remote.c | 131 +++++++++++++++++++++++++++++++++++++++++++ src/remote/remote_driver.c | 99 ++++++++++++++++++++++++++++++++ src/remote/remote_protocol.x | 40 ++++++++++++- src/remote_protocol-structs | 24 ++++++++ 4 files changed, 293 insertions(+), 1 deletion(-)

diff --git a/daemon/remote.c b/daemon/remote.c index 6ace7af..7091cab 100644 --- a/daemon/remote.c +++ b/daemon/remote.c @@ -5144,7 +5144,138 @@ cleanup:

+ +static int +remoteDispatchDomainInterfaceAddresses( + virNetServerPtr server ATTRIBUTE_UNUSED, + virNetServerClientPtr client, + virNetMessagePtr msg ATTRIBUTE_UNUSED, + virNetMessageErrorPtr rerr, + remote_domain_interface_addresses_args *args, + remote_domain_interface_addresses_ret *ret)

Normal practice for this file is to layout args thus: static int remoteDispatchDomainInterfaceAddresses(virNetServerPtr server ATTRIBUTE_UNUSED, virNetServerClientPtr client, virNetMessagePtr msg ATTRIBUTE_UNUSED, virNetMessageErrorPtr rerr, remote_domain_interface_addresses_args *args, remote_domain_interface_addresses_ret *ret) ACK if the style issue is fixed 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 03/09/13 15:00, Nehal J Wani wrote:

On Mon, Sep 2, 2013 at 5:11 PM, Daniel P. Berrange <berrange@redhat.com> wrote:

...

On Sun, Sep 01, 2013 at 07:13:32PM +0530, Nehal J Wani wrote:

...

daemon/remote.c * Define remoteSerializeDomainInterface, remoteDispatchDomainInterfaceAddresses

src/remote/remote_driver.c * Define remoteDomainInterfaceAddresses

src/remote/remote_protocol.x * New RPC procedure: REMOTE_PROC_DOMAIN_INTERFACE_ADDRESSES * Define structs remote_domain_ip_addr, remote_domain_interface, remote_domain_interfaces_addresse_args, remote_domain_interface_addresses_ret * Introduce upper bounds (to handle DDoS attacks): REMOTE_DOMAIN_INTERFACE_MAX = 2048 REMOTE_DOMAIN_IP_ADDR_MAX = 2048 Restrictions on the maximum number of aliases per interface were removed after kernel v2.0, and theoretically, at present, there are no upper limits on number of interfaces per virtual machine and on the number of IP addresses per interface.

src/remote_protocol-structs * New structs added

--- daemon/remote.c | 131 +++++++++++++++++++++++++++++++++++++++++++ src/remote/remote_driver.c | 99 ++++++++++++++++++++++++++++++++ src/remote/remote_protocol.x | 40 ++++++++++++- src/remote_protocol-structs | 24 ++++++++ 4 files changed, 293 insertions(+), 1 deletion(-)

diff --git a/daemon/remote.c b/daemon/remote.c index 6ace7af..7091cab 100644 --- a/daemon/remote.c +++ b/daemon/remote.c @@ -5144,7 +5144,138 @@ cleanup: + +static int +remoteDispatchDomainInterfaceAddresses( + virNetServerPtr server ATTRIBUTE_UNUSED, + virNetServerClientPtr client, + virNetMessagePtr msg ATTRIBUTE_UNUSED, + virNetMessageErrorPtr rerr, + remote_domain_interface_addresses_args *args, + remote_domain_interface_addresses_ret *ret) Normal practice for this file is to layout args thus:

static int remoteDispatchDomainInterfaceAddresses(virNetServerPtr server ATTRIBUTE_UNUSED, virNetServerClientPtr client, virNetMessagePtr msg ATTRIBUTE_UNUSED, virNetMessageErrorPtr rerr, remote_domain_interface_addresses_args *args, remote_domain_interface_addresses_ret *ret)

ACK if the style issue is fixed

Style issue fix (File attached):

diff --git a/daemon/remote.c b/daemon/remote.c index 7091cab..d46e3ea 100644 --- a/daemon/remote.c +++ b/daemon/remote.c @@ -5230,13 +5230,12 @@ cleanup: }

static int -remoteDispatchDomainInterfaceAddresses( - virNetServerPtr server ATTRIBUTE_UNUSED, - virNetServerClientPtr client, - virNetMessagePtr msg ATTRIBUTE_UNUSED, - virNetMessageErrorPtr rerr, - remote_domain_interface_addresses_args *args, - remote_domain_interface_addresses_ret *ret) +remoteDispatchDomainInterfaceAddresses(virNetServerPtr server ATTRIBUTE_UNUSED, + virNetServerClientPtr client, + virNetMessagePtr msg ATTRIBUTE_UNUSED, + virNetMessageErrorPtr rerr, + remote_domain_interface_addresses_args *args, + remote_domain_interface_addresses_ret *ret) { size_t i; int rv = -1;

PS: IMO, other functions like remoteDispatchDomainCreateWithFiles, remoteDispatchDomainCreateXMLWithFiles, remoteDispatchDomainMigrateFinish3Params, etc also need same style change

I don't see problem with the diff applied. Patch is welcomed for the other style problems. Osier

On Sun, Sep 01, 2013 at 07:13:33PM +0530, Nehal J Wani wrote:

+int +qemuAgentGetInterfaces(qemuAgentPtr mon, + virDomainInterfacePtr **ifaces) +{ + + /* 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; + } + + /* Handle aliases of type <ifname>:<alias-name> */ + ifname = virStringSplit(name, ":", 2); + ifname_s = ifname[0]; + + iface = virHashLookup(ifaces_store, ifname_s); + + /* If the storage bag doesn't contain this iface, add it */ + if (!iface) { + if (VIR_EXPAND_N(ifaces_ret, ifaces_count, 1) < 0) + goto cleanup; + + if (VIR_ALLOC(ifaces_ret[ifaces_count - 1]) < 0) + goto cleanup; + + if (virHashAddEntry(ifaces_store, ifname_s, + ifaces_ret[ifaces_count - 1]) < 0) + goto cleanup; + + iface = ifaces_ret[ifaces_count - 1]; + iface->naddrs = 0; + + if (VIR_STRDUP(iface->name, ifname_s) < 0) + goto error; + + /* hwaddr might be omitted */

Really ? Can qemu guest agent report interfacs with 'hardware-address' field not being present ? I'd expect that field to be mandatory and so report an error in libvirt if missing.

+ hwaddr = virJSONValueObjectGetString(tmp_iface, "hardware-address"); + if (hwaddr && VIR_STRDUP(iface->hwaddr, hwaddr) < 0) + goto error; + } + + /* Has to be freed for each interface. */ + virStringFreeList(ifname);

You're leaking 'ifname' if any of the 'goto xxxx' branches are taken above.

+ + /* 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;

The '< 0' condition indicates an error scenario, so shouldn't be silently ignored. '== 0' is the empty list scenario that is ok to ignore, but already handled by the for() loop conditions.

+ + /* If current iface already exists, continue with the count */ + addrs_count = iface->naddrs; + + for (j = 0; j < ip_addr_arr_size; j++) { +

diff --git a/tests/qemuagenttest.c b/tests/qemuagenttest.c index 4e27981..4014a09 100644 --- a/tests/qemuagenttest.c +++ b/tests/qemuagenttest.c @@ -576,6 +576,154 @@ 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" + " }," + " {\"ip-address-type\":\"ipv4\"," + " \"ip-address\":\"192.168.234.152\"," + " \"prefix\":16" + " }," + " {\"ip-address-type\":\"ipv6\"," + " \"ip-address\":\"fe80::5054:ff:fec3:68bb\"," + " \"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\"" + " }," + " {\"name\":\"eth1:0\"," + " \"ip-addresses\":" + " [" + " {\"ip-address-type\":\"ipv4\"," + " \"ip-address\":\"192.168.10.91\"," + " \"prefix\":24" + " }," + " {\"ip-address-type\":\"ipv6\"," + " \"ip-address\":\"fe80::fc54:ff:fefe:4c4f\"," + " \"prefix\":64" + " }" + " ]," + " \"hardware-address\":\"52:54:00:d3:39:ee\"" + " }," + " {\"name\":\"eth2\"," + " \"hardware-address\":\"52:54:00:36:2a:e5\"" + " }" + " ]" + "}";

I'd re-arrange these a little, eg so that 'eth2' comes before 'eth1:1', just so that we validate that we're coping with things in a non-obvious sort order.

+ +static int +testQemuAgentGetInterfaces(const void *data) +{ + virDomainXMLOptionPtr xmlopt = (virDomainXMLOptionPtr)data; + qemuMonitorTestPtr test = qemuMonitorTestNewAgent(xmlopt); + size_t i; + int ret = -1; + int ifaces_count = 0; + virDomainInterfacePtr *ifaces = NULL; + + 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; + + if (ifaces_count != 4) { + virReportError(VIR_ERR_INTERNAL_ERROR, + "expected 4 interfaces, got %d", ret); + goto cleanup; + } + + if (ifaces[0]->naddrs != 2 || + ifaces[1]->naddrs != 4 || + ifaces[2]->naddrs != 4 || + ifaces[3]->naddrs != 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + "unexpected return value for number of IP addresses"); + 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(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 (STRNEQ(ifaces[2]->name, "eth1") || + 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; + }

You're only validating a couple of the IP addresses here & MAC addrs and names. I'd like to see all of them validated for all NIC. This would give us higher confidence that we're correctly merging eth1 and eth1:0 together. 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 :|

Except what Daniel pointed out: On 01/09/13 21:43, 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.

Although "ifconfig" is deprecated, IP aliases created by "ifconfig" are supported by this API. The legacy syntax of an IP alias is: "<ifname>:<alias-name>". Since we want all aliases to be clubbed under parent interface, simply stripping ":<alias-name>" suffices.

s/suffices/suffixes/,

Note that IP aliases formed by "ip" aren't visible to "ifconfig", and aliases created by "ip" do not have any specific name. But we are lucky, as qemuga detects aliases created by both.

s/qemuga/qemu guest agent/, or s/qemuga/qemu-ga/,

src/qemu/qemu_agent.h: * Define qemuAgentGetInterfaces

src/qemu/qemu_agent.c: * Implement qemuAgentGetInterface

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

src/remote_protocol-sructs: * Define new structs

tests/qemuagenttest.c: * Add new test: testQemuAgentGetInterfaces Test cases for IP aliases, 0 or multiple ipv4/ipv6 address(es)

--- src/qemu/qemu_agent.c | 193 +++++++++++++++++++++++++++++++++++++++++++++++++ src/qemu/qemu_agent.h | 3 + src/qemu/qemu_driver.c | 55 ++++++++++++++ tests/qemuagenttest.c | 149 ++++++++++++++++++++++++++++++++++++++ 4 files changed, 400 insertions(+)

diff --git a/src/qemu/qemu_agent.c b/src/qemu/qemu_agent.c index 2cd0ccc..009ed77 100644 --- a/src/qemu/qemu_agent.c +++ b/src/qemu/qemu_agent.c @@ -1320,6 +1320,199 @@ cleanup: }

/* + * qemuAgentGetInterfaces: + * @mon: Agent

s/Agent/Agent monitor/,

+ * @ifaces: pointer to an array of pointers pointing to interface objects + * + * Issue guest-network-get-interfaces to guest agent, which returns the

s/the/a/,

+ * list of a interfaces of a running domain along with their IP and MAC

s/of a/of/,

+ * addresses. + * + * Returns: number of interfaces on success, -1 on error. + */ +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; + size_t ifaces_count = 0; + size_t addrs_count = 0; + virDomainInterfacePtr *ifaces_ret = NULL; + virHashTablePtr ifaces_store = NULL; + + /* Initially the bag of ifaces is empty */

"bag" is magic here, how about: /* Hash table to handle the interface alias */

+ if (!(ifaces_store = virHashCreate(ifaces_count, NULL))) + return -1; + + if (!(cmd = qemuAgentMakeCommand("guest-network-get-interfaces", NULL))) + return -1;

You should free the created hash table.

+ + 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; + } + + for (i = 0; i < size; i++) { + virJSONValuePtr tmp_iface = virJSONValueArrayGet(ret_array, i); + virJSONValuePtr ip_addr_arr = NULL; + const char *hwaddr, *ifname_s, *name = NULL; + char **ifname = NULL; + int ip_addr_arr_size; + virDomainInterfacePtr iface = NULL; + + /* 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; + } + + /* Handle aliases of type <ifname>:<alias-name> */

I think no need to mention the "type" here, since it only can be "ifname:alias". So how about: /* Handle interface alias (<ifname>:<alias>

+ ifname = virStringSplit(name, ":", 2); + ifname_s = ifname[0]; + + iface = virHashLookup(ifaces_store, ifname_s); + + /* If the storage bag doesn't contain this iface, add it */

s/storage bag/hash table/,

+ if (!iface) { + if (VIR_EXPAND_N(ifaces_ret, ifaces_count, 1) < 0) + goto cleanup;

goto error;

+ + if (VIR_ALLOC(ifaces_ret[ifaces_count - 1]) < 0) + goto cleanup;

goto error;

+ + if (virHashAddEntry(ifaces_store, ifname_s, + ifaces_ret[ifaces_count - 1]) < 0)

Do we really need to use the virDomainInterfacePtr object as hash value? isn't a reference count is enough? if you use the reference count as the hash value, the logic can be more compact and clear: if (!iface) { VIR_EXPAND_N VIR_ALLOC virHashAddEntry ..... } iface = iface_ret[ifaces_count - 1];

+ goto cleanup;

goto error;

+ + iface = ifaces_ret[ifaces_count - 1]; + iface->naddrs = 0; + + if (VIR_STRDUP(iface->name, ifname_s) < 0) + goto error; + + /* hwaddr might be omitted */ + hwaddr = virJSONValueObjectGetString(tmp_iface, "hardware-address"); + if (hwaddr && VIR_STRDUP(iface->hwaddr, hwaddr) < 0) + goto error; + } + + /* Has to be freed for each interface. */ + virStringFreeList(ifname); + + /* 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; + + /* If current iface already exists, continue with the count */ + addrs_count = iface->naddrs; + + for (j = 0; j < ip_addr_arr_size; j++) { +

Meaningless blank line.

+ if (VIR_EXPAND_N(iface->addrs, addrs_count, 1) < 0) + goto error; + + virJSONValuePtr ip_addr_obj = virJSONValueArrayGet(ip_addr_arr, j); + virDomainIPAddressPtr ip_addr = &iface->addrs[addrs_count - 1]; + 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 (virJSONValueObjectGetNumberUint(ip_addr_obj, "prefix", + &ip_addr->prefix) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("malformed 'prefix' field")); + goto error; + } + } + + iface->naddrs = addrs_count; + } + + *ifaces = ifaces_ret; + ifaces_ret = NULL; + ret = ifaces_count; + +cleanup: + virJSONValueFree(cmd); + virJSONValueFree(reply); + virHashFree(ifaces_store); + 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 ed29373..908f19c 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -15870,6 +15870,60 @@ qemuNodeSuspendForDuration(virConnectPtr conn, return nodeSuspendForDuration(target, duration, flags); }

+static int +qemuDomainInterfaceAddresses(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 (virDomainInterfaceAddressesEnsureACL(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, @@ -15965,6 +16019,7 @@ static virDriver qemuDriver = { .domainMemoryPeek = qemuDomainMemoryPeek, /* 0.4.4 */ .domainGetBlockInfo = qemuDomainGetBlockInfo, /* 0.8.1 */ .nodeGetCPUStats = qemuNodeGetCPUStats, /* 0.9.3 */ + .domainInterfaceAddresses = qemuDomainInterfaceAddresses, /* 1.1.3 */ .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..4014a09 100644 --- a/tests/qemuagenttest.c +++ b/tests/qemuagenttest.c @@ -576,6 +576,154 @@ 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" + " }," + " {\"ip-address-type\":\"ipv4\"," + " \"ip-address\":\"192.168.234.152\"," + " \"prefix\":16" + " }," + " {\"ip-address-type\":\"ipv6\"," + " \"ip-address\":\"fe80::5054:ff:fec3:68bb\"," + " \"prefix\":64"

Can qemu guest agent really returns mutiple same type IP addresses? Isn't it returning the other IP addresses as "iface:alias" ?

+ " }" + " ]," + " \"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\"" + " }," + " {\"name\":\"eth1:0\"," + " \"ip-addresses\":" + " [" + " {\"ip-address-type\":\"ipv4\"," + " \"ip-address\":\"192.168.10.91\"," + " \"prefix\":24" + " }," + " {\"ip-address-type\":\"ipv6\"," + " \"ip-address\":\"fe80::fc54:ff:fefe:4c4f\"," + " \"prefix\":64" + " }" + " ]," + " \"hardware-address\":\"52:54:00:d3:39:ee\"" + " }," + " {\"name\":\"eth2\"," + " \"hardware-address\":\"52:54:00:36:2a:e5\"" + " }" + " ]" + "}"; + +static int +testQemuAgentGetInterfaces(const void *data) +{ + virDomainXMLOptionPtr xmlopt = (virDomainXMLOptionPtr)data; + qemuMonitorTestPtr test = qemuMonitorTestNewAgent(xmlopt); + size_t i; + int ret = -1; + int ifaces_count = 0; + virDomainInterfacePtr *ifaces = NULL; + + 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; + + if (ifaces_count != 4) { + virReportError(VIR_ERR_INTERNAL_ERROR, + "expected 4 interfaces, got %d", ret); + goto cleanup; + } + + if (ifaces[0]->naddrs != 2 || + ifaces[1]->naddrs != 4 || + ifaces[2]->naddrs != 4 || + ifaces[3]->naddrs != 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + "unexpected return value for number of IP addresses"); + 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(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 (STRNEQ(ifaces[2]->name, "eth1") || + 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; + } + + ret = 0; + +cleanup: + qemuMonitorTestFree(test); + if (ifaces) { + for (i = 0; i < ifaces_count; i++) + virDomainInterfaceFree(ifaces[i]); + } + VIR_FREE(ifaces); + + return ret; +}

static int mymain(void) @@ -605,6 +753,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 03/09/13 22:37, Nehal J Wani wrote:

On Tue, Sep 3, 2013 at 7:46 PM, Osier Yang <jyang@redhat.com> wrote:

...

Except what Daniel pointed out:

On 01/09/13 21:43, 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.

Although "ifconfig" is deprecated, IP aliases created by "ifconfig" are supported by this API. The legacy syntax of an IP alias is: "<ifname>:<alias-name>". Since we want all aliases to be clubbed under parent interface, simply stripping ":<alias-name>" suffices.

s/suffices/suffixes/, Here, I by suffices, I meant: "Be enough or adequate."

...

Note that IP aliases formed by "ip" aren't visible to "ifconfig", and aliases created by "ip" do not have any specific name. But we are lucky, as qemuga detects aliases created by both.

s/qemuga/qemu guest agent/, or s/qemuga/qemu-ga/,

src/qemu/qemu_agent.h: * Define qemuAgentGetInterfaces

src/qemu/qemu_agent.c: * Implement qemuAgentGetInterface

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

src/remote_protocol-sructs: * Define new structs

tests/qemuagenttest.c: * Add new test: testQemuAgentGetInterfaces Test cases for IP aliases, 0 or multiple ipv4/ipv6 address(es)

--- src/qemu/qemu_agent.c | 193 +++++++++++++++++++++++++++++++++++++++++++++++++ src/qemu/qemu_agent.h | 3 + src/qemu/qemu_driver.c | 55 ++++++++++++++ tests/qemuagenttest.c | 149 ++++++++++++++++++++++++++++++++++++++ 4 files changed, 400 insertions(+)

diff --git a/src/qemu/qemu_agent.c b/src/qemu/qemu_agent.c index 2cd0ccc..009ed77 100644 --- a/src/qemu/qemu_agent.c +++ b/src/qemu/qemu_agent.c @@ -1320,6 +1320,199 @@ cleanup: }

/* + * qemuAgentGetInterfaces: + * @mon: Agent

s/Agent/Agent monitor/,

+ * @ifaces: pointer to an array of pointers pointing to interface objects + * + * Issue guest-network-get-interfaces to guest agent, which returns the

s/the/a/,

+ * list of a interfaces of a running domain along with their IP and MAC

s/of a/of/,

+ * addresses. + * + * Returns: number of interfaces on success, -1 on error. + */ +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; + size_t ifaces_count = 0; + size_t addrs_count = 0; + virDomainInterfacePtr *ifaces_ret = NULL; + virHashTablePtr ifaces_store = NULL; + + /* Initially the bag of ifaces is empty */

"bag" is magic here, how about:

/* Hash table to handle the interface alias */

+ if (!(ifaces_store = virHashCreate(ifaces_count, NULL))) + return -1; + + if (!(cmd = qemuAgentMakeCommand("guest-network-get-interfaces", NULL))) + return -1;

You should free the created hash table.

In the label cleanup, I have freed the has table.

But you "returns -1" here.

...

+ + 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; + } + + for (i = 0; i < size; i++) { + virJSONValuePtr tmp_iface = virJSONValueArrayGet(ret_array, i); + virJSONValuePtr ip_addr_arr = NULL; + const char *hwaddr, *ifname_s, *name = NULL; + char **ifname = NULL; + int ip_addr_arr_size; + virDomainInterfacePtr iface = NULL; + + /* 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; + } + + /* Handle aliases of type <ifname>:<alias-name> */

I think no need to mention the "type" here, since it only can be "ifname:alias". So how about:

/* Handle interface alias (<ifname>:<alias>

+ ifname = virStringSplit(name, ":", 2); + ifname_s = ifname[0]; + + iface = virHashLookup(ifaces_store, ifname_s); + + /* If the storage bag doesn't contain this iface, add it */

s/storage bag/hash table/,

+ if (!iface) { + if (VIR_EXPAND_N(ifaces_ret, ifaces_count, 1) < 0) + goto cleanup;

goto error;

+ + if (VIR_ALLOC(ifaces_ret[ifaces_count - 1]) < 0) + goto cleanup;

goto error;

+ + if (virHashAddEntry(ifaces_store, ifname_s, + ifaces_ret[ifaces_count - 1]) < 0)

Do we really need to use the virDomainInterfacePtr object as hash value? isn't a reference count is enough? if you use the reference count as the hash value, the logic can be more compact and clear:

if (!iface) { VIR_EXPAND_N VIR_ALLOC virHashAddEntry ..... }

iface = iface_ret[ifaces_count - 1];

If I am not wrong, are you suggesting to have the position of the nth interface in ifaces_ret as the reference count?

No, something like "(void *)1" will work. The only purpose of the hash table in this function is to check whether the same interface name is already in the ifaces array, so no need to use a pointer to the interface object as the hash value, though it doesn't hurt. Osier

On 03/09/13 23:00, Nehal J Wani wrote:

On Tue, Sep 3, 2013 at 8:21 PM, Osier Yang <jyang@redhat.com> wrote:

...

On 03/09/13 22:37, Nehal J Wani wrote:

...

On Tue, Sep 3, 2013 at 7:46 PM, Osier Yang <jyang@redhat.com> wrote:

...

Except what Daniel pointed out:

On 01/09/13 21:43, 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.

Although "ifconfig" is deprecated, IP aliases created by "ifconfig" are supported by this API. The legacy syntax of an IP alias is: "<ifname>:<alias-name>". Since we want all aliases to be clubbed under parent interface, simply stripping ":<alias-name>" suffices.

s/suffices/suffixes/, Here, I by suffices, I meant: "Be enough or adequate."

...

Note that IP aliases formed by "ip" aren't visible to "ifconfig", and aliases created by "ip" do not have any specific name. But we are lucky, as qemuga detects aliases created by both.

s/qemuga/qemu guest agent/, or s/qemuga/qemu-ga/,

src/qemu/qemu_agent.h: * Define qemuAgentGetInterfaces

src/qemu/qemu_agent.c: * Implement qemuAgentGetInterface

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

src/remote_protocol-sructs: * Define new structs

tests/qemuagenttest.c: * Add new test: testQemuAgentGetInterfaces Test cases for IP aliases, 0 or multiple ipv4/ipv6 address(es)

--- src/qemu/qemu_agent.c | 193 +++++++++++++++++++++++++++++++++++++++++++++++++ src/qemu/qemu_agent.h | 3 + src/qemu/qemu_driver.c | 55 ++++++++++++++ tests/qemuagenttest.c | 149 ++++++++++++++++++++++++++++++++++++++ 4 files changed, 400 insertions(+)

diff --git a/src/qemu/qemu_agent.c b/src/qemu/qemu_agent.c index 2cd0ccc..009ed77 100644 --- a/src/qemu/qemu_agent.c +++ b/src/qemu/qemu_agent.c @@ -1320,6 +1320,199 @@ cleanup: }

/* + * qemuAgentGetInterfaces: + * @mon: Agent

s/Agent/Agent monitor/,

+ * @ifaces: pointer to an array of pointers pointing to interface objects + * + * Issue guest-network-get-interfaces to guest agent, which returns the

s/the/a/,

+ * list of a interfaces of a running domain along with their IP and MAC

s/of a/of/,

+ * addresses. + * + * Returns: number of interfaces on success, -1 on error. + */ +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; + size_t ifaces_count = 0; + size_t addrs_count = 0; + virDomainInterfacePtr *ifaces_ret = NULL; + virHashTablePtr ifaces_store = NULL; + + /* Initially the bag of ifaces is empty */

"bag" is magic here, how about:

/* Hash table to handle the interface alias */

+ if (!(ifaces_store = virHashCreate(ifaces_count, NULL))) + return -1; + + if (!(cmd = qemuAgentMakeCommand("guest-network-get-interfaces", NULL))) + return -1;

You should free the created hash table. In the label cleanup, I have freed the has table.

But you "returns -1" here.

...

...

+ + 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; + } + + for (i = 0; i < size; i++) { + virJSONValuePtr tmp_iface = virJSONValueArrayGet(ret_array, i); + virJSONValuePtr ip_addr_arr = NULL; + const char *hwaddr, *ifname_s, *name = NULL; + char **ifname = NULL; + int ip_addr_arr_size; + virDomainInterfacePtr iface = NULL; + + /* 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; + } + + /* Handle aliases of type <ifname>:<alias-name> */

I think no need to mention the "type" here, since it only can be "ifname:alias". So how about:

/* Handle interface alias (<ifname>:<alias>

+ ifname = virStringSplit(name, ":", 2); + ifname_s = ifname[0]; + + iface = virHashLookup(ifaces_store, ifname_s); + + /* If the storage bag doesn't contain this iface, add it */

s/storage bag/hash table/,

+ if (!iface) { + if (VIR_EXPAND_N(ifaces_ret, ifaces_count, 1) < 0) + goto cleanup;

goto error;

+ + if (VIR_ALLOC(ifaces_ret[ifaces_count - 1]) < 0) + goto cleanup;

goto error;

+ + if (virHashAddEntry(ifaces_store, ifname_s, + ifaces_ret[ifaces_count - 1]) < 0)

Do we really need to use the virDomainInterfacePtr object as hash value? isn't a reference count is enough? if you use the reference count as the hash value, the logic can be more compact and clear:

if (!iface) { VIR_EXPAND_N VIR_ALLOC virHashAddEntry ..... }

iface = iface_ret[ifaces_count - 1];

If I am not wrong, are you suggesting to have the position of the nth interface in ifaces_ret as the reference count?

No, something like "(void *)1" will work. The only purpose of the hash table in this function is to check whether the same interface name is already in the ifaces array, so no need to use a pointer to the interface object as the hash value, though it doesn't hurt.

Osier To check if interface already exists, I use:

iface = virHashLookup(ifaces_store, ifname_s);

Consider the case when the interface is already present, in that case, according to you, iface will be equal to (void *)1. Which is not what we want. We *need* iface to be equal to that virDomainInterfacePtr which points to the already existing interface, and hence it is put as hash value.

iface won't always be iface_ret[ifaces_count - 1];

I was confused, but then the refering the index works. However, in this case, I'm fine with either solution, since anyway the hash value is a pointer.

On 01/09/13 21:43, Nehal J Wani wrote:

Use virDomainInterfaceAddresses in virsh

tools/virsh-domain-monitor.c * Introduce new command : domifaddr Usage: domifaddr <domain> [interface] [full]

As Daniel pointed out, it should be "[--full]"

Example output: virsh # domifaddr f18 Name MAC address Protocol IP Address ------------------------------------------------------------------------------- lo 00:00:00:00:00:00 ipv4 127.0.0.1/8 - - ipv6 ::1/128 eth0 52:54:00:2e:45:ce ipv4 10.1.33.188/24 - - ipv6 2001:db8:0:f101::2/64 - - ipv6 fe80::5054:ff:fe2e:45ce/64 eth1 52:54:00:b1:70:19 ipv4 192.168.105.201/16 - - ipv4 192.168.201.195/16 - - ipv6 fe80::5054:ff:feb1:7019/64 eth2 52:54:00:36:2a:e5

tools/virsh.pod * Document new command

--- tools/virsh-domain-monitor.c | 119 +++++++++++++++++++++++++++++++++++++++++++ tools/virsh.pod | 11 ++++ 2 files changed, 130 insertions(+)

diff --git a/tools/virsh-domain-monitor.c b/tools/virsh-domain-monitor.c index b29b82a..cd1df7a 100644 --- a/tools/virsh-domain-monitor.c +++ b/tools/virsh-domain-monitor.c @@ -1871,6 +1871,119 @@ cleanup: } #undef FILTER

+/* "domifaddr" command + */ +static const vshCmdInfo info_domifaddr[] = { + {"help", N_("Get network interfaces' addresses for a running domain")}, + {"desc", N_("Get network interfaces' addresses for a running domain")}, + {NULL, NULL} +}; + +static const vshCmdOptDef opts_domifaddr[] = { + {"domain", VSH_OT_DATA, VSH_OFLAG_REQ, N_("domain name, id or uuid")}, + {"interface", VSH_OT_DATA, VSH_OFLAG_NONE, N_("network interface name")}, + {"full", VSH_OT_BOOL, VSH_OFLAG_NONE, N_("display full fields")}, + {NULL, 0, 0, NULL} +}; + +static bool +cmdDomIfAddr(vshControl *ctl, const vshCmd *cmd) +{ + virDomainPtr dom = NULL; + const char *interface = NULL; + virDomainInterfacePtr *ifaces = NULL; + size_t i, j; + int ifaces_count = 0; + unsigned int flags = 0; + bool ret = false; + bool optFull = vshCommandOptBool(cmd, "full");

s/optFull/full/,

+ + if (!(dom = vshCommandOptDomain(ctl, cmd, NULL))) + return false; + + if (vshCommandOptString(cmd, "interface", &interface) < 0) { + goto cleanup; + } + + if ((ifaces_count = virDomainInterfaceAddresses(dom, &ifaces, flags)) < 0) { + vshError(ctl, _("Failed to query for interfaces addresses")); + goto cleanup; + } + + vshPrintExtra(ctl, " %-10s %-20s %-8s %s\n%s%s\n", _("Name"), + _("MAC address"), _("Protocol"), _("Address"), + _("-------------------------------------------------"), + _("------------------------------")); + + for (i = 0; i < ifaces_count; i++) { + virDomainInterfacePtr iface = ifaces[i]; + const char *hwaddr = ""; + const char *ip_addr_str = NULL; + const char *type = NULL; + + if (interface && STRNEQ(interface, iface->name)) + continue; + + if (iface->hwaddr) + hwaddr = iface->hwaddr;

Variable "hwaddr" can be avoided? directly using iface->hwaddr should be fine.

+ + /* When the interface has no IP address */ + if (!iface->naddrs) { + vshPrintExtra(ctl, " %-10s %-17s\n", + iface->name, hwaddr);

Not harm to add "continue" here. It will avoid the enter into the later for loop. And to be consistent, we use "N/A" if the field is empty in virsh, instead of nothing in the output.

+ } + + for (j = 0; j < iface->naddrs; j++) { + virBuffer buf = VIR_BUFFER_INITIALIZER; + + switch (iface->addrs[j].type) { + case VIR_IP_ADDR_TYPE_IPV4: + type = "ipv4"; + break; + case VIR_IP_ADDR_TYPE_IPV6: + type = "ipv6"; + break; + } + + virBufferAsprintf(&buf, "%-12s %s/%d", + type, iface->addrs[j].addr, + iface->addrs[j].prefix); + + if (virBufferError(&buf)) { + virBufferFreeAndReset(&buf); + virReportOOMError(); + return ret;

Memory leak, need "goto cleanup;"

+ } + + ip_addr_str = virBufferContentAndReset(&buf); + + if (!ip_addr_str) + ip_addr_str = ""; + + /* Don't repeat interface name */ + if (optFull || !j) + vshPrintExtra(ctl, " %-10s %-17s %s\n", + iface->name, hwaddr, ip_addr_str); + else + vshPrintExtra(ctl, " %-10s %-17s %s\n", + "-","-",ip_addr_str);

"-", "-", ip_add_str); I.E Add the space after ","

+ + virBufferFreeAndReset(&buf); + } + } + + ret = true; + +cleanup: + if (ifaces) + 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 +2057,11 @@ const vshCmdDef domMonitoringCmds[] = { .info = info_list, .flags = 0 }, + {.name = "domifaddr", + .handler = cmdDomIfAddr, + .opts = opts_domifaddr, + .info = info_domifaddr, + .flags = 0 + }, {.name = NULL} }; diff --git a/tools/virsh.pod b/tools/virsh.pod index 0ae5178..04814fa 100644 --- a/tools/virsh.pod +++ b/tools/virsh.pod @@ -636,6 +636,17 @@ B<Explanation of fields> (fields appear in the following order): flush_total_times - total time flush operations took (ns) <-- other fields provided by hypervisor -->

+ +=item B<domifaddr> I<domain> [I<interface>] [I<--full>] + +Get a list of interfaces of a running domain along with their IP and MAC +addresses, or limited output just for one interface if I<interface> is +specified. Note, that I<interface> can be driver dependent, it can be +the name within guest OS or the name you would see in domain XML. +Moreover, the whole command may require a guest agent to be configured +for the queried domain under some drivers, notably qemu. All fields can +be fully displayed by passing the option: I<--full>.

This doesn't tell what the real purpose of <--full>. How about: If I<--full> is specified, the interface name is always displayed when the interface has multiple addresses or alias, otherwise it only displays the interface name for the first address, and "-" for the others.

+ =item B<domifstat> I<domain> I<interface-device>

Get network interface stats for a running domain.

On Sun, Sep 1, 2013 at 7:13 PM, Nehal J Wani <nehaljw.kkd1@gmail.com> wrote:

Expose virDomainInterfaceAddresses 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 virDomainInterfaceAddresses

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

Example: $ ./run python ./examples/python/domipaddrs.py f18 Interface 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 eth2 52:54:00:36:2a:e5 eth1 52:54:00:b1:70:19 ipv4 192.168.105.201/16 eth1 52:54:00:b1:70:19 ipv4 192.168.201.195/16 eth1 52:54:00:b1:70:19 ipv6 fe80::5054:ff:feb1:7019/64 eth0 52:54:00:2e:45:ce ipv4 10.1.33.188/24 eth0 52:54:00:2e:45:ce ipv6 2001:db8:0:f101::2/64 eth0 52:54:00:2e:45:ce ipv6 fe80::5054:ff:fe2e:45ce/64

--- examples/python/Makefile.am | 2 +- examples/python/README | 1 + examples/python/domipaddrs.py | 50 ++++++++++++++++++ python/libvirt-override-api.xml | 8 ++- python/libvirt-override.c | 111 ++++++++++++++++++++++++++++++++++++++++ 5 files changed, 170 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..679e0bf --- /dev/null +++ b/examples/python/domipaddrs.py @@ -0,0 +1,50 @@ +#!/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.interfaceAddresses(0) +if (ifaces == None): + print "Failed to get domain interfaces" + sys.exit(0) + +print " {0:10} {1:20} {2:12} {3}".format("Interface", "MAC address", "Protocol", "Address") + +for (name, val) in ifaces.iteritems(): + if val['ip_addrs']: + for addr in val['ip_addrs']: + print " {0:10} {1:19}".format(name, val['hwaddr']), + print " {0:12} {1}/{2} ".format(addr['type'], addr['addr'], addr['prefix']), + print + else: + print " {0:10} {1:19}".format(name, val['hwaddr']), + print diff --git a/python/libvirt-override-api.xml b/python/libvirt-override-api.xml index 9a88215..60491de 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='virDomainInterfaceAddresses' 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..67e0cb8 100644 --- a/python/libvirt-override.c +++ b/python/libvirt-override.c @@ -4761,6 +4761,116 @@ cleanup: return py_retval; }

+ +static PyObject * +libvirt_virDomainInterfaceAddresses(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 = virDomainInterfaceAddresses(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 = NULL; + + 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"; + break; + case VIR_IP_ADDR_TYPE_IPV6: + type = "ipv6"; + break; + } + + 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: + if (ifaces) { + for (i = 0; full_free && 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 + */ + virDomainInterfaceFree(ifaces[i]); + } + } + 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 +7394,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 *) "virDomainInterfaceAddresses", libvirt_virDomainInterfaceAddresses, 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

In agreement with Guido's suggestion on 5/5 of the previous version (https://www.redhat.com/archives/libvir-list/2013-August/msg01271.html), instead of string, the enum virIPAddrType is to be used in python, for which examples/python/domipaddrs.py and python/libvirt-override.c are to be modified a bit. Diff has been attached. -- Nehal J Wani