Re: [libvirt] [PATCHv5 3/5] domifaddr: Implement the API for qemu

On Tue, Sep 3, 2013 at 8:21 PM, Osier Yang <jyang(a)redhat.com&gt; wrote: > On 03/09/13 22:37, Nehal J Wani wrote: >> On Tue, Sep 3, 2013 at 7:46 PM, Osier Yang <jyang(a)redhat.com&gt; 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];