|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH v4 2/8] libxl: libxl__device_disk_local_attach return a new libxl_device_disk
On Tue, 2012-04-24 at 11:45 +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.
>
> Signed-off-by: Stefano Stabellini <stefano.stabellini@xxxxxxxxxxxxx>
> ---
> tools/libxl/libxl_bootloader.c | 10 ++++----
> tools/libxl/libxl_internal.c | 47 +++++++++++++++++++++++----------------
> tools/libxl/libxl_internal.h | 3 +-
> 3 files changed, 35 insertions(+), 25 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;
>
> 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 1f428b2..3af77e7 100644
> --- a/tools/libxl/libxl_internal.c
> +++ b/tools/libxl/libxl_internal.c
> @@ -323,64 +323,73 @@ out:
> return rc;
> }
>
> -_hidden char * libxl__device_disk_local_attach(libxl__gc *gc,
> libxl_device_disk *disk)
> +_hidden char * libxl__device_disk_local_attach(libxl__gc *gc,
> + const libxl_device_disk *disk,
> + libxl_device_disk **new_disk)
> {
> libxl_ctx *ctx = gc->owner;
> char *dev = NULL;
> - char *ret = NULL;
> int rc;
> -
> - rc = libxl__device_disk_setdefault(gc, disk);
> + libxl_device_disk *tmpdisk = libxl__zalloc(gc,
> sizeof(libxl_device_disk));
> + if (tmpdisk == NULL) goto out;
> +
> + *new_disk = tmpdisk;
It would be good practice to not do this until we have succeeded, or do
you expect the caller to clean this up on error, even if the disk is
half constructed? I'd suggest that it ought be up to this function to
unwind on failure not the called.
tmpdisk isn't actually temporary, it's actually the result of this
function and lives for a fair while afterwards, perhaps loopdisk? or
dom0disk? or was this the patch where IanJ suggested renaming the param
disk (e.g. to guest_disk) and then using disk for this one? (That's
actually a good idea)
> + memcpy(tmpdisk, disk, sizeof(libxl_device_disk));
You don't actually need zalloc if you immediately memcpy over the whole
thing. Minor thing though.
> + if (disk->pdev_path != NULL)
> + tmpdisk->pdev_path = libxl__strdup(gc, disk->pdev_path);
> + if (disk->script != NULL)
> + tmpdisk->script = libxl__strdup(gc, disk->script);
> + tmpdisk->vdev = NULL;
> +
> + rc = libxl__device_disk_setdefault(gc, tmpdisk);
> if (rc) goto out;
>
> - switch (disk->backend) {
> + switch (tmpdisk->backend) {
> case LIBXL_DISK_BACKEND_PHY:
> LIBXL__LOG(ctx, LIBXL__LOG_DEBUG, "locally attaching PHY disk
> %s",
> - disk->pdev_path);
> - dev = disk->pdev_path;
> + tmpdisk->pdev_path);
> + dev = tmpdisk->pdev_path;
> break;
> case LIBXL_DISK_BACKEND_TAP:
> - switch (disk->format) {
> + switch (tmpdisk->format) {
> case LIBXL_DISK_FORMAT_RAW:
> /* optimise away the early tapdisk attach in this case */
> LIBXL__LOG(ctx, LIBXL__LOG_DEBUG, "locally attaching"
> " tap disk %s directly (ie without using blktap)",
> - disk->pdev_path);
> - dev = disk->pdev_path;
> + tmpdisk->pdev_path);
> + dev = tmpdisk->pdev_path;
> break;
> case LIBXL_DISK_FORMAT_VHD:
> - dev = libxl__blktap_devpath(gc, disk->pdev_path,
> - disk->format);
> + dev = libxl__blktap_devpath(gc, tmpdisk->pdev_path,
> + tmpdisk->format);
> break;
> case LIBXL_DISK_FORMAT_QCOW:
> case LIBXL_DISK_FORMAT_QCOW2:
> abort(); /* prevented by libxl__device_disk_set_backend */
> default:
> LIBXL__LOG(ctx, LIBXL__LOG_ERROR,
> - "unrecognized disk format: %d", disk->format);
> + "unrecognized disk format: %d", tmpdisk->format);
> break;
> }
> break;
> case LIBXL_DISK_BACKEND_QDISK:
> - if (disk->format != LIBXL_DISK_FORMAT_RAW) {
> + if (tmpdisk->format != LIBXL_DISK_FORMAT_RAW) {
> LIBXL__LOG(ctx, LIBXL__LOG_ERROR, "cannot locally"
> " attach a qdisk image if the format is not raw");
> break;
> }
> LIBXL__LOG(ctx, LIBXL__LOG_DEBUG, "locally attaching qdisk %s\n",
> disk->pdev_path);
> - dev = disk->pdev_path;
> + dev = tmpdisk->pdev_path;
> break;
> default:
> LIBXL__LOG(ctx, LIBXL__LOG_ERROR, "unrecognized disk backend "
> - "type: %d", disk->backend);
> + "type: %d", tmpdisk->backend);
> break;
> }
>
> out:
> - if (dev != NULL)
> - ret = strdup(dev);
> - return ret;
> + return dev;
> }
>
> _hidden 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 6927aef..2f1cf0f 100644
> --- a/tools/libxl/libxl_internal.h
> +++ b/tools/libxl/libxl_internal.h
> @@ -1006,7 +1006,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 *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
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |