[libvirt PATCH 05/14] qemu: agent: expand addrs upfront

[Ján Tomko]

On a Tuesday in 2020, Jonathon Jongsma wrote:
>On Tue,  6 Oct 2020 08:58:41 +0200
>Ján Tomko <jtomko at 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 at 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);
>
>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 488 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20201007/7e793040/attachment-0001.sig>