[libvirt PATCHv2 00/16] refactor qemuAgentGetInterfaces
v2: * add new patch separating error checking in qemuAgentGetInterfaceOneAddress * split out naddrs++ on a separate line * added documentation for qemuAgentGetInterfaceAddresses * added qemuAgentGetAllInterfaceAddresses Ján Tomko (16): qemu: agent: remove redundant checks qemu: agent: split out qemuAgentGetInterfaceOneAddress qemu: agent: remove impossible errors qemu: agent: reduce scope of addrs_count qemu: agent: expand addrs upfront qemu: agent: use g_auto for ifname qemu: agent: use virHashNew qemu: agent: simplify access to ifaces_ret qemu: agent: split out qemuAgentGetInterfaceAddresses qemu: agent: use GetArray to remove a check qemu: agent: use g_auto in qemuAgentGetInterfaces qemu: agent: remove cleanup in qemuAgentGetInterfaces qemuAgentGetInterfaceAddresses: turn ifname into char* qemu: agent: rename tmp_iface to iface_obj qemuAgentGetInterfaceOneAddress: check for errors early qemu: agent: split out qemuAgentGetAllInterfaceAddresses src/qemu/qemu_agent.c | 355 +++++++++++++++++++++--------------------- 1 file changed, 180 insertions(+), 175 deletions(-) -- 2.26.2
virJSONValueObjectGetArray returns NULL if the object with the supplied key is not an array. Calling virJSONValueIsArray right after is redundant. Signed-off-by: Ján Tomko <jtomko@redhat.com> Reviewed-by: Jonathon Jongsma <jjongsma@redhat.com> --- src/qemu/qemu_agent.c | 12 ------------ 1 file changed, 12 deletions(-) diff --git a/src/qemu/qemu_agent.c b/src/qemu/qemu_agent.c index 09116e749c..456f0b69e6 100644 --- a/src/qemu/qemu_agent.c +++ b/src/qemu/qemu_agent.c @@ -1444,12 +1444,6 @@ qemuAgentGetVCPUs(qemuAgentPtr agent, goto cleanup; } - if (!virJSONValueIsArray(data)) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("Malformed guest-get-vcpus data array")); - goto cleanup; - } - ndata = virJSONValueArraySize(data); *info = g_new0(qemuAgentCPUInfo, ndata); @@ -2314,12 +2308,6 @@ qemuAgentGetUsers(qemuAgentPtr agent, return -1; } - if (!virJSONValueIsArray(data)) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("Malformed guest-get-users data array")); - return -1; - } - ndata = virJSONValueArraySize(data); if (virTypedParamsAddUInt(params, nparams, maxparams, -- 2.26.2
A function that takes one entry from the "ip-addresses" array returned by "guest-network-get-interfaces" and converts it into virDomainIPAddress. Signed-off-by: Ján Tomko <jtomko@redhat.com> --- src/qemu/qemu_agent.c | 78 +++++++++++++++++++++++++------------------ 1 file changed, 46 insertions(+), 32 deletions(-) diff --git a/src/qemu/qemu_agent.c b/src/qemu/qemu_agent.c index 456f0b69e6..fc7b65de2a 100644 --- a/src/qemu/qemu_agent.c +++ b/src/qemu/qemu_agent.c @@ -2059,6 +2059,51 @@ qemuAgentGetFSInfo(qemuAgentPtr agent, return ret; } + +static int +qemuAgentGetInterfaceOneAddress(virDomainIPAddressPtr ip_addr, + virJSONValuePtr ip_addr_obj, + const char *name) +{ + const char *type, *addr; + + 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); + return -1; + } 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); + return -1; + } + + 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); + return -1; + } + ip_addr->addr = g_strdup(addr); + + if (virJSONValueObjectGetNumberUint(ip_addr_obj, "prefix", + &ip_addr->prefix) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("malformed 'prefix' field")); + return -1; + } + + return 0; +} + + /* * qemuAgentGetInterfaces: * @agent: agent object @@ -2176,7 +2221,6 @@ qemuAgentGetInterfaces(qemuAgentPtr agent, addrs_count = iface->naddrs; for (j = 0; j < virJSONValueArraySize(ip_addr_arr); j++) { - const char *type, *addr; virJSONValuePtr ip_addr_obj = virJSONValueArrayGet(ip_addr_arr, j); virDomainIPAddressPtr ip_addr; @@ -2192,38 +2236,8 @@ qemuAgentGetInterfaces(qemuAgentPtr agent, 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); + if (qemuAgentGetInterfaceOneAddress(ip_addr, ip_addr_obj, name) < 0) 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; - } - ip_addr->addr = g_strdup(addr); - - 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; -- 2.26.2
For both 'ip_addr_arr' and 'ret_array', we: 1) already checked that they are arrays 2) only iterate up to the array size Remove the duplicate checks. Signed-off-by: Ján Tomko <jtomko@redhat.com> Reviewed-by: Jonathon Jongsma <jjongsma@redhat.com> --- src/qemu/qemu_agent.c | 14 -------------- 1 file changed, 14 deletions(-) diff --git a/src/qemu/qemu_agent.c b/src/qemu/qemu_agent.c index fc7b65de2a..51c597f57e 100644 --- a/src/qemu/qemu_agent.c +++ b/src/qemu/qemu_agent.c @@ -2160,13 +2160,6 @@ qemuAgentGetInterfaces(qemuAgentPtr agent, const char *hwaddr, *ifname_s, *name = NULL; virDomainInterfacePtr iface = NULL; - /* Shouldn't happen but doesn't hurt to check neither */ - if (!tmp_iface) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("qemu agent reply missing interface entry in array")); - goto error; - } - /* interface name is required to be presented */ name = virJSONValueObjectGetString(tmp_iface, "name"); if (!name) { @@ -2229,13 +2222,6 @@ qemuAgentGetInterfaces(qemuAgentPtr agent, ip_addr = &iface->addrs[addrs_count - 1]; - /* Shouldn't happen but doesn't hurt to check neither */ - if (!ip_addr_obj) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("qemu agent reply missing IP addr in array")); - goto error; - } - if (qemuAgentGetInterfaceOneAddress(ip_addr, ip_addr_obj, name) < 0) goto error; } -- 2.26.2
Signed-off-by: Ján Tomko <jtomko@redhat.com> Reviewed-by: Jonathon Jongsma <jjongsma@redhat.com> --- src/qemu/qemu_agent.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/qemu/qemu_agent.c b/src/qemu/qemu_agent.c index 51c597f57e..c6878c8590 100644 --- a/src/qemu/qemu_agent.c +++ b/src/qemu/qemu_agent.c @@ -2125,7 +2125,6 @@ qemuAgentGetInterfaces(qemuAgentPtr agent, 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; @@ -2159,6 +2158,7 @@ qemuAgentGetInterfaces(qemuAgentPtr agent, virJSONValuePtr ip_addr_arr = NULL; const char *hwaddr, *ifname_s, *name = NULL; virDomainInterfacePtr iface = NULL; + size_t addrs_count = 0; /* interface name is required to be presented */ name = virJSONValueObjectGetString(tmp_iface, "name"); -- 2.26.2
qemuAgentGetInterfaceOneAddress returns exactly one address for every iteration of the loop (and we error out if not). Instead of expanding the addrs by one on every iteration, do it upfront since we know how many times the loop will execute. Signed-off-by: Ján Tomko <jtomko@redhat.com> --- src/qemu/qemu_agent.c | 14 ++++++-------- 1 file changed, 6 insertions(+), 8 deletions(-) diff --git a/src/qemu/qemu_agent.c b/src/qemu/qemu_agent.c index c6878c8590..0394a72518 100644 --- a/src/qemu/qemu_agent.c +++ b/src/qemu/qemu_agent.c @@ -2213,20 +2213,18 @@ qemuAgentGetInterfaces(qemuAgentPtr agent, /* If current iface already exists, continue with the count */ addrs_count = iface->naddrs; + if (VIR_EXPAND_N(iface->addrs, addrs_count, + virJSONValueArraySize(ip_addr_arr)) < 0) + goto error; + for (j = 0; j < virJSONValueArraySize(ip_addr_arr); j++) { virJSONValuePtr ip_addr_obj = virJSONValueArrayGet(ip_addr_arr, j); - virDomainIPAddressPtr ip_addr; - - if (VIR_EXPAND_N(iface->addrs, addrs_count, 1) < 0) - goto error; - - ip_addr = &iface->addrs[addrs_count - 1]; + virDomainIPAddressPtr ip_addr = iface->addrs + iface->naddrs; + iface->naddrs++; if (qemuAgentGetInterfaceOneAddress(ip_addr, ip_addr_obj, name) < 0) goto error; } - - iface->naddrs = addrs_count; } *ifaces = g_steal_pointer(&ifaces_ret); -- 2.26.2
This lets us conveniently reduce its scope to the outer loop. Signed-off-by: Ján Tomko <jtomko@redhat.com> Reviewed-by: Jonathon Jongsma <jjongsma@redhat.com> --- src/qemu/qemu_agent.c | 8 +------- 1 file changed, 1 insertion(+), 7 deletions(-) diff --git a/src/qemu/qemu_agent.c b/src/qemu/qemu_agent.c index 0394a72518..b11f8afde7 100644 --- a/src/qemu/qemu_agent.c +++ b/src/qemu/qemu_agent.c @@ -2127,7 +2127,6 @@ qemuAgentGetInterfaces(qemuAgentPtr agent, size_t ifaces_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))) { @@ -2158,6 +2157,7 @@ qemuAgentGetInterfaces(qemuAgentPtr agent, virJSONValuePtr ip_addr_arr = NULL; const char *hwaddr, *ifname_s, *name = NULL; virDomainInterfacePtr iface = NULL; + g_auto(GStrv) ifname = NULL; size_t addrs_count = 0; /* interface name is required to be presented */ @@ -2194,10 +2194,6 @@ qemuAgentGetInterfaces(qemuAgentPtr agent, iface->hwaddr = g_strdup(hwaddr); } - /* Has to be freed for each interface. */ - g_strfreev(ifname); - ifname = NULL; - /* as well as IP address which - moreover - * can be presented multiple times */ ip_addr_arr = virJSONValueObjectGet(tmp_iface, "ip-addresses"); @@ -2242,8 +2238,6 @@ qemuAgentGetInterfaces(qemuAgentPtr agent, virDomainInterfaceFree(ifaces_ret[i]); } VIR_FREE(ifaces_ret); - g_strfreev(ifname); - goto cleanup; } -- 2.26.2
We're passing 'ifaces_count' to virHashCreate as the initial hash table size just after we've initialized it to zero. This translates to a default of 256 inside virHashCreateFull. Instead of this obfuscation, use virHashNew (default of 32), to make it obvious we don't care about the initial hash size. Also remove the error handling, since neither of the functions return any errors after switching to g_new0. Signed-off-by: Ján Tomko <jtomko@redhat.com> Reviewed-by: Jonathon Jongsma <jjongsma@redhat.com> --- src/qemu/qemu_agent.c | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/src/qemu/qemu_agent.c b/src/qemu/qemu_agent.c index b11f8afde7..a09fb4da2a 100644 --- a/src/qemu/qemu_agent.c +++ b/src/qemu/qemu_agent.c @@ -2129,10 +2129,7 @@ qemuAgentGetInterfaces(qemuAgentPtr agent, virHashTablePtr ifaces_store = NULL; /* Hash table to handle the interface alias */ - if (!(ifaces_store = virHashCreate(ifaces_count, NULL))) { - virHashFree(ifaces_store); - return -1; - } + ifaces_store = virHashNew(NULL); if (!(cmd = qemuAgentMakeCommand("guest-network-get-interfaces", NULL))) goto cleanup; -- 2.26.2
We have a local 'iface' variable that contains the same value eventually. Initialize it early instead of indexing two more times. Signed-off-by: Ján Tomko <jtomko@redhat.com> Reviewed-by: Jonathon Jongsma <jjongsma@redhat.com> --- src/qemu/qemu_agent.c | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/src/qemu/qemu_agent.c b/src/qemu/qemu_agent.c index a09fb4da2a..721ff55fff 100644 --- a/src/qemu/qemu_agent.c +++ b/src/qemu/qemu_agent.c @@ -2176,15 +2176,13 @@ qemuAgentGetInterfaces(qemuAgentPtr agent, if (VIR_EXPAND_N(ifaces_ret, ifaces_count, 1) < 0) goto error; - ifaces_ret[ifaces_count - 1] = g_new0(virDomainInterface, 1); + iface = g_new0(virDomainInterface, 1); + ifaces_ret[ifaces_count - 1] = iface; - if (virHashAddEntry(ifaces_store, ifname_s, - ifaces_ret[ifaces_count - 1]) < 0) + if (virHashAddEntry(ifaces_store, ifname_s, iface) < 0) goto error; - iface = ifaces_ret[ifaces_count - 1]; iface->naddrs = 0; - iface->name = g_strdup(ifname_s); hwaddr = virJSONValueObjectGetString(tmp_iface, "hardware-address"); -- 2.26.2
Convert one interface from the "return" array returned by "guest-network-get-interfaces" to virDomainInterface. Due to the functionality of squashing interface aliases together, this is not a pure function - it either: * Adds the interface to ifaces_ret, incrementing ifaces_count and adds a pointer to it into the ifaces_store hash table. * Adds the additional IP addresses from the interface alias to the existing interface entry, found through the hash table. This does not increment ifaces_count or extend the array. Signed-off-by: Ján Tomko <jtomko@redhat.com> --- src/qemu/qemu_agent.c | 162 +++++++++++++++++++++++++----------------- 1 file changed, 98 insertions(+), 64 deletions(-) diff --git a/src/qemu/qemu_agent.c b/src/qemu/qemu_agent.c index 721ff55fff..e614fdd7c4 100644 --- a/src/qemu/qemu_agent.c +++ b/src/qemu/qemu_agent.c @@ -2104,6 +2104,101 @@ qemuAgentGetInterfaceOneAddress(virDomainIPAddressPtr ip_addr, } +/** + * qemuAgentGetInterfaceAddresses: + * @ifaces_ret: the array to put/update the interface in + * @ifaces_count: the number of interfaces in that array + * @ifaces_store: hash table into @ifaces_ret by interface name + * @tmp_iface: one item from the JSON array of interfaces + * + * This function processes @tmp_iface (which represents + * information about a single interface) and adds the information + * into the ifaces_ret array. + * + * If we're processing an interface alias, the suffix is stripped + * and information is appended to the entry found via the @ifaces_store + * hash table. + * + * Otherwise, the next free position in @ifaces_ret is used, + * its address added to @ifaces_store, and @ifaces_count incremented. + */ +static int +qemuAgentGetInterfaceAddresses(virDomainInterfacePtr **ifaces_ret, + size_t *ifaces_count, + virHashTablePtr ifaces_store, + virJSONValuePtr tmp_iface) +{ + virJSONValuePtr ip_addr_arr = NULL; + const char *hwaddr, *ifname_s, *name = NULL; + virDomainInterfacePtr iface = NULL; + g_auto(GStrv) ifname = NULL; + size_t addrs_count = 0; + size_t j; + + /* 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")); + return -1; + } + + /* 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) + return -1; + + iface = g_new0(virDomainInterface, 1); + (*ifaces_ret)[*ifaces_count - 1] = iface; + + if (virHashAddEntry(ifaces_store, ifname_s, iface) < 0) + return -1; + + iface->naddrs = 0; + iface->name = g_strdup(ifname_s); + + hwaddr = virJSONValueObjectGetString(tmp_iface, "hardware-address"); + iface->hwaddr = g_strdup(hwaddr); + } + + /* as well as IP address which - moreover - + * can be presented multiple times */ + ip_addr_arr = virJSONValueObjectGet(tmp_iface, "ip-addresses"); + if (!ip_addr_arr) + return 0; + + if (!virJSONValueIsArray(ip_addr_arr)) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Malformed ip-addresses array")); + return -1; + } + + /* If current iface already exists, continue with the count */ + addrs_count = iface->naddrs; + + if (VIR_EXPAND_N(iface->addrs, addrs_count, + virJSONValueArraySize(ip_addr_arr)) < 0) + return -1; + + for (j = 0; j < virJSONValueArraySize(ip_addr_arr); j++) { + virJSONValuePtr ip_addr_obj = virJSONValueArrayGet(ip_addr_arr, j); + virDomainIPAddressPtr ip_addr = iface->addrs + iface->naddrs; + iface->naddrs++; + + if (qemuAgentGetInterfaceOneAddress(ip_addr, ip_addr_obj, name) < 0) + return -1; + } + + return 0; +} + + /* * qemuAgentGetInterfaces: * @agent: agent object @@ -2120,7 +2215,7 @@ qemuAgentGetInterfaces(qemuAgentPtr agent, virDomainInterfacePtr **ifaces) { int ret = -1; - size_t i, j; + size_t i; virJSONValuePtr cmd = NULL; virJSONValuePtr reply = NULL; virJSONValuePtr ret_array = NULL; @@ -2151,71 +2246,10 @@ qemuAgentGetInterfaces(qemuAgentPtr agent, for (i = 0; i < virJSONValueArraySize(ret_array); i++) { virJSONValuePtr tmp_iface = virJSONValueArrayGet(ret_array, i); - virJSONValuePtr ip_addr_arr = NULL; - const char *hwaddr, *ifname_s, *name = NULL; - virDomainInterfacePtr iface = NULL; - g_auto(GStrv) ifname = NULL; - size_t addrs_count = 0; - /* 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")); + if (qemuAgentGetInterfaceAddresses(&ifaces_ret, &ifaces_count, + ifaces_store, tmp_iface) < 0) 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; - - iface = g_new0(virDomainInterface, 1); - ifaces_ret[ifaces_count - 1] = iface; - - if (virHashAddEntry(ifaces_store, ifname_s, iface) < 0) - goto error; - - iface->naddrs = 0; - iface->name = g_strdup(ifname_s); - - hwaddr = virJSONValueObjectGetString(tmp_iface, "hardware-address"); - iface->hwaddr = g_strdup(hwaddr); - } - - /* 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 (!virJSONValueIsArray(ip_addr_arr)) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("Malformed ip-addresses array")); - goto error; - } - - /* If current iface already exists, continue with the count */ - addrs_count = iface->naddrs; - - if (VIR_EXPAND_N(iface->addrs, addrs_count, - virJSONValueArraySize(ip_addr_arr)) < 0) - goto error; - - for (j = 0; j < virJSONValueArraySize(ip_addr_arr); j++) { - virJSONValuePtr ip_addr_obj = virJSONValueArrayGet(ip_addr_arr, j); - virDomainIPAddressPtr ip_addr = iface->addrs + iface->naddrs; - iface->naddrs++; - - if (qemuAgentGetInterfaceOneAddress(ip_addr, ip_addr_obj, name) < 0) - goto error; - } } *ifaces = g_steal_pointer(&ifaces_ret); -- 2.26.2
The error check for ValueObjectGet("return") is redundant, qemuAgentCommand already checked for us that the reply contains a "return" object. It does not guarantee, that it is an array. Use virJSONValueObjectGetArray that combines getting the object with checking for its type and return the more helpful of the two error messages. Signed-off-by: Ján Tomko <jtomko@redhat.com> Reviewed-by: Jonathon Jongsma <jjongsma@redhat.com> --- src/qemu/qemu_agent.c | 8 +------- 1 file changed, 1 insertion(+), 7 deletions(-) diff --git a/src/qemu/qemu_agent.c b/src/qemu/qemu_agent.c index e614fdd7c4..e7ea03f840 100644 --- a/src/qemu/qemu_agent.c +++ b/src/qemu/qemu_agent.c @@ -2232,13 +2232,7 @@ qemuAgentGetInterfaces(qemuAgentPtr agent, if (qemuAgentCommand(agent, cmd, &reply, agent->timeout) < 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 (!virJSONValueIsArray(ret_array)) { + if (!(ret_array = virJSONValueObjectGetArray(reply, "return"))) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("qemu agent didn't return an array of interfaces")); goto cleanup; -- 2.26.2
Signed-off-by: Ján Tomko <jtomko@redhat.com> Reviewed-by: Jonathon Jongsma <jjongsma@redhat.com> --- src/qemu/qemu_agent.c | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-) diff --git a/src/qemu/qemu_agent.c b/src/qemu/qemu_agent.c index e7ea03f840..39c955fdf1 100644 --- a/src/qemu/qemu_agent.c +++ b/src/qemu/qemu_agent.c @@ -2216,12 +2216,12 @@ qemuAgentGetInterfaces(qemuAgentPtr agent, { int ret = -1; size_t i; - virJSONValuePtr cmd = NULL; - virJSONValuePtr reply = NULL; + g_autoptr(virJSONValue) cmd = NULL; + g_autoptr(virJSONValue) reply = NULL; virJSONValuePtr ret_array = NULL; size_t ifaces_count = 0; virDomainInterfacePtr *ifaces_ret = NULL; - virHashTablePtr ifaces_store = NULL; + g_autoptr(virHashTable) ifaces_store = NULL; /* Hash table to handle the interface alias */ ifaces_store = virHashNew(NULL); @@ -2250,9 +2250,6 @@ qemuAgentGetInterfaces(qemuAgentPtr agent, ret = ifaces_count; cleanup: - virJSONValueFree(cmd); - virJSONValueFree(reply); - virHashFree(ifaces_store); return ret; error: -- 2.26.2
Signed-off-by: Ján Tomko <jtomko@redhat.com> Reviewed-by: Jonathon Jongsma <jjongsma@redhat.com> --- src/qemu/qemu_agent.c | 14 +++++--------- 1 file changed, 5 insertions(+), 9 deletions(-) diff --git a/src/qemu/qemu_agent.c b/src/qemu/qemu_agent.c index 39c955fdf1..06e143b0b9 100644 --- a/src/qemu/qemu_agent.c +++ b/src/qemu/qemu_agent.c @@ -2214,7 +2214,6 @@ int qemuAgentGetInterfaces(qemuAgentPtr agent, virDomainInterfacePtr **ifaces) { - int ret = -1; size_t i; g_autoptr(virJSONValue) cmd = NULL; g_autoptr(virJSONValue) reply = NULL; @@ -2227,15 +2226,15 @@ qemuAgentGetInterfaces(qemuAgentPtr agent, ifaces_store = virHashNew(NULL); if (!(cmd = qemuAgentMakeCommand("guest-network-get-interfaces", NULL))) - goto cleanup; + return -1; if (qemuAgentCommand(agent, cmd, &reply, agent->timeout) < 0) - goto cleanup; + return -1; if (!(ret_array = virJSONValueObjectGetArray(reply, "return"))) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("qemu agent didn't return an array of interfaces")); - goto cleanup; + return -1; } for (i = 0; i < virJSONValueArraySize(ret_array); i++) { @@ -2247,10 +2246,7 @@ qemuAgentGetInterfaces(qemuAgentPtr agent, } *ifaces = g_steal_pointer(&ifaces_ret); - ret = ifaces_count; - - cleanup: - return ret; + return ifaces_count; error: if (ifaces_ret) { @@ -2258,7 +2254,7 @@ qemuAgentGetInterfaces(qemuAgentPtr agent, virDomainInterfaceFree(ifaces_ret[i]); } VIR_FREE(ifaces_ret); - goto cleanup; + return -1; } -- 2.26.2
We only care about the first part of the 'ifname' string, splitting it is overkill. Instead, just replace the ':' with a '\0' in a copy of the string. This reduces the count of the varaibles containing some form of the interface name to two. Signed-off-by: Ján Tomko <jtomko@redhat.com> Reviewed-by: Jonathon Jongsma <jjongsma@redhat.com> --- src/qemu/qemu_agent.c | 13 ++++++------- 1 file changed, 6 insertions(+), 7 deletions(-) diff --git a/src/qemu/qemu_agent.c b/src/qemu/qemu_agent.c index 06e143b0b9..5cb6257bfc 100644 --- a/src/qemu/qemu_agent.c +++ b/src/qemu/qemu_agent.c @@ -2129,9 +2129,9 @@ qemuAgentGetInterfaceAddresses(virDomainInterfacePtr **ifaces_ret, virJSONValuePtr tmp_iface) { virJSONValuePtr ip_addr_arr = NULL; - const char *hwaddr, *ifname_s, *name = NULL; + const char *hwaddr, *name = NULL; virDomainInterfacePtr iface = NULL; - g_auto(GStrv) ifname = NULL; + g_autofree char *ifname = NULL; size_t addrs_count = 0; size_t j; @@ -2144,10 +2144,9 @@ qemuAgentGetInterfaceAddresses(virDomainInterfacePtr **ifaces_ret, } /* Handle interface alias (<ifname>:<alias>) */ - ifname = virStringSplit(name, ":", 2); - ifname_s = ifname[0]; + ifname = g_strdelimit(g_strdup(name), ":", '\0'); - iface = virHashLookup(ifaces_store, ifname_s); + iface = virHashLookup(ifaces_store, ifname); /* If the hash table doesn't contain this iface, add it */ if (!iface) { @@ -2157,11 +2156,11 @@ qemuAgentGetInterfaceAddresses(virDomainInterfacePtr **ifaces_ret, iface = g_new0(virDomainInterface, 1); (*ifaces_ret)[*ifaces_count - 1] = iface; - if (virHashAddEntry(ifaces_store, ifname_s, iface) < 0) + if (virHashAddEntry(ifaces_store, ifname, iface) < 0) return -1; iface->naddrs = 0; - iface->name = g_strdup(ifname_s); + iface->name = g_strdup(ifname); hwaddr = virJSONValueObjectGetString(tmp_iface, "hardware-address"); iface->hwaddr = g_strdup(hwaddr); -- 2.26.2
Signed-off-by: Ján Tomko <jtomko@redhat.com> Reviewed-by: Jonathon Jongsma <jjongsma@redhat.com> --- src/qemu/qemu_agent.c | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/src/qemu/qemu_agent.c b/src/qemu/qemu_agent.c index 5cb6257bfc..301707b136 100644 --- a/src/qemu/qemu_agent.c +++ b/src/qemu/qemu_agent.c @@ -2109,9 +2109,9 @@ qemuAgentGetInterfaceOneAddress(virDomainIPAddressPtr ip_addr, * @ifaces_ret: the array to put/update the interface in * @ifaces_count: the number of interfaces in that array * @ifaces_store: hash table into @ifaces_ret by interface name - * @tmp_iface: one item from the JSON array of interfaces + * @iface_obj: one item from the JSON array of interfaces * - * This function processes @tmp_iface (which represents + * This function processes @iface_obj (which represents * information about a single interface) and adds the information * into the ifaces_ret array. * @@ -2126,7 +2126,7 @@ static int qemuAgentGetInterfaceAddresses(virDomainInterfacePtr **ifaces_ret, size_t *ifaces_count, virHashTablePtr ifaces_store, - virJSONValuePtr tmp_iface) + virJSONValuePtr iface_obj) { virJSONValuePtr ip_addr_arr = NULL; const char *hwaddr, *name = NULL; @@ -2136,7 +2136,7 @@ qemuAgentGetInterfaceAddresses(virDomainInterfacePtr **ifaces_ret, size_t j; /* interface name is required to be presented */ - name = virJSONValueObjectGetString(tmp_iface, "name"); + name = virJSONValueObjectGetString(iface_obj, "name"); if (!name) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("qemu agent didn't provide 'name' field")); @@ -2162,13 +2162,13 @@ qemuAgentGetInterfaceAddresses(virDomainInterfacePtr **ifaces_ret, iface->naddrs = 0; iface->name = g_strdup(ifname); - hwaddr = virJSONValueObjectGetString(tmp_iface, "hardware-address"); + hwaddr = virJSONValueObjectGetString(iface_obj, "hardware-address"); iface->hwaddr = g_strdup(hwaddr); } /* as well as IP address which - moreover - * can be presented multiple times */ - ip_addr_arr = virJSONValueObjectGet(tmp_iface, "ip-addresses"); + ip_addr_arr = virJSONValueObjectGet(iface_obj, "ip-addresses"); if (!ip_addr_arr) return 0; @@ -2237,10 +2237,10 @@ qemuAgentGetInterfaces(qemuAgentPtr agent, } for (i = 0; i < virJSONValueArraySize(ret_array); i++) { - virJSONValuePtr tmp_iface = virJSONValueArrayGet(ret_array, i); + virJSONValuePtr iface_obj = virJSONValueArrayGet(ret_array, i); if (qemuAgentGetInterfaceAddresses(&ifaces_ret, &ifaces_count, - ifaces_store, tmp_iface) < 0) + ifaces_store, iface_obj) < 0) goto error; } -- 2.26.2
For readability, and to ensure we do allocation when returning 0. Signed-off-by: Ján Tomko <jtomko@redhat.com> --- src/qemu/qemu_agent.c | 15 +++++++++------ 1 file changed, 9 insertions(+), 6 deletions(-) diff --git a/src/qemu/qemu_agent.c b/src/qemu/qemu_agent.c index 301707b136..60247e1616 100644 --- a/src/qemu/qemu_agent.c +++ b/src/qemu/qemu_agent.c @@ -2073,11 +2073,9 @@ qemuAgentGetInterfaceOneAddress(virDomainIPAddressPtr ip_addr, _("qemu agent didn't provide 'ip-address-type'" " field for interface '%s'"), name); return -1; - } 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 { + } + + if (STRNEQ(type, "ipv4") && STRNEQ(type, "ipv6")) { virReportError(VIR_ERR_INTERNAL_ERROR, _("unknown ip address type '%s'"), type); @@ -2091,7 +2089,6 @@ qemuAgentGetInterfaceOneAddress(virDomainIPAddressPtr ip_addr, " field for interface '%s'"), name); return -1; } - ip_addr->addr = g_strdup(addr); if (virJSONValueObjectGetNumberUint(ip_addr_obj, "prefix", &ip_addr->prefix) < 0) { @@ -2100,6 +2097,12 @@ qemuAgentGetInterfaceOneAddress(virDomainIPAddressPtr ip_addr, return -1; } + if (STREQ(type, "ipv4")) + ip_addr->type = VIR_IP_ADDR_TYPE_IPV4; + else + ip_addr->type = VIR_IP_ADDR_TYPE_IPV6; + + ip_addr->addr = g_strdup(addr); return 0; } -- 2.26.2
Remove more logic from qemuAgentGetInterfaces. Signed-off-by: Ján Tomko <jtomko@redhat.com> --- src/qemu/qemu_agent.c | 57 ++++++++++++++++++++++++------------------- 1 file changed, 32 insertions(+), 25 deletions(-) diff --git a/src/qemu/qemu_agent.c b/src/qemu/qemu_agent.c index 60247e1616..c9c4b034d3 100644 --- a/src/qemu/qemu_agent.c +++ b/src/qemu/qemu_agent.c @@ -2201,6 +2201,37 @@ qemuAgentGetInterfaceAddresses(virDomainInterfacePtr **ifaces_ret, } +static int +qemuAgentGetAllInterfaceAddresses(virDomainInterfacePtr **ifaces_ret, + virJSONValuePtr ret_array) +{ + g_autoptr(virHashTable) ifaces_store = NULL; + size_t ifaces_count = 0; + size_t i; + + /* Hash table to handle the interface alias */ + ifaces_store = virHashNew(NULL); + + for (i = 0; i < virJSONValueArraySize(ret_array); i++) { + virJSONValuePtr iface_obj = virJSONValueArrayGet(ret_array, i); + + if (qemuAgentGetInterfaceAddresses(ifaces_ret, &ifaces_count, + ifaces_store, iface_obj) < 0) + goto error; + } + + return ifaces_count; + + error: + if (ifaces_ret) { + for (i = 0; i < ifaces_count; i++) + virDomainInterfaceFree(*ifaces_ret[i]); + } + VIR_FREE(*ifaces_ret); + return -1; +} + + /* * qemuAgentGetInterfaces: * @agent: agent object @@ -2216,16 +2247,9 @@ int qemuAgentGetInterfaces(qemuAgentPtr agent, virDomainInterfacePtr **ifaces) { - size_t i; g_autoptr(virJSONValue) cmd = NULL; g_autoptr(virJSONValue) reply = NULL; virJSONValuePtr ret_array = NULL; - size_t ifaces_count = 0; - virDomainInterfacePtr *ifaces_ret = NULL; - g_autoptr(virHashTable) ifaces_store = NULL; - - /* Hash table to handle the interface alias */ - ifaces_store = virHashNew(NULL); if (!(cmd = qemuAgentMakeCommand("guest-network-get-interfaces", NULL))) return -1; @@ -2239,24 +2263,7 @@ qemuAgentGetInterfaces(qemuAgentPtr agent, return -1; } - for (i = 0; i < virJSONValueArraySize(ret_array); i++) { - virJSONValuePtr iface_obj = virJSONValueArrayGet(ret_array, i); - - if (qemuAgentGetInterfaceAddresses(&ifaces_ret, &ifaces_count, - ifaces_store, iface_obj) < 0) - goto error; - } - - *ifaces = g_steal_pointer(&ifaces_ret); - return ifaces_count; - - error: - if (ifaces_ret) { - for (i = 0; i < ifaces_count; i++) - virDomainInterfaceFree(ifaces_ret[i]); - } - VIR_FREE(ifaces_ret); - return -1; + return qemuAgentGetAllInterfaceAddresses(ifaces, ret_array); } -- 2.26.2
On Wed, 7 Oct 2020 14:35:21 +0200 Ján Tomko <jtomko@redhat.com> wrote:
v2: * add new patch separating error checking in qemuAgentGetInterfaceOneAddress * split out naddrs++ on a separate line * added documentation for qemuAgentGetInterfaceAddresses * added qemuAgentGetAllInterfaceAddresses
Ján Tomko (16): qemu: agent: remove redundant checks qemu: agent: split out qemuAgentGetInterfaceOneAddress qemu: agent: remove impossible errors qemu: agent: reduce scope of addrs_count qemu: agent: expand addrs upfront qemu: agent: use g_auto for ifname qemu: agent: use virHashNew qemu: agent: simplify access to ifaces_ret qemu: agent: split out qemuAgentGetInterfaceAddresses qemu: agent: use GetArray to remove a check qemu: agent: use g_auto in qemuAgentGetInterfaces qemu: agent: remove cleanup in qemuAgentGetInterfaces qemuAgentGetInterfaceAddresses: turn ifname into char* qemu: agent: rename tmp_iface to iface_obj qemuAgentGetInterfaceOneAddress: check for errors early qemu: agent: split out qemuAgentGetAllInterfaceAddresses
src/qemu/qemu_agent.c | 355 +++++++++++++++++++++--------------------- 1 file changed, 180 insertions(+), 175 deletions(-)
Looks good to me. Reviewed-by: Jonathon Jongsma <jjongsma@redhat.com>
On Wed, Oct 7, 2020 at 8:35 AM Ján Tomko <jtomko@redhat.com> wrote:
v2: * add new patch separating error checking in qemuAgentGetInterfaceOneAddress * split out naddrs++ on a separate line * added documentation for qemuAgentGetInterfaceAddresses * added qemuAgentGetAllInterfaceAddresses
Ján Tomko (16): qemu: agent: remove redundant checks qemu: agent: split out qemuAgentGetInterfaceOneAddress qemu: agent: remove impossible errors qemu: agent: reduce scope of addrs_count qemu: agent: expand addrs upfront qemu: agent: use g_auto for ifname qemu: agent: use virHashNew qemu: agent: simplify access to ifaces_ret qemu: agent: split out qemuAgentGetInterfaceAddresses qemu: agent: use GetArray to remove a check qemu: agent: use g_auto in qemuAgentGetInterfaces qemu: agent: remove cleanup in qemuAgentGetInterfaces qemuAgentGetInterfaceAddresses: turn ifname into char* qemu: agent: rename tmp_iface to iface_obj qemuAgentGetInterfaceOneAddress: check for errors early qemu: agent: split out qemuAgentGetAllInterfaceAddresses
src/qemu/qemu_agent.c | 355 +++++++++++++++++++++--------------------- 1 file changed, 180 insertions(+), 175 deletions(-)
-- 2.26.2
Series LGTM. Reviewed-by: Neal Gompa <ngompa13@gmail.com> -- 真実はいつも一つ！/ Always, there's only one truth!
participants (3)
-
Jonathon Jongsma -
Ján Tomko -
Neal Gompa