On Tue, 6 Oct 2020 08:58:38 +0200 Ján Tomko <jtomko@redhat.com> wrote:

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

(This comment is also true of the existing code, but somehow it feels a bit more fragile when it's extracted out to its own function) If the "prefix" check below fails, we will have allocated memory for the address but will return a failure status. That string may leak if the calling code does not increment iface->naddrs even on failure. Perhaps we should only allocate the string if all sanity checks pass and we are guaranteed to return success from this function?

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

On a Tuesday in 2020, Jonathon Jongsma wrote:

On Tue, 6 Oct 2020 08:58:41 +0200 Ján Tomko <jtomko@redhat.com> wrote:

...

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 | 13 +++++-------- 1 file changed, 5 insertions(+), 8 deletions(-)

diff --git a/src/qemu/qemu_agent.c b/src/qemu/qemu_agent.c index c6878c8590..dc989622b9 100644 --- a/src/qemu/qemu_agent.c +++ b/src/qemu/qemu_agent.c @@ -2213,20 +2213,17 @@ 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; +

I don't see much benefit to keeping the local "addrs_count" variable here. Previously this local var was incremented within the loop and then used to set iface->naddrs after all iterations of the loop were completed. But you're no longer doing anything with it other than setting it right here. The iface->naddrs value is incremented separately within the loop. It should be safe to just set iface->naddrs here since any newly allocated elements are guaranteed to be filled with zeros.

VIR_EXPAND_N expects size_t, iface->naddrs is unsigned int. Setting iface->naddrs to the end value here would be OK from the caller's PoV, but I would still need a separate variable to know the old iface->naddrs value.

...

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

Then you could just use 'iface->addrs + j' here instead of incrementing the naddrs variable (which IMO reduces the readability of the code).

j starts at 0, not at the original iface->naddrs. I can split out the increment onto a separate line.

...

if (qemuAgentGetInterfaceOneAddress(ip_addr, ip_addr_obj, name) < 0) goto error; } - - iface->naddrs = addrs_count; }

*ifaces = g_steal_pointer(&ifaces_ret);

On a Tuesday in 2020, Jonathon Jongsma wrote:

On Tue, 6 Oct 2020 08:58:45 +0200 Ján Tomko <jtomko@redhat.com> wrote:

...

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.

It seems to me that if we were to factor out this function, this documentation would belong within the code rather than just in the commit message. Without it, it's not very obvious what this function does with its arguments. [1]

The fact that it needs this documentation to be understood and it is only called from a single place makes me think that it probably isn't worthwhile to factor it out into a separate function.

It is called from one place because it does one very specific thing. It needs documentation because the code is (still unnecessarily) complicated. If we were to leave the code as-is, it would still need that documentation.

I think it actually makes the code a bit harder to understand.

I see it the other way - even if it changes three out of four of its arguments, it lets me know that it cannot change others. Arguably, qemuAgentGetInterfaceAddresses is not as descriptive as it could be.

[1] I was going to suggest simply removing the 'ifaces_ret' and 'ifaces_count' arguments from this function and doing all of the processing with just the hash table for storage. Then the calling function could construct the return array from the hash table after everything is processed. Unfortunately this won't necessarily maintain the ordering of the returned interfaces. I don't know if the order is important to maintain or not...

Yes, this can be improved further by giving up on the order, or adding a second (even a third pass) to collect them from the hash table in order, or use an array and collapse the entries afterwards. I specifically wanted to preserve the existing behavior here to improve the readability without breaking compatibility (or speed? how many addresses does the guest need to have to make the hash table worth it?) Jano

...

Signed-off-by: Ján Tomko <jtomko@redhat.com> --- src/qemu/qemu_agent.c | 141 +++++++++++++++++++++++------------------- 1 file changed, 79 insertions(+), 62 deletions(-)

diff --git a/src/qemu/qemu_agent.c b/src/qemu/qemu_agent.c index 7a4f5cd6db..b314c610e7 100644 --- a/src/qemu/qemu_agent.c +++ b/src/qemu/qemu_agent.c @@ -2104,6 +2104,82 @@ qemuAgentGetInterfaceOneAddress(virDomainIPAddressPtr ip_addr, }

+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++; + + if (qemuAgentGetInterfaceOneAddress(ip_addr, ip_addr_obj, name) < 0) + return -1; + } + + return 0; +} + + /* * qemuAgentGetInterfaces: * @agent: agent object @@ -2120,7 +2196,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,70 +2227,11 @@ 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++; - - if (qemuAgentGetInterfaceOneAddress(ip_addr, ip_addr_obj, name) < 0) - goto error; - } }

*ifaces = g_steal_pointer(&ifaces_ret);