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

On Tue, 6 Oct 2020 08:58:38 +0200 Ján Tomko <jtomko(a)redhat.com&gt; 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(a)redhat.com&gt; > --- > 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.

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

> + > + if (virJSONValueObjectGetNumberUint(ip_addr_obj, "prefix", > + &ip_addr->prefix) < 0) { > + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", > + _("malformed 'prefix' field"));