Re: [Xen-devel] [PATCH v5 2/8] libxl: libxl__device_disk_local_attach return a new libxl_device_disk

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

On Fri, 2012-05-04 at 12:13 +0100, Stefano Stabellini wrote:
> Introduce a new libxl_device_disk** parameter to
> libxl__device_disk_local_attach, the parameter is allocated on the gc by
> libxl__device_disk_local_attach. It is going to fill it with
> informations about the new locally attached disk.  The new
> libxl_device_disk should be passed to libxl_device_disk_local_detach
> afterwards.
> 
> Changes in v5:
> - rename disk to in_disk;
> - rename tmpdisk to disk;
> - copy disk to new_disk only on success;
> - remove check on libxl__zalloc success.
> 
> Signed-off-by: Stefano Stabellini <stefano.stabellini@xxxxxxxxxxxxx>
> ---
>  tools/libxl/libxl_bootloader.c |   10 +++++-----
>  tools/libxl/libxl_internal.c   |   20 ++++++++++++++------
>  tools/libxl/libxl_internal.h   |    3 ++-
>  3 files changed, 21 insertions(+), 12 deletions(-)
> 
> diff --git a/tools/libxl/libxl_bootloader.c b/tools/libxl/libxl_bootloader.c
> index 977c6d3..83cac78 100644
> --- a/tools/libxl/libxl_bootloader.c
> +++ b/tools/libxl/libxl_bootloader.c
> @@ -330,6 +330,7 @@ int libxl_run_bootloader(libxl_ctx *ctx,
>      char *fifo = NULL;
>      char *diskpath = NULL;
>      char **args = NULL;
> +    libxl_device_disk *tmpdisk = NULL;

Do you need to keep tmpdisk valid outside the scope of this function?
You don't seem to in this patch (I didn't look over the next patches
yet).

If not then you could just use "libxl_device_disk tmpdisk" and have
libxl__device_disk_local_attach update in place instead allocating and
passing the pointer back.

>      char tempdir_template[] = "/var/run/libxl/bl.XXXXXX";
>      char *tempdir;
> @@ -386,8 +387,9 @@ int libxl_run_bootloader(libxl_ctx *ctx,
>          goto out_close;
>      }
>  
> -    diskpath = libxl__device_disk_local_attach(gc, disk);
> +    diskpath = libxl__device_disk_local_attach(gc, disk, &tmpdisk);
>      if (!diskpath) {
> +        rc = ERROR_FAIL;
>          goto out_close;
>      }
>  
> @@ -452,10 +454,8 @@ int libxl_run_bootloader(libxl_ctx *ctx,
>  
>      rc = 0;
>  out_close:
> -    if (diskpath) {
> -        libxl__device_disk_local_detach(gc, disk);
> -        free(diskpath);
> -    }
> +    if (tmpdisk)
> +        libxl__device_disk_local_detach(gc, tmpdisk);
>      if (fifo_fd > -1)
>          close(fifo_fd);
>      if (bootloader_fd > -1)
> diff --git a/tools/libxl/libxl_internal.c b/tools/libxl/libxl_internal.c
> index fc771ff..55dc55c 100644
> --- a/tools/libxl/libxl_internal.c
> +++ b/tools/libxl/libxl_internal.c
> @@ -323,12 +323,21 @@ out:
>      return rc;
>  }
>  
> -char * libxl__device_disk_local_attach(libxl__gc *gc, libxl_device_disk 
> *disk)
> +char * libxl__device_disk_local_attach(libxl__gc *gc,
> +        const libxl_device_disk *in_disk,
> +        libxl_device_disk **new_disk)
>  {
>      libxl_ctx *ctx = gc->owner;
>      char *dev = NULL;
> -    char *ret = NULL;
>      int rc;
> +    libxl_device_disk *disk = libxl__zalloc(gc, sizeof(libxl_device_disk));
> +
> +    memcpy(disk, in_disk, sizeof(libxl_device_disk));
> +    if (in_disk->pdev_path != NULL)
> +        disk->pdev_path = libxl__strdup(gc, in_disk->pdev_path);
> +    if (in_disk->script != NULL)
> +        disk->script = libxl__strdup(gc, in_disk->script);
> +    disk->vdev = NULL;
>  
>      rc = libxl__device_disk_setdefault(gc, disk);
>      if (rc) goto out;
> @@ -368,7 +377,7 @@ char * libxl__device_disk_local_attach(libxl__gc *gc, 
> libxl_device_disk *disk)
>                  break;
>              }
>              LIBXL__LOG(ctx, LIBXL__LOG_DEBUG, "locally attaching qdisk %s\n",
> -                       disk->pdev_path);
> +                       in_disk->pdev_path);
>              dev = disk->pdev_path;
>              break;
>          default:
> @@ -378,9 +387,8 @@ char * libxl__device_disk_local_attach(libxl__gc *gc, 
> libxl_device_disk *disk)
>      }
>  
>   out:
> -    if (dev != NULL)
> -        ret = strdup(dev);
> -    return ret;
> +    *new_disk = disk;
> +    return dev;
>  }
>  
>  int libxl__device_disk_local_detach(libxl__gc *gc, libxl_device_disk *disk)
> diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
> index ddd6a37..965d13b 100644
> --- a/tools/libxl/libxl_internal.h
> +++ b/tools/libxl/libxl_internal.h
> @@ -1008,7 +1008,8 @@ _hidden void libxl__device_destroy_tapdisk(libxl__gc 
> *gc, char *be_path);
>   * a device.
>   */
>  _hidden char * libxl__device_disk_local_attach(libxl__gc *gc,
> -        libxl_device_disk *disk);
> +        const libxl_device_disk *in_disk,
> +        libxl_device_disk **new_disk);
>  _hidden int libxl__device_disk_local_detach(libxl__gc *gc,
>          libxl_device_disk *disk);
>  



_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel