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