From mboxrd@z Thu Jan 1 00:00:00 1970 From: Ian Campbell Subject: Re: [PATCH 1/6] libxl: libxl_device_disk_local_attach return a new libxl_device_disk Date: Tue, 27 Mar 2012 15:07:32 +0100 Message-ID: <1332857252.25560.27.camel@zakaz.uk.xensource.com> References: <1332856772-30292-1-git-send-email-stefano.stabellini@eu.citrix.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <1332856772-30292-1-git-send-email-stefano.stabellini@eu.citrix.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Stefano Stabellini Cc: "xen-devel@lists.xensource.com" , Ian Jackson List-Id: xen-devel@lists.xenproject.org On Tue, 2012-03-27 at 14:59 +0100, Stefano Stabellini wrote: > The caller passes an additional pre-allocated libxl_device_disk struct > to libxl_device_disk_local_attach, that 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. > > This patch is just about adding the new parameter and updating the > caller. > > Signed-off-by: Stefano Stabellini > --- > tools/libxl/libxl.c | 46 ++++++++++++++++++++++++++-------------- > tools/libxl/libxl.h | 3 +- > tools/libxl/libxl_bootloader.c | 9 +++++-- > 3 files changed, 38 insertions(+), 20 deletions(-) > > diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c > index 5344366..d33fbdf 100644 > --- a/tools/libxl/libxl.c > +++ b/tools/libxl/libxl.c > @@ -1644,63 +1644,77 @@ out: > return ret; > } > > -char * libxl_device_disk_local_attach(libxl_ctx *ctx, libxl_device_disk *disk) > +char * libxl_device_disk_local_attach(libxl_ctx *ctx, const libxl_device_disk *disk, > + libxl_device_disk **new_disk) I think this would be better as a caller allocated libxl_device_disk *. That's much simpler since the caller can just put it on the stack or in an existing datastructure (I expect a suitable one comes into being with IanJ's async patch series). Do we need this as a public facing API? Does anything use it? I think it should be an implementation detail of libxl_device_disk_add where domid == SELF? That would also allow you to use the gc here. > { > GC_INIT(ctx); > char *dev = NULL; > char *ret = NULL; > int rc; > - > - rc = libxl__device_disk_setdefault(gc, disk); > + libxl_device_disk *tmpdisk = (libxl_device_disk *) > + malloc(sizeof(libxl_device_disk)); > + if (tmpdisk == NULL) goto out; > + > + *new_disk = tmpdisk; > + memcpy(tmpdisk, disk, sizeof(libxl_device_disk)); > + if (tmpdisk->pdev_path != NULL) > + tmpdisk->pdev_path = strdup(tmpdisk->pdev_path); > + if (tmpdisk->script != NULL) > + tmpdisk->script = strdup(tmpdisk->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) > + if (dev != NULL) { > ret = strdup(dev); > + tmpdisk->vdev = strdup(tmpdisk->vdev); > + } > GC_FREE; > return ret; > } > diff --git a/tools/libxl/libxl.h b/tools/libxl/libxl.h > index 6b69030..d297b04 100644 > --- a/tools/libxl/libxl.h > +++ b/tools/libxl/libxl.h > @@ -540,7 +540,8 @@ int libxl_cdrom_insert(libxl_ctx *ctx, uint32_t domid, libxl_device_disk *disk); > * Make a disk available in this (the control) domain. Returns path to > * a device. > */ > -char * libxl_device_disk_local_attach(libxl_ctx *ctx, libxl_device_disk *disk); > +char * libxl_device_disk_local_attach(libxl_ctx *ctx, const libxl_device_disk *disk, > + libxl_device_disk **new_disk); > int libxl_device_disk_local_detach(libxl_ctx *ctx, libxl_device_disk *disk); > > /* Network Interfaces */ > diff --git a/tools/libxl/libxl_bootloader.c b/tools/libxl/libxl_bootloader.c > index 2774062..0abc31f 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,7 +387,7 @@ int libxl_run_bootloader(libxl_ctx *ctx, > goto out_close; > } > > - diskpath = libxl_device_disk_local_attach(ctx, disk); > + diskpath = libxl_device_disk_local_attach(ctx, disk, &tmpdisk); > if (!diskpath) { > goto out_close; > } > @@ -452,9 +453,11 @@ int libxl_run_bootloader(libxl_ctx *ctx, > > rc = 0; > out_close: > - if (diskpath) { > - libxl_device_disk_local_detach(ctx, disk); > + if (diskpath) > free(diskpath); > + if (tmpdisk) { > + libxl_device_disk_local_detach(ctx, tmpdisk); > + free(tmpdisk); > } > if (fifo_fd > -1) > close(fifo_fd);