Re: [Patch v3 3/4] tools/libxl: Fix libxl__device_nic_from_xs_be()

[Andrew Cooper <andrew.cooper3@citrix.com>]

On 26/11/13 15:08, Ian Jackson wrote:
> Andrew Cooper writes ("Re: [Xen-devel] [Patch v3 3/4] tools/libxl: Fix libxl__device_nic_from_xs_be()"):
>> Commit message should include the Coverity ID 1055886, and perhaps a
>> reference to the fact that it is a memory leak.
> ...
>>> +    nic->ip = READ_BACKEND("ip");
>>> +    nic->bridge = READ_BACKEND("bridge");
>>> +    nic->script = READ_BACKEND("script");
>> This is not correct.  libxl_device_nic_dispose() is in charge of freeing
>> these pointers, but now they are part of the gc.
> Oops.
>
> From 5bdaef8453bc8fd06da956df90fe33a12190342c Mon Sep 17 00:00:00 2001
> From: Ian Jackson <ian.jackson@eu.citrix.com>
> Date: Tue, 26 Nov 2013 12:08:09 +0000
> Subject: [PATCH] libxl: Fix error handling in libxl__device_nic_from_xs_be
>
> Previously, this function had a memory leak.  Fix this, and the rest
> of the error handling.
>
> This requires changing its return type and fixing the callers.
>
> Introduce here a READ_BACKEND macro to make the code less repetitive.
>
> Coverity ID: 1055886
>
> Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>

This looks like it might plausibly work.

Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>

>
> ---
> v2: Strings nic-> allocated from NOGC, not the gc.
>     Improve commit message.
> ---
>  tools/libxl/libxl.c |   62 ++++++++++++++++++++++++++++++---------------------
>  1 file changed, 37 insertions(+), 25 deletions(-)
>
> diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
> index 2b847ef..ae1b7aa 100644
> --- a/tools/libxl/libxl.c
> +++ b/tools/libxl/libxl.c
> @@ -2976,45 +2976,51 @@ out:
>      return;
>  }
>  
> -static void libxl__device_nic_from_xs_be(libxl__gc *gc,
> -                                         const char *be_path,
> -                                         libxl_device_nic *nic)
> +static int libxl__device_nic_from_xs_be(libxl__gc *gc,
> +                                        const char *be_path,
> +                                        libxl_device_nic *nic)
>  {
> -    libxl_ctx *ctx = libxl__gc_owner(gc);
> -    unsigned int len;
> -    char *tmp;
> +    const char *tmp;
>      int rc;
>  
>      libxl_device_nic_init(nic);
>  
> -    tmp = xs_read(ctx->xsh, XBT_NULL,
> -                  libxl__sprintf(gc, "%s/handle", be_path), &len);
> -    if ( tmp )
> +#define READ_BACKEND(tgc, subpath) ({                                   \
> +        rc = libxl__xs_read_checked(tgc, XBT_NULL,                      \
> +                                    GCSPRINTF("%s/" subpath, be_path),  \
> +                                    &tmp);                              \
> +        if (rc) goto out;                                               \
> +        (char*)tmp;                                                     \
> +    });
> +
> +    tmp = READ_BACKEND(gc, "handle");
> +    if (tmp)
>          nic->devid = atoi(tmp);
>      else
>          nic->devid = 0;
>  
>      /* nic->mtu = */
>  
> -    tmp = xs_read(ctx->xsh, XBT_NULL,
> -                  libxl__sprintf(gc, "%s/mac", be_path), &len);
> -    rc = libxl__parse_mac(tmp, nic->mac);
> -    if (rc)
> +    tmp = READ_BACKEND(gc, "mac");
> +    if (tmp) {
> +        rc = libxl__parse_mac(tmp, nic->mac);
> +        if (rc) goto out;
> +    } else {
>          memset(nic->mac, 0, sizeof(nic->mac));
> +    }
>  
> -    nic->ip = xs_read(ctx->xsh, XBT_NULL,
> -                      libxl__sprintf(gc, "%s/ip", be_path), &len);
> -
> -    nic->bridge = xs_read(ctx->xsh, XBT_NULL,
> -                      libxl__sprintf(gc, "%s/bridge", be_path), &len);
> -
> -    nic->script = xs_read(ctx->xsh, XBT_NULL,
> -                      libxl__sprintf(gc, "%s/script", be_path), &len);
> +    nic->ip = READ_BACKEND(NOGC, "ip");
> +    nic->bridge = READ_BACKEND(NOGC, "bridge");
> +    nic->script = READ_BACKEND(NOGC, "script");
>  
>      /* vif_ioemu nics use the same xenstore entries as vif interfaces */
>      nic->nictype = LIBXL_NIC_TYPE_VIF;
>      nic->model = NULL; /* XXX Only for TYPE_IOEMU */
>      nic->ifname = NULL; /* XXX Only for TYPE_IOEMU */
> +
> +    rc = 0;
> + out:
> +    return rc;
>  }
>  
>  int libxl_devid_to_device_nic(libxl_ctx *ctx, uint32_t domid,
> @@ -3035,7 +3041,8 @@ int libxl_devid_to_device_nic(libxl_ctx *ctx, uint32_t domid,
>      if (!path)
>          goto out;
>  
> -    libxl__device_nic_from_xs_be(gc, path, nic);
> +    rc = libxl__device_nic_from_xs_be(gc, path, nic);
> +    if (rc) goto out;
>  
>      rc = 0;
>  out:
> @@ -3053,6 +3060,7 @@ static int libxl__append_nic_list_of_type(libxl__gc *gc,
>      char **dir = NULL;
>      unsigned int n = 0;
>      libxl_device_nic *pnic = NULL, *pnic_end = NULL;
> +    int rc;
>  
>      be_path = libxl__sprintf(gc, "%s/backend/%s/%d",
>                               libxl__xs_get_dompath(gc, 0), type, domid);
> @@ -3064,16 +3072,20 @@ static int libxl__append_nic_list_of_type(libxl__gc *gc,
>              return ERROR_NOMEM;
>          *nics = tmp;
>          pnic = *nics + *nnics;
> -        *nnics += n;
> -        pnic_end = *nics + *nnics;
> +        pnic_end = *nics + *nnics + n;
>          for (; pnic < pnic_end; pnic++, dir++) {
>              const char *p;
>              p = libxl__sprintf(gc, "%s/%s", be_path, *dir);
> -            libxl__device_nic_from_xs_be(gc, p, pnic);
> +            rc = libxl__device_nic_from_xs_be(gc, p, pnic);
> +            if (rc) goto out;
>              pnic->backend_domid = 0;
>          }
> +        *nnics += n;
>      }
>      return 0;
> +
> + out:
> +    return rc;
>  }
>  
>  libxl_device_nic *libxl_device_nic_list(libxl_ctx *ctx, uint32_t domid, int *num)