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

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

On 26/11/13 12:09, Ian Jackson wrote:
> Andrew Cooper writes ("Re: [Xen-devel] [Patch v3 3/4] tools/libxl: Fix libxl__device_nic_from_xs_be()"):
>> As this function cant fail, I was trying to force all error paths to
>> apply safe defaults to the libxl_device_nic structure.
> Perhaps the function should be able to fail.
>
> From 3cea493c97f23eeb8e175915186f7ca2701da60a 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
>
> This requires changing its return type and fixing the callers.
>
> Introduce here a READ_BACKEND macro to make the code less repetitive.
>
> Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>

Commit message should include the Coverity ID 1055886, and perhaps a
reference to the fact that it is a memory leak.

> ---
>  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..62ff6db 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(subpath) ({                                        \
> +        rc = libxl__xs_read_checked(gc, XBT_NULL,                       \
> +                                    GCSPRINTF("%s/" subpath, be_path),  \
> +                                    &tmp);                              \
> +        if (rc) goto out;                                               \
> +        (char*)tmp;                                                     \
> +    });
> +
> +    tmp = READ_BACKEND("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("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("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.

~Andrew

>  
>      /* 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)