Re: [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  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 
---
 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",
+_addr->prefix) < 0) {
+virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+   _("malformed 'prefix' field"));


signature.asc
Description: PGP signature

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

[Jonathon Jongsma]

On Tue,  6 Oct 2020 08:58:38 +0200
Ján Tomko  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 
> ---
>  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",
> +_addr->prefix) < 0) {
> +virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> +   _("malformed 'prefix' field"));
> +return -1;
> +}
> +
> +return 0;
> +}
> +
> +
>  /*
>   * qemuAgentGetInterfaces:
>   * @agent: agent object
> @@ -2176,7 +2221,6 @@ qemuAgentGetInterfaces(qemuAgentPtr agent,
>  addrs_count = iface->naddrs;
>  
>  for (j = 0; j < virJSONValueArraySize(ip_addr_arr); j++) {
> -const char *type, *addr;
>  virJSONValuePtr ip_addr_obj =
> virJSONValueArrayGet(ip_addr_arr, j); virDomainIPAddressPtr ip_addr;
>  
> @@ -2192,38 +2236,8 @@ qemuAgentGetInterfaces(qemuAgentPtr agent,
>  goto error;
>  }
>  
> -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);
> +if (qemuAgentGetInterfaceOneAddress(ip_addr,
> ip_addr_obj, name) < 0) goto error;
> -} 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);
> -goto error;
> -}
> -
> -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);
> -goto error;
> -}
> -ip_addr->addr = g_strdup(addr);
> -
> -if (virJSONValueObjectGetNumberUint(ip_addr_obj,
> "prefix",
> -_addr->prefix) <
> 0) {
> -virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> -   _("malformed 'prefix' field"));
> -goto error;
> -}
>  }
>  
>  iface->naddrs = addrs_count;