Re: [Xen-devel] [PATCH v8 02/11] libxl: libxl__device_disk_local_attach return a new libxl_device_disk

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

On Tue, 2012-05-29 at 15:48 +0100, Stefano Stabellini wrote:
> On Tue, 29 May 2012, Stefano Stabellini wrote:
> > On Tue, 29 May 2012, Ian Campbell wrote:
> > > On Tue, 2012-05-29 at 11:39 +0100, Stefano Stabellini wrote:
> > > > @@ -1804,6 +1814,8 @@ int libxl__device_disk_local_detach(libxl__gc 
> > > > *gc, libxl_device_disk *disk)
> > > >       * needed by the soon to be started domain and do nothing.
> > > >       */
> > > >  
> > > > +    free(disk->pdev_path);
> > > > +    free(disk->script);
> > > 
> > > This is open coding libxl_device_disk_dispose(disk) but missed the vdev
> > > member, is that deliberate?
> > 
> > I think it is a mistake: all these strings used to be allocated on the
> > gc, on previous versions of the series. However meanwhile the event
> > series went in, changing completely libxl__bootloader_run (that is the
> > caller of libxl__device_disk_local_attach).
> > Allocating stuff on the gc is not correct anymore, so now they need to
> > be explicitly freed. I think I should call libxl_device_disk_dispose
> > here and strdup in libxl__device_disk_local_attach to make sure vdev is
> > not on the gc.
> 
> The appended patch switches back the behavior to what we had in v5 and
> before: pdev_path, script, and vdev are all allocated on the gc,
> therefore freed automatically.

Thanks, this looks good to me.

If I slot this in in the place of the original will the rest of the
series apply or shall I wait for a resend/retest?

Ian.

> 
> 
> ---
> 
> libxl: libxl__device_disk_local_attach return a new libxl_device_disk
> 
> Introduce a new libxl_device_disk* parameter to
> libxl__device_disk_local_attach, the parameter is allocated by the
> caller. libxl__device_disk_local_attach is going to fill the new disk
> 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 v9:
> - allocate pdev_path, script, and vdev on the gc.
> 
> Changes in v8:
> - free pdev_path and script in local_detach;
> - use libxl__strdup instead of strdup.
> 
> Changes in v7:
> - rename tmpdisk to localdisk;
> - add a comment in libxl__bootloader_state about localdisk.
> 
> Changes in v6:
> - return error in case pdev_path is NULL;
> - libxl__device_disk_local_attach update the new disk, the caller
> allocates it;
> - remove \n from logs.
> 
> 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>
> 
> diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
> index cd870c4..be1c900 100644
> --- a/tools/libxl/libxl.c
> +++ b/tools/libxl/libxl.c
> @@ -1735,13 +1735,24 @@ out:
>      return ret;
>  }
>  
> -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 *disk)
>  {
>      libxl_ctx *ctx = gc->owner;
>      char *dev = NULL;
>      char *ret = NULL;
>      int rc;
>  
> +    if (in_disk->pdev_path == NULL)
> +        return NULL;
> +
> +    memcpy(disk, in_disk, sizeof(libxl_device_disk));
> +    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;
>  
> @@ -1779,8 +1790,7 @@ char * libxl__device_disk_local_attach(libxl__gc *gc, 
> libxl_device_disk *disk)
>                             " 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);
> +            LOG(DEBUG, "locally attaching qdisk %s", in_disk->pdev_path);
>              dev = disk->pdev_path;
>              break;
>          default:
> diff --git a/tools/libxl/libxl_bootloader.c b/tools/libxl/libxl_bootloader.c
> index e8950b1..82371f1 100644
> --- a/tools/libxl/libxl_bootloader.c
> +++ b/tools/libxl/libxl_bootloader.c
> @@ -228,7 +228,7 @@ static void bootloader_cleanup(libxl__egc *egc, 
> libxl__bootloader_state *bl)
>      if (bl->outputdir) libxl__remove_directory(gc, bl->outputdir);
>  
>      if (bl->diskpath) {
> -        libxl__device_disk_local_detach(gc, bl->disk);
> +        libxl__device_disk_local_detach(gc, &bl->localdisk);
>          free(bl->diskpath);
>          bl->diskpath = 0;
>      }
> @@ -330,7 +330,7 @@ void libxl__bootloader_run(libxl__egc *egc, 
> libxl__bootloader_state *bl)
>          goto out;
>      }
>  
> -    bl->diskpath = libxl__device_disk_local_attach(gc, bl->disk);
> +    bl->diskpath = libxl__device_disk_local_attach(gc, bl->disk, 
> &bl->localdisk);
>      if (!bl->diskpath) {
>          rc = ERROR_FAIL;
>          goto out;
> diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
> index d34e561..21b8b54 100644
> --- a/tools/libxl/libxl_internal.h
> +++ b/tools/libxl/libxl_internal.h
> @@ -1265,7 +1265,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);
>  
> @@ -1803,6 +1804,13 @@ struct libxl__bootloader_state {
>      libxl__bootloader_console_callback *console_available;
>      libxl_domain_build_info *info; /* u.pv.{kernel,ramdisk,cmdline} updated 
> */
>      libxl_device_disk *disk;
> +    /* Should be zeroed by caller on entry.  Will be filled in by
> +     * bootloader machinery; represents the local attachment of the
> +     * disk for the benefit of the bootloader.  Must be detached by
> +     * the caller using libxl__device_disk_local_detach, but only
> +     * after the domain's kernel and initramfs have been loaded into
> +     * memory and the file references disposed of. */
> +    libxl_device_disk localdisk;
>      uint32_t domid;
>      /* private to libxl__run_bootloader */
>      char *outputpath, *outputdir, *logfile;



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