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