[libvirt PATCH 02/14] qemu: agent: split out qemuAgentGetInterfaceOneAddress

[Ján Tomko]

On a Tuesday in 2020, Jonathon Jongsma wrote:
>On Tue,  6 Oct 2020 08:58:38 +0200
>Ján Tomko <jtomko at redhat.com> wrote:
>
>> A function that takes one entry from the "ip-addresses" array
>> returned by "guest-network-get-interfaces" and converts it
>> into virDomainIPAddress.
>>
>> Signed-off-by: Ján Tomko <jtomko at redhat.com>
>> ---
>>  src/qemu/qemu_agent.c | 78
>> +++++++++++++++++++++++++------------------ 1 file changed, 46
>> insertions(+), 32 deletions(-)
>>
>> diff --git a/src/qemu/qemu_agent.c b/src/qemu/qemu_agent.c
>> index 456f0b69e6..fc7b65de2a 100644
>> --- a/src/qemu/qemu_agent.c
>> +++ b/src/qemu/qemu_agent.c
>> @@ -2059,6 +2059,51 @@ qemuAgentGetFSInfo(qemuAgentPtr agent,
>>      return ret;
>>  }
>>
>> +
>> +static int
>> +qemuAgentGetInterfaceOneAddress(virDomainIPAddressPtr ip_addr,
>> +                                virJSONValuePtr ip_addr_obj,
>> +                                const char *name)
>> +{
>> +    const char *type, *addr;
>> +
>> +    type = virJSONValueObjectGetString(ip_addr_obj,
>> "ip-address-type");
>> +    if (!type) {
>> +        virReportError(VIR_ERR_INTERNAL_ERROR,
>> +                       _("qemu agent didn't provide
>> 'ip-address-type'"
>> +                         " field for interface '%s'"), name);
>> +        return -1;
>> +    } else if (STREQ(type, "ipv4")) {
>> +        ip_addr->type = VIR_IP_ADDR_TYPE_IPV4;
>> +    } else if (STREQ(type, "ipv6")) {
>> +        ip_addr->type = VIR_IP_ADDR_TYPE_IPV6;
>> +    } else {
>> +        virReportError(VIR_ERR_INTERNAL_ERROR,
>> +                       _("unknown ip address type '%s'"),
>> +                       type);
>> +        return -1;
>> +    }
>> +
>> +    addr = virJSONValueObjectGetString(ip_addr_obj, "ip-address");
>> +    if (!addr) {
>> +        virReportError(VIR_ERR_INTERNAL_ERROR,
>> +                       _("qemu agent didn't provide 'ip-address'"
>> +                         " field for interface '%s'"), name);
>> +        return -1;
>> +    }
>> +    ip_addr->addr = g_strdup(addr);
>
>(This comment is also true of the existing code, but somehow it feels a
>bit more fragile when it's extracted out to its own function)
>
>If the "prefix" check below fails, we will have allocated memory for
>the address but will return a failure status. That string may leak if
>the calling code does not increment iface->naddrs even on failure.

Just in theory, right?
It seems to be freed correctly with both the original code and after
my refactor.

>Perhaps we should only allocate the string if all sanity checks pass and
>we are guaranteed to return success from this function?
>

I actually thought about doing that, if not for the allocation
consistency, then at least for code readability. But I was tired
of looking at the function(s) already and sent what I had because
I find it strictly better.

Jano

>
>> +
>> +    if (virJSONValueObjectGetNumberUint(ip_addr_obj, "prefix",
>> +                                        &ip_addr->prefix) < 0) {
>> +        virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
>> +                       _("malformed 'prefix' field"));
-------------- 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/4e6dcd0c/attachment-0001.sig>