On a Tuesday in 2020, Jonathon Jongsma wrote:
On Tue, 6 Oct 2020 08:58:38 +0200 Ján Tomko <jtomko@xxxxxxxxxx> 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@xxxxxxxxxx> --- 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"));
Attachment:
signature.asc
Description: PGP signature
