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