On a Tuesday in 2020, Jonathon Jongsma wrote:
On Tue, 6 Oct 2020 08:58:41 +0200 Ján Tomko <jtomko@xxxxxxxxxx> 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@xxxxxxxxxx> --- 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);
Attachment:
signature.asc
Description: PGP signature
