Re: [libvirt PATCH 05/14] qemu: agent: expand addrs upfront
On Tue, 6 Oct 2020 08:58:41 +0200
Ján Tomko <jtomko(a)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(a)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.
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).
if (qemuAgentGetInterfaceOneAddress(ip_addr,
ip_addr_obj, name) < 0) goto error;
}
-
- iface->naddrs = addrs_count;
}
*ifaces = g_steal_pointer(&ifaces_ret);