On 01/25/2015 01:38 PM, Nehal J Wani wrote:

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 DHCP lease file

include/libvirt/libvirt-domain.h * Define virDomainInterfaceAddresses, virDomainInterfaceFree * Define structs virDomainInterface, virDomainIPAddress

src/driver-hypervisor.h: * Define domainInterfaceAddresses

src/libvirt-domain.c: * Implement virDomainInterfaceAddresses * Implement virDomainInterfaceFree

src/libvirt_public.syms: * Export the new symbols

Signed-off-by: Nehal J Wani <nehaljw.kkd1@gmail.com> --- include/libvirt/libvirt-domain.h | 27 ++++++++ src/driver-hypervisor.h | 5 ++ src/libvirt-domain.c | 129 +++++++++++++++++++++++++++++++++++++++ src/libvirt_public.syms | 6 ++ 4 files changed, 167 insertions(+)

diff --git a/include/libvirt/libvirt-domain.h b/include/libvirt/libvirt-domain.h index 4dbd7f5..1f832d0 100644 --- a/include/libvirt/libvirt-domain.h +++ b/include/libvirt/libvirt-domain.h @@ -3682,5 +3682,32 @@ typedef struct _virTypedParameter virMemoryParameter; */ typedef virMemoryParameter *virMemoryParameterPtr;

+typedef enum { + VIR_DOMAIN_INTERFACE_ADDRESSES_LEASE = (1 << 0), /* Parse DHCP lease file */ + VIR_DOMAIN_INTERFACE_ADDRESSES_AGENT = (1 << 1), /* Query qemu guest agent */ +} virDomainInterfaceAddressesFlags; + +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);

While I haven't followed this from the first RFC or taken the time to look at all 8 patches, I'll assume this set of data has been "agreed upon" as the relatively important set of IP Address and Network Interface data. Looking at the .0 comments it seems the desire was for some more flexibility to handle future possible issues - I guess my comment there is - IPv4 isn't changing much and getting IPv6 adoption as the norm seems to have been an uphill religious battle for quite a few years - so as they say - change isn't very frequent thus perhaps this is a "safe set" of data to collect/display

#endif /* __VIR_LIBVIRT_DOMAIN_H__ */

<...snip...>

diff --git a/src/libvirt-domain.c b/src/libvirt-domain.c index 492e90a..4149332 100644 --- a/src/libvirt-domain.c +++ b/src/libvirt-domain.c @@ -11249,3 +11249,132 @@ virDomainFSInfoFree(virDomainFSInfoPtr info) VIR_FREE(info->devAlias[i]); VIR_FREE(info->devAlias); } + +/** + * virDomainInterfaceAddresses: + * @dom: domain object + * @ifaces: pointer to an array of pointers pointing to interface objects + * @flags: bitwise-OR of virDomainInterfaceAddressesFlags + * + * Return a pointer to the allocated array of pointers pointing to interfaces + * present in given domain along with their IP and MAC addresses. Note that + * single interface can have multiple or even 0 IP address. + * + * This API dynamically allocates the virDomainInterfacePtr struct based on + * how many interfaces domain @dom has, usually there's 1:1 correlation. The + * count of the interfaces is returned as the return value. + * + * In case @flags includes VIR_DOMAIN_INTERFACE_ADDRESSES_AGENT, a configured + * 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. + * + * The lease-file parsing method returns the interface name of the form "vnetN", + * which is different from what guest agent returns (like ethN or emN), and + * since the MAC address from guest agent might be different with what @dom XML + * specifies, we have no way to convert it into the names present in @dom + * config. Hence, it is not recommended to mix the flag ..._AGENT with + * ..._LEASE as it may lead to ambiguous results because we cannot be sure if + * the name came from the agent or from the other method.

Reads to me like perhaps this should be an EITHER ... OR situation and not an AND type allowance...

+ * + * If 0 is passed as @flags, libvirt will choose the best way, and won't + * include agent in it. + * + * @ifaces->name and @ifaces->hwaddr are never NULL. + * + * The caller *must* free @ifaces when no longer needed. Usual use case + * looks like this: + * + * virDomainInterfacePtr *ifaces = NULL; + * int ifaces_count = 0; + * size_t i, j; + * virDomainPtr dom = ... obtain a domain here ...; + * + * if ((ifaces_count = virDomainInterfaceAddresses(dom, &ifaces, 0)) < 0) + * goto cleanup; + * + * ... do something with returned values, for example: + * for (i = 0; i < ifaces_count; i++) { + * printf("name: %s", ifaces[i]->name); + * if (ifaces[i]->hwaddr) + * printf(" hwaddr: %s", ifaces[i]->hwaddr); + * + * for (j = 0; j < ifaces[i]->naddrs; j++) { + * virDomainIPAddressPtr ip_addr = ifaces[i]->addrs + j; + * printf("[addr: %s prefix: %d type: %d]", + * ip_addr->addr, ip_addr->prefix, ip_addr->type); + * } + * printf("\n"); + * } + * + * cleanup: + * if (ifaces) + * for (i = 0; i < ifaces_count; i++) + * virDomainInterfaceFree(ifaces[i]); + * free(ifaces); + * + * Returns the number of interfaces on success, -1 in case of error. + */ +int +virDomainInterfaceAddresses(virDomainPtr dom, + virDomainInterfacePtr **ifaces, + unsigned int flags) +{ + virConnectPtr conn; + + VIR_DOMAIN_DEBUG(dom, "ifaces=%p, flags=%x", ifaces, flags); + + virResetLastError(); + + conn = dom->conn;

Need a virCheckDomainReturn() prior to deref

+ + virCheckNonNullArgGoto(ifaces, error);

Add a "*ifaces = NULL;" here. John

+ + if ((flags & VIR_DOMAIN_INTERFACE_ADDRESSES_AGENT) + && (conn->flags & VIR_CONNECT_RO)) { + virReportError(VIR_ERR_OPERATION_DENIED, "%s", + _("Cannot query guest agent in readonly mode")); + goto error; + } + + if (conn->driver->domainInterfaceAddresses) { + int ret; + ret = conn->driver->domainInterfaceAddresses(dom, ifaces, flags); + if (ret < 0) + goto error; + return ret; + } + + virReportError(VIR_ERR_NO_SUPPORT, __FUNCTION__); + + 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); +}

On 03/05/2015 07:45 AM, Daniel P. Berrange wrote:

On Mon, Jan 26, 2015 at 12:08:46AM +0530, Nehal J Wani wrote:

...

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 DHCP lease file

...

+++ b/include/libvirt/libvirt-domain.h @@ -3682,5 +3682,32 @@ typedef struct _virTypedParameter virMemoryParameter; */ typedef virMemoryParameter *virMemoryParameterPtr;

+typedef enum { + VIR_DOMAIN_INTERFACE_ADDRESSES_LEASE = (1 << 0), /* Parse DHCP lease file */ + VIR_DOMAIN_INTERFACE_ADDRESSES_AGENT = (1 << 1), /* Query qemu guest agent */ +} virDomainInterfaceAddressesFlags;

This feels like it should not be named 'Flags', because it seems like you can never request multiple modes at once.

I can't help but have a nagging feeling this ambiguos return data means our API design is wrong. Instead of having a bitwise OR of flags to select the data source, should we perhaps use a straight enum, so the application always specifies exactly which data source to use ?

eg an API that is

enum { VIR_DOMAIN_INTERFACE_ADDRESSES_SOURCE_LEASES, VIR_DOMAIN_INTERFACE_ADDRESSES_SOURCE_AGENT, VIR_DOMAIN_INTERFACE_ADDRESSES_SOURCE_SNOOP, };

Yep - that's exactly my concern, and the right solution (that is, _LEASES is 0, _AGENT is 1, _SNOOP is 2, and for now we have a _LAST at 3 that can later bump to 4 if we add another method).

virDomainInterfaceAddresses(virDomainPtr dom, int source, virDomainInterfacePtr **ifaces, unsigned int flags);

so the caller always knows how to interpret the returned data for mac addr andname. Leave the flags field unused, for future help if needed.

Yes, that sounds like the right direction. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org

On 01/25/2015 01:38 PM, 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 DoS 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

Signed-off-by: Nehal J Wani <nehaljw.kkd1@gmail.com> --- daemon/remote.c | 134 +++++++++++++++++++++++++++++++++++++++++++ src/libvirt_public.syms | 4 +- src/remote/remote_driver.c | 100 ++++++++++++++++++++++++++++++++ src/remote/remote_protocol.x | 36 +++++++++++- src/remote_protocol-structs | 24 ++++++++ 5 files changed, 295 insertions(+), 3 deletions(-)

diff --git a/daemon/remote.c b/daemon/remote.c index d657a09..32b567c 100644 --- a/daemon/remote.c +++ b/daemon/remote.c @@ -6438,6 +6438,140 @@ remoteDispatchDomainGetFSInfo(virNetServerPtr server ATTRIBUTE_UNUSED, }

+static int +remoteSerializeDomainInterface(virDomainInterfacePtr *ifaces, + unsigned int ifaces_count, + remote_domain_interface_addresses_ret *ret) +{ + size_t i, j; + + if (ifaces_count > REMOTE_DOMAIN_INTERFACE_MAX) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Number of interfaces, %d exceeds the max limit: %d"), + ifaces_count, REMOTE_DOMAIN_INTERFACE_MAX); + return -1; + } + + if (VIR_ALLOC_N(ret->ifaces.ifaces_val, ifaces_count) < 0) + return -1;

Something is not right here... Coverity squawks later on [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; + }

so hw_addrp is an allocated array of one element of which the [0] is allocated (strdup'd); however, there's only ever one free... Should this be something like: if (iface->hwaddr) { if (VIR_ALLOC_N(iface_ret->hwaddr, strlen(iface->hwaddr) + 1) < 0) goto cleanup; memcpy(iface_ret->hwaddr, iface->hwaddr, strlen(iface->hwaddr)); } ?

+ + iface_ret->hwaddr = hwaddr_p; + } + + if (iface->naddrs > REMOTE_DOMAIN_IP_ADDR_MAX) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Number of interfaces, %d exceeds the max limit: %d"), + iface->naddrs, REMOTE_DOMAIN_IP_ADDR_MAX); + 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);

hwaddr was a VIR_ALLOC then VIR_STRDUP(*iface_ret->hwaddr)... so this would leak the strdup'd string.... unless you change to perhaps _N model.

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

^^^[1] Coverity points out that iface_ret is an incorrect free since it wasn't allocated separately.... The ifaces_val is an VIR_ALLOC_N, but the ifaces_val[i] is not.

+ } + VIR_FREE(ret->ifaces.ifaces_val); + } + + return -1; +} + + +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) +{ + size_t i; + 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 = virDomainInterfaceAddresses(dom, &ifaces, args->flags)) < 0)

[1] Coverity found this one...

+ goto cleanup; + + if (remoteSerializeDomainInterface(ifaces, ifaces_count, ret) < 0) + goto cleanup; + + rv = 0; + + cleanup: + if (rv < 0) + virNetMessageSaveError(rerr); + + virObjectUnref(dom); + + if (ifaces) { + for (i = 0; i < ifaces_count; i++)

[1] Coverity doesn't like this because ifaces_count can be negative, but it doesn't know/follow enough of the path in virDomainInterfaceAddresses to know they two are related. Coverity is pacified with the following if (ifaces_count > 0) { for (i = 0; i < ifaces_count; i++) ...

+ virDomainInterfaceFree(ifaces[i]); + VIR_FREE(ifaces); + } + + return rv; +} + + /*----- Helpers. -----*/

/* get_nonnull_domain and get_nonnull_network turn an on-wire diff --git a/src/libvirt_public.syms b/src/libvirt_public.syms index ab86543..52a0a64 100644 --- a/src/libvirt_public.syms +++ b/src/libvirt_public.syms @@ -695,10 +695,10 @@ LIBVIRT_1.2.12 { virDomainDefineXMLFlags; } LIBVIRT_1.2.11;

-LIBVIRT_1.2.12 { +LIBVIRT_1.2.13 { global: virDomainInterfaceAddresses; virDomainInterfaceFree; -} LIBVIRT_1.2.13; +} LIBVIRT_1.2.12;

This hunk needed to be extracted/merged into patch 1

# .... define new API here using predicted next version number .... diff --git a/src/remote/remote_driver.c b/src/remote/remote_driver.c index 3cc603f..ec0532d 100644 --- a/src/remote/remote_driver.c +++ b/src/remote/remote_driver.c @@ -7942,6 +7942,105 @@ remoteDomainGetFSInfo(virDomainPtr dom, }

+static int +remoteDomainInterfaceAddresses(virDomainPtr dom, + virDomainInterfacePtr **ifaces, + unsigned int flags) +{ + int rv = -1; + size_t i, j; + + virDomainInterfacePtr *ifaces_ret = NULL; + remote_domain_interface_addresses_args args; + remote_domain_interface_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_INTERFACE_ADDRESSES, + (xdrproc_t)xdr_remote_domain_interface_addresses_args, + (char *)&args, + (xdrproc_t)xdr_remote_domain_interface_addresses_ret, + (char *)&ret) == -1) { + goto done; + } + + if (ret.ifaces.ifaces_len > REMOTE_DOMAIN_INTERFACE_MAX) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Number of interfaces, %d exceeds the max limit: %d"), + ret.ifaces.ifaces_len, REMOTE_DOMAIN_INTERFACE_MAX); + goto cleanup; + } + + 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]);

While it's allowed under some compilers - I believe others will complain about defs after code - eg use the following: for (i = 0; i < ret.ifaces.ifaces_len; i++) virDomainInterfacePtr iface; remote_domain_interface *iface_ret = &(ret.ifaces.ifaces_val[i]); if (VIR_ALLOC(ifaces_ret[i]) < 0) goto cleanup; iface = ifaces_ret[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;

Note my comment above in remote.c - may change how this is handled.

+ + if (iface_ret->addrs.addrs_len > REMOTE_DOMAIN_IP_ADDR_MAX) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Number of interfaces, %d exceeds the max limit: %d"), + iface_ret->addrs.addrs_len, REMOTE_DOMAIN_IP_ADDR_MAX); + 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; + + 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_interface_addresses_ret, + (char *) &ret); + done: + remoteDriverUnlock(priv); + return rv; +} + + /* get_nonnull_domain and get_nonnull_network turn an on-wire * (name, uuid) pair into virDomainPtr or virNetworkPtr object. * These can return NULL if underlying memory allocations fail, @@ -8286,6 +8385,7 @@ static virHypervisorDriver hypervisor_driver = { .connectGetAllDomainStats = remoteConnectGetAllDomainStats, /* 1.2.8 */ .nodeAllocPages = remoteNodeAllocPages, /* 1.2.9 */ .domainGetFSInfo = remoteDomainGetFSInfo, /* 1.2.11 */ + .domainInterfaceAddresses = remoteDomainInterfaceAddresses, /* 1.2.12 */

1.2.13 (at least) John

};

static virNetworkDriver network_driver = {

<...snip...>

On 01/25/2015 01:38 PM, 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. 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 qemu guest agent detects aliases created by both.

src/qemu/qemu_agent.h: * Define qemuAgentGetInterfaces

src/qemu/qemu_agent.c: * Implement qemuAgentGetInterface

src/qemu/qemu_driver.c: * New function qemuGetDHCPInterfaces * 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)

Signed-off-by: Nehal J Wani <nehaljw.kkd1@gmail.com> --- src/qemu/qemu_agent.c | 202 +++++++++++++++++++++++++++++++++++++++++++++++++ src/qemu/qemu_agent.h | 4 + src/qemu/qemu_driver.c | 173 ++++++++++++++++++++++++++++++++++++++++++ tests/qemuagenttest.c | 188 +++++++++++++++++++++++++++++++++++++++++++++ 4 files changed, 567 insertions(+)

diff --git a/src/qemu/qemu_agent.c b/src/qemu/qemu_agent.c index 5fcc40f..e881cdc 100644 --- a/src/qemu/qemu_agent.c +++ b/src/qemu/qemu_agent.c @@ -1953,3 +1953,205 @@ qemuAgentGetFSInfo(qemuAgentPtr mon, virDomainFSInfoPtr **info, virJSONValueFree(reply); return ret; } + +/* + * qemuAgentGetInterfaces: + * @mon: Agent monitor + * @ifaces: pointer to an array of pointers pointing to interface objects + * + * Issue guest-network-get-interfaces to guest agent, which returns a + * list of interfaces of a running domain along with their IP and MAC + * 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; + char **ifname = NULL; + + /* Hash table to handle the interface alias */ + if (!(ifaces_store = virHashCreate(ifaces_count, NULL))) { + virHashFree(ifaces_store); + return -1; + } + + if (!(cmd = qemuAgentMakeCommand("guest-network-get-interfaces", NULL))) + goto cleanup; + + if (qemuAgentCommand(mon, cmd, &reply, false, 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; + 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 interface alias (<ifname>:<alias>) */ + ifname = virStringSplit(name, ":", 2); + ifname_s = ifname[0]; + + iface = virHashLookup(ifaces_store, ifname_s); + + /* If the hash table doesn't contain this iface, add it */ + if (!iface) { + if (VIR_EXPAND_N(ifaces_ret, ifaces_count, 1) < 0) + goto error; + + if (VIR_ALLOC(ifaces_ret[ifaces_count - 1]) < 0) + goto error; + + if (virHashAddEntry(ifaces_store, ifname_s, + ifaces_ret[ifaces_count - 1]) < 0) + goto error; + + iface = ifaces_ret[ifaces_count - 1]; + iface->naddrs = 0; + + if (VIR_STRDUP(iface->name, ifname_s) < 0) + goto error; + + hwaddr = virJSONValueObjectGetString(tmp_iface, "hardware-address"); + if (!hwaddr) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("qemu agent didn't provide" + " 'hardware-address' field")); + goto error; + } + + if (VIR_STRDUP(iface->hwaddr, hwaddr) < 0) + goto error;

[1] According to this set of checks and the one later in the DHCP code - upon return iface->hwaddr is filled in; however, in patch 2 there's a check for "if (iface->hwaddr)" before the VIR_ALLOC/VIR_STRDUP (or as I asked, why not VIR_ALLOC_N/memcpy). Given that this and the other occurrance guarantee something filled in - why does the remote code make the extra handstand in order to validate?

+ } + + /* Has to be freed for each interface. */ + virStringFreeList(ifname); + + /* as well as IP address which - moreover - + * can be presented multiple times */

This comment in combination the one directly above made me first think - we're free'ing something here...

+ ip_addr_arr = virJSONValueObjectGet(tmp_iface, "ip-addresses"); + if (!ip_addr_arr) + continue; +

Remove extra line whitespace

+ + if ((ip_addr_arr_size = virJSONValueArraySize(ip_addr_arr)) < 0) + /* Mmm, empty 'ip-address'? */ + goto error; + + /* If current iface already exists, continue with the count */ + addrs_count = iface->naddrs; + + for (j = 0; j < ip_addr_arr_size; j++) { + 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;

here again we have definitions after code - move the definitions up or the VIR_EXPAND_N down depending on how you view it... in fact I'd probably move the VIR_EXPAND_N down below the following "if"...

+ + /* 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); + virStringFreeList(ifname); + + goto cleanup; +} diff --git a/src/qemu/qemu_agent.h b/src/qemu/qemu_agent.h index c983828..1cd5749 100644 --- a/src/qemu/qemu_agent.h +++ b/src/qemu/qemu_agent.h @@ -108,4 +108,8 @@ int qemuAgentSetTime(qemuAgentPtr mon, long long seconds, unsigned int nseconds, bool sync); + +int qemuAgentGetInterfaces(qemuAgentPtr mon, + virDomainInterfacePtr **ifaces); + #endif /* __QEMU_AGENT_H__ */ diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index bc6aae4..5e7207b 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -171,6 +171,9 @@ static int qemuOpenFileAs(uid_t fallback_uid, gid_t fallback_gid, const char *path, int oflags, bool *needUnlink, bool *bypassSecurityDriver);

+static int qemuGetDHCPInterfaces(virDomainPtr dom, + virDomainObjPtr vm, + virDomainInterfacePtr **ifaces);

virQEMUDriverPtr qemu_driver = NULL;

@@ -18897,6 +18900,175 @@ qemuDomainGetFSInfo(virDomainPtr dom, return ret; }

+static int +qemuDomainInterfaceAddresses(virDomainPtr dom, + virDomainInterfacePtr **ifaces, + unsigned int flags) +{ + virQEMUDriverPtr driver = dom->conn->privateData; + qemuDomainObjPrivatePtr priv = NULL; + virDomainObjPtr vm = NULL; + int ret = -1, i = 0; + bool tryLease, tryAgent; + + if (!(vm = qemuDomObjFromDomain(dom))) + goto cleanup; + + priv = vm->privateData; + + if (!virDomainObjIsActive(vm)) { + virReportError(VIR_ERR_OPERATION_INVALID, "%s", + _("domain is not running")); + goto cleanup; + } + + if (virDomainInterfaceAddressesEnsureACL(dom->conn, vm->def) < 0) + goto cleanup; + + if (flags) { + tryLease = (flags & VIR_DOMAIN_INTERFACE_ADDRESSES_LEASE); + tryAgent = (flags & VIR_DOMAIN_INTERFACE_ADDRESSES_AGENT); + } else { + tryLease = false; + i = vm->def->nnets; + /* check if the guest has any NICs on libvirt virtual network */ + while (i >= 0 && !tryLease) + tryLease = (vm->def->nets[--i]->type == VIR_DOMAIN_NET_TYPE_NETWORK); + + tryAgent = (priv->agent != NULL); + } + + if (tryLease) { + ret = qemuGetDHCPInterfaces(dom, vm, ifaces); + goto cleanup; + } + + if (tryAgent) { + if (priv->agentError) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("QEMU guest agent is not " + "available due to an error")); + goto cleanup; + } + More recent agent related code seems to have BeginJob here... then check again for domain obj active:

if (!virDomainObjIsActive(vm)) { virReportError(VIR_ERR_OPERATION_INVALID, "%s", _("domain is not running")); goto endjob; }

+ if (!priv->agent) { + virReportError(VIR_ERR_ARGUMENT_UNSUPPORTED, "%s", + _("QEMU guest agent is not configured")); + goto cleanup;

then have goto endjob here...

+ } + + if (qemuDomainObjBeginJob(driver, vm, QEMU_JOB_QUERY) < 0) + goto cleanup; + + qemuDomainObjEnterAgent(vm); + ret = qemuAgentGetInterfaces(priv->agent, ifaces); + qemuDomainObjExitAgent(vm); + endjob:

+ qemuDomainObjEndJob(driver, vm); + + goto cleanup; + } + + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("No IP address data source found")); + + cleanup: + if (vm) + virObjectUnlock(vm); + return ret; +} + +static int +qemuGetDHCPInterfaces(virDomainPtr dom, + virDomainObjPtr vm, + virDomainInterfacePtr **ifaces) +{ + int rv = -1; + int n_leases = 0; + size_t i, j; + size_t ifaces_count = 0; + virNetworkPtr network; + char macaddr[VIR_MAC_STRING_BUFLEN]; + virDomainInterfacePtr iface = NULL; + virNetworkDHCPLeasePtr *leases = NULL; + virDomainInterfacePtr *ifaces_ret = NULL; +

DHCP is not my area of expertise...

+ if (dom->conn->networkDriver && + dom->conn->networkDriver->networkGetDHCPLeases) {

So you're checking if this connection has the API... and then calling the GetDHCPLeases 'generally' - I dunno, this just seems odd and out of place.

+ + for (i = 0; i < vm->def->nnets; i++) { + if (vm->def->nets[i]->type != VIR_DOMAIN_NET_TYPE_NETWORK) ^^

Extra space

+ continue; + + virMacAddrFormat(&(vm->def->nets[i]->mac), macaddr); + network = virNetworkLookupByName(dom->conn, + vm->def->nets[i]->data.network.name); + + if ((n_leases = virNetworkGetDHCPLeases(network, macaddr, + &leases, 0)) < 0) ^^^ move under network alignment issues.

+ goto error; + + if (n_leases) { + if (VIR_EXPAND_N(ifaces_ret, ifaces_count, 1) < 0) + goto error; + + if (VIR_ALLOC(ifaces_ret[ifaces_count - 1]) < 0) + goto error; + + iface = ifaces_ret[ifaces_count - 1]; + /* Assuming each lease corresponds to a separate IP */ + iface->naddrs = n_leases; + + if (VIR_ALLOC_N(iface->addrs, iface->naddrs) < 0) + goto error; + + if (VIR_STRDUP(iface->name, vm->def->nets[i]->ifname) < 0) + goto cleanup; + + if (VIR_STRDUP(iface->hwaddr, macaddr) < 0) + goto cleanup;

[1] See comment above in agent code regarding this value and it's relationship in the 2/4 (remote) code...

+ } + + for (j = 0; j < n_leases; j++) { + virNetworkDHCPLeasePtr lease = leases[j]; + virDomainIPAddressPtr ip_addr = &iface->addrs[j]; + + if (VIR_STRDUP(ip_addr->addr, lease->ipaddr) < 0) + goto cleanup; + + ip_addr->type = lease->type; + ip_addr->prefix = lease->prefix; + } + + for (j = 0; j < n_leases; j++) + virNetworkDHCPLeaseFree(leases[j]); + + VIR_FREE(leases); + } + } + + *ifaces = ifaces_ret; + ifaces_ret = NULL; + rv = ifaces_count;

If we failed the conn->networkDriver comparison, then we fall to here and return *ifaces = NULL; and rv = 0; Which means the caller gets nothing and seems OK, but I'm just trying to think through this... With a 0 return here - is there condition or reason that perhaps you'd want to try the agent if flags weren't provided in the caller rather than just returning nothing? Just trying to think of some strange edge condition...

+ + cleanup: + if (leases) { + for (i = 0; i < n_leases; i++) + virNetworkDHCPLeaseFree(leases[i]); + } + VIR_FREE(leases); + + return rv; + + error: + if (ifaces_ret) { + for (i = 0; i < ifaces_count; i++) + virDomainInterfaceFree(ifaces_ret[i]); + } + VIR_FREE(ifaces_ret); + + goto cleanup; +}

static virHypervisorDriver qemuDriver = { .no = VIR_DRV_QEMU, @@ -19100,6 +19272,7 @@ static virHypervisorDriver qemuDriver = { .connectGetAllDomainStats = qemuConnectGetAllDomainStats, /* 1.2.8 */ .nodeAllocPages = qemuNodeAllocPages, /* 1.2.9 */ .domainGetFSInfo = qemuDomainGetFSInfo, /* 1.2.11 */ + .domainInterfaceAddresses = qemuDomainInterfaceAddresses, /* 1.2.12 */

1.2.13 (at least)

};

Yah! tests... didn't see anything in the tests that appeared to be wrong/missing. John

diff --git a/tests/qemuagenttest.c b/tests/qemuagenttest.c index 54a45df..d8d1e41 100644 --- a/tests/qemuagenttest.c +++ b/tests/qemuagenttest.c @@ -723,6 +723,193 @@ testQemuAgentTimeout(const void *data) return ret; }

+static const char testQemuAgentGetInterfacesResponse[] = + "{\"return\": " + " [" + " {\"name\":\"eth2\"," + " \"hardware-address\":\"52:54:00:36:2a:e5\"" + " }," + " {\"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\":\"eth0\"," + " \"ip-addresses\":" + " [" + " {\"ip-address-type\":\"ipv6\"," + " \"ip-address\":\"fe80::5054:ff:fe89:ad35\"," + " \"prefix\":64" + " }," + " {\"ip-address-type\":\"ipv4\"," + " \"ip-address\":\"192.168.102.142\"," + " \"prefix\":24" + " }," + " {\"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\":32" + " }," + " {\"ip-address-type\":\"ipv6\"," + " \"ip-address\":\"fe80::5054:ff:fed3:39ee\"," + " \"prefix\":64" + " }" + " ]," + " \"hardware-address\":\"52:54:00:d3:39:ee\"" + " }," + " {\"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\"" + " }" + " ]" + "}"; + +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 (STRNEQ(ifaces[0]->name, "eth2") || + STRNEQ(ifaces[1]->name, "eth1") || + STRNEQ(ifaces[2]->name, "eth0") || + STRNEQ(ifaces[3]->name, "lo")) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + "unexpected return values for interface names"); + goto cleanup; + } + + if (STRNEQ(ifaces[0]->hwaddr, "52:54:00:36:2a:e5") || + STRNEQ(ifaces[1]->hwaddr, "52:54:00:d3:39:ee") || + STRNEQ(ifaces[2]->hwaddr, "52:54:00:89:ad:35") || + STRNEQ(ifaces[3]->hwaddr, "00:00:00:00:00:00")) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + "unexpected return values for MAC addresses"); + goto cleanup; + } + + if (ifaces[0]->naddrs != 0 || + ifaces[1]->naddrs != 4 || + ifaces[2]->naddrs != 4 || + ifaces[3]->naddrs != 2) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + "unexpected return values for number of IP addresses"); + goto cleanup; + } + + if (ifaces[1]->addrs[0].type != VIR_IP_ADDR_TYPE_IPV4 || + ifaces[1]->addrs[1].type != VIR_IP_ADDR_TYPE_IPV6 || + ifaces[1]->addrs[2].type != VIR_IP_ADDR_TYPE_IPV4 || + ifaces[1]->addrs[3].type != VIR_IP_ADDR_TYPE_IPV6 || + ifaces[2]->addrs[0].type != VIR_IP_ADDR_TYPE_IPV6 || + ifaces[2]->addrs[1].type != VIR_IP_ADDR_TYPE_IPV4 || + ifaces[2]->addrs[2].type != VIR_IP_ADDR_TYPE_IPV4 || + ifaces[2]->addrs[3].type != VIR_IP_ADDR_TYPE_IPV6 || + ifaces[3]->addrs[0].type != VIR_IP_ADDR_TYPE_IPV4 || + ifaces[3]->addrs[1].type != VIR_IP_ADDR_TYPE_IPV6) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + "unexpected return values for IP address types"); + goto cleanup; + } + + if (ifaces[1]->addrs[0].prefix != 24 || + ifaces[1]->addrs[1].prefix != 64 || + ifaces[1]->addrs[2].prefix != 32 || + ifaces[1]->addrs[3].prefix != 64 || + ifaces[2]->addrs[0].prefix != 64 || + ifaces[2]->addrs[1].prefix != 24 || + ifaces[2]->addrs[2].prefix != 16 || + ifaces[2]->addrs[3].prefix != 64 || + ifaces[3]->addrs[0].prefix != 8 || + ifaces[3]->addrs[1].prefix != 128) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + "unexpected return values for IP address prefix"); + goto cleanup; + } + + if (STRNEQ(ifaces[1]->addrs[0].addr, "192.168.10.91") || + STRNEQ(ifaces[1]->addrs[1].addr, "fe80::fc54:ff:fefe:4c4f") || + STRNEQ(ifaces[1]->addrs[2].addr, "192.168.103.83") || + STRNEQ(ifaces[1]->addrs[3].addr, "fe80::5054:ff:fed3:39ee") || + STRNEQ(ifaces[2]->addrs[0].addr, "fe80::5054:ff:fe89:ad35") || + STRNEQ(ifaces[2]->addrs[1].addr, "192.168.102.142") || + STRNEQ(ifaces[2]->addrs[2].addr, "192.168.234.152") || + STRNEQ(ifaces[2]->addrs[3].addr, "fe80::5054:ff:fec3:68bb") || + STRNEQ(ifaces[3]->addrs[0].addr, "127.0.0.1") || + STRNEQ(ifaces[3]->addrs[1].addr, "::1")) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + "unexpected return values for IP address values"); + 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) @@ -753,6 +940,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 01/25/2015 01:38 PM, Nehal J Wani wrote:

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

Example outputs: virsh # domifaddr f20 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: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 N/A N/A eth3 52:54:00:20:70:3d ipv4 192.168.105.240/16 - - ipv6 fe80::5054:ff:fe20:703d/64

virsh # domifaddr f20 eth1 --agent Name MAC address Protocol Address ------------------------------------------------------------------------------- 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

virsh # domifaddr f20 eth0 --agent --full Name MAC address Protocol Address ------------------------------------------------------------------------------- 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

tools/virsh.pod * Document new command

I for one like the format of the output and appreciate seeing it in the commit message!

Signed-off-by: Nehal J Wani <nehaljw.kkd1@gmail.com> --- tools/virsh-domain-monitor.c | 141 +++++++++++++++++++++++++++++++++++++++++++ tools/virsh.pod | 16 +++++ 2 files changed, 157 insertions(+)

diff --git a/tools/virsh-domain-monitor.c b/tools/virsh-domain-monitor.c index 925eb1b..960b831 100644 --- a/tools/virsh-domain-monitor.c +++ b/tools/virsh-domain-monitor.c @@ -2177,6 +2177,141 @@ cmdDomstats(vshControl *ctl, const vshCmd *cmd) return ret; }

+/* "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[] = { + {.name = "domain", + .type = VSH_OT_DATA, + .flags = VSH_OFLAG_REQ, + .help = N_("domain name, id or uuid")}, + {.name = "interface", + .type = VSH_OT_STRING, + .flags = VSH_OFLAG_NONE, + .help = N_("network interface name")}, + {.name = "full", + .type = VSH_OT_BOOL, + .flags = VSH_OFLAG_NONE, + .help = N_("display full fields")}, + {.name = "lease", + .type = VSH_OT_BOOL, + .flags = VSH_OFLAG_NONE, + .help = N_("parse dhcp lease file")}, + {.name = "agent", + .type = VSH_OT_BOOL, + .flags = VSH_OFLAG_NONE, + .help = N_("query qemu guest agent")}, + {.name = 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 full = vshCommandOptBool(cmd, "full"); + + if (!(dom = vshCommandOptDomain(ctl, cmd, NULL))) + return false; + + if (vshCommandOptString(cmd, "interface", &interface) < 0) + goto cleanup; + + if (vshCommandOptBool(cmd, "lease")) + flags |= VIR_DOMAIN_INTERFACE_ADDRESSES_LEASE; + + if (vshCommandOptBool(cmd, "agent")) + flags |= VIR_DOMAIN_INTERFACE_ADDRESSES_AGENT; +

Since only one style would be displayed (lease in preference over agent) - providing both flags could be an error... Unless of course you change 3/4 to allow tryAgent after tryLease returns.

+ if ((ifaces_count = virDomainInterfaceAddresses(dom, &ifaces, flags)) < 0) {

[1] Same Coverity issue as 2/4

+ 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; + + hwaddr = iface->hwaddr; + + /* When the interface has no IP address */ + if (!iface->naddrs) { + vshPrintExtra(ctl, " %-10s %-17s %-12s %s\n", + iface->name, hwaddr, "N/A", "N/A"); + continue; + } + + 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",

Should this be " %-12s..." instead of "%-12s" (since everything else adds that extra space).

+ type, iface->addrs[j].addr, + iface->addrs[j].prefix); + + if (virBufferError(&buf)) { + virBufferFreeAndReset(&buf); + virReportOOMError(); + goto cleanup; + } + + ip_addr_str = virBufferContentAndReset(&buf); + + if (!ip_addr_str) + ip_addr_str = ""; + + /* Don't repeat interface name */ + if (full || !j) + vshPrintExtra(ctl, " %-10s %-17s %s\n", + iface->name, hwaddr, ip_addr_str); + else + vshPrintExtra(ctl, " %-10s %-17s %s\n", + "-", "-", ip_addr_str); + + virBufferFreeAndReset(&buf); + } + } + + ret = true; + + cleanup: + if (ifaces)

again here Coverity complains because it doesn't understand the relationship between ifaces_count and ifaces, so going with : if (ifaces_count > 0) { for (i = 0; i < ifaces_count; i++) virDomainInterfaceFree(ifaces[i]); } pacifies Coverity.

+ for (i = 0; i < ifaces_count; i++) + virDomainInterfaceFree(ifaces[i]); + VIR_FREE(ifaces); + + virDomainFree(dom); + return ret; +} + const vshCmdDef domMonitoringCmds[] = { {.name = "domblkerror", .handler = cmdDomBlkError, @@ -2214,6 +2349,12 @@ const vshCmdDef domMonitoringCmds[] = { .info = info_domif_getlink, .flags = 0 }, + {.name = "domifaddr", + .handler = cmdDomIfAddr, + .opts = opts_domifaddr, + .info = info_domifaddr, + .flags = 0 + }, {.name = "domiflist", .handler = cmdDomiflist, .opts = opts_domiflist, diff --git a/tools/virsh.pod b/tools/virsh.pod index abe80c2..f5457bb 100644 --- a/tools/virsh.pod +++ b/tools/virsh.pod @@ -729,6 +729,22 @@ 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>] + [I<--snoop>]] [I<--lease>] [I<--agent>] + +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. If I<--full> is specified, the interface name is +always displayed when the interfac has multiple addresses or alias, otherwise

s/interfac/interface

+it only displays the interface name for the first address, and "-" for the +others. If I<--lease> is specified, dhcp file is parsed and if I<--agent> is +specified, qemu guest agent is queried. It is not recommended to mix I<--agent> +with I<--lease> as it may return ambiguous results. +

Well technically as the code is written - if --lease and --agent are provided, only --lease is returned John

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

Get network interface stats for a running domain.