Re: [PATCH] libxl: Make an internal function explicitly check existence of expected paths

[George Dunlap <george.dunlap@eu.citrix.com>]

I've sent another one with a "v3" to help make it clear what's going on...

  -George

On 05/12/12 18:28, George Dunlap wrote:
> # HG changeset patch
> # User George Dunlap <george.dunlap@eu.citrix.com>
> # Date 1354732124 0
> # Node ID 21504ec56304ada2f093c30b290ac33c28381ae1
> # Parent  670b07e8d7382229639af0d1df30071e6c1ebb19
> libxl: Make an internal function explicitly check existence of expected paths
>
> libxl__device_disk_from_xs_be() was failing without error for some
> missing xenstore nodes in a backend, while assuming (without checking)
> that other nodes were valid, causing a crash when another internal
> error wrote these nodes in the wrong place.
>
> Make this function consistent by:
> * Checking the existence of all nodes before using
> * Choosing a default only when the node is not written in device_disk_add()
> * Failing with log msg if any node written by device_disk_add() is not present
> * Returning an error on failure
> * Disposing of the structure before returning using libxl_device_disk_displose()
>
> Also make the callers of the function pay attention to the error and
> behave appropriately.  In the case of libxl__append_disk_list_of_type(),
> this means only incrementing *ndisks as the disk structures are
> successfully initialized.
>
> v3:
>   * Make a failure path in libxl__device_disk_from_be() to free allocations
>     from half-initialized disks
>   * Modify libxl__append_disk_list_of_type to only update *ndisks as they are
>     completed.  This will allow the only caller (libxl_device_disk_list()) to
>     clean up the completed disks properly on an error.
> v2:
>   * Remove "Internal error", as the failure will most likely look internal
>   * Use LOG(ERROR...) macros for incrased prettiness
>
> Signed-off-by: George Dunlap <george.dunlap@eu.citrix.com>
>
> diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
> --- a/tools/libxl/libxl.c
> +++ b/tools/libxl/libxl.c
> @@ -2169,9 +2169,9 @@ void libxl__device_disk_add(libxl__egc *
>       device_disk_add(egc, domid, disk, aodev, NULL, NULL);
>   }
>   
> -static void libxl__device_disk_from_xs_be(libxl__gc *gc,
> -                                          const char *be_path,
> -                                          libxl_device_disk *disk)
> +static int libxl__device_disk_from_xs_be(libxl__gc *gc,
> +                                         const char *be_path,
> +                                         libxl_device_disk *disk)
>   {
>       libxl_ctx *ctx = libxl__gc_owner(gc);
>       unsigned int len;
> @@ -2179,6 +2179,7 @@ static void libxl__device_disk_from_xs_b
>   
>       libxl_device_disk_init(disk);
>   
> +    /* "params" may not be present; but everything else must be. */
>       tmp = xs_read(ctx->xsh, XBT_NULL,
>                     libxl__sprintf(gc, "%s/params", be_path), &len);
>       if (tmp && strchr(tmp, ':')) {
> @@ -2187,21 +2188,36 @@ static void libxl__device_disk_from_xs_b
>       } else {
>           disk->pdev_path = tmp;
>       }
> -    libxl_string_to_backend(ctx,
> -                        libxl__xs_read(gc, XBT_NULL,
> -                                       libxl__sprintf(gc, "%s/type", be_path)),
> -                        &(disk->backend));
> +
> +
> +    tmp = libxl__xs_read(gc, XBT_NULL,
> +                         libxl__sprintf(gc, "%s/type", be_path));
> +    if (!tmp) {
> +        LOG(ERROR, "Missing xenstore node %s/type", be_path);
> +        goto cleanup;
> +    }
> +    libxl_string_to_backend(ctx, tmp, &(disk->backend));
> +
>       disk->vdev = xs_read(ctx->xsh, XBT_NULL,
>                            libxl__sprintf(gc, "%s/dev", be_path), &len);
> +    if (!disk->vdev) {
> +        LOG(ERROR, "Missing xenstore node %s/dev", be_path);
> +        goto cleanup;
> +    }
> +
>       tmp = libxl__xs_read(gc, XBT_NULL, libxl__sprintf
>                            (gc, "%s/removable", be_path));
> -
> -    if (tmp)
> -        disk->removable = atoi(tmp);
> -    else
> -        disk->removable = 0;
> +    if (!tmp) {
> +        LOG(ERROR, "Missing xenstore node %s/removable", be_path);
> +        goto cleanup;
> +    }
> +    disk->removable = atoi(tmp);
>   
>       tmp = libxl__xs_read(gc, XBT_NULL, libxl__sprintf(gc, "%s/mode", be_path));
> +    if (!tmp) {
> +        LOG(ERROR, "Missing xenstore node %s/mode", be_path);
> +        goto cleanup;
> +    }
>       if (!strcmp(tmp, "w"))
>           disk->readwrite = 1;
>       else
> @@ -2209,9 +2225,18 @@ static void libxl__device_disk_from_xs_b
>   
>       tmp = libxl__xs_read(gc, XBT_NULL,
>                            libxl__sprintf(gc, "%s/device-type", be_path));
> +    if (!tmp) {
> +        LOG(ERROR, "Missing xenstore node %s/device-type", be_path);
> +        goto cleanup;
> +    }
>       disk->is_cdrom = !strcmp(tmp, "cdrom");
>   
>       disk->format = LIBXL_DISK_FORMAT_UNKNOWN;
> +
> +    return 0;
> +cleanup:
> +    libxl_device_disk_dispose(disk);
> +    return ERROR_FAIL;
>   }
>   
>   int libxl_vdev_to_device_disk(libxl_ctx *ctx, uint32_t domid,
> @@ -2237,9 +2262,7 @@ int libxl_vdev_to_device_disk(libxl_ctx
>       if (!path)
>           goto out;
>   
> -    libxl__device_disk_from_xs_be(gc, path, disk);
> -
> -    rc = 0;
> +    rc = libxl__device_disk_from_xs_be(gc, path, disk);
>   out:
>       GC_FREE;
>       return rc;
> @@ -2256,6 +2279,8 @@ static int libxl__append_disk_list_of_ty
>       char **dir = NULL;
>       unsigned int n = 0;
>       libxl_device_disk *pdisk = NULL, *pdisk_end = NULL;
> +    int rc=0;
> +    int initial_disks = *ndisks;
>   
>       be_path = libxl__sprintf(gc, "%s/backend/%s/%d",
>                                libxl__xs_get_dompath(gc, 0), type, domid);
> @@ -2266,17 +2291,19 @@ static int libxl__append_disk_list_of_ty
>           if (tmp == NULL)
>               return ERROR_NOMEM;
>           *disks = tmp;
> -        pdisk = *disks + *ndisks;
> -        *ndisks += n;
> -        pdisk_end = *disks + *ndisks;
> +        pdisk = *disks + initial_disks;
> +        pdisk_end = *disks + initial_disks + n;
>           for (; pdisk < pdisk_end; pdisk++, dir++) {
>               const char *p;
>               p = libxl__sprintf(gc, "%s/%s", be_path, *dir);
> -            libxl__device_disk_from_xs_be(gc, p, pdisk);
> +            if ((rc=libxl__device_disk_from_xs_be(gc, p, pdisk)))
> +                goto out;
>               pdisk->backend_domid = 0;
> +            *ndisks += 1;
>           }
>       }
> -    return 0;
> +out:
> +    return rc;
>   }
>   
>   libxl_device_disk *libxl_device_disk_list(libxl_ctx *ctx, uint32_t domid, int *num)