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

[Stefano Stabellini <stefano.stabellini@xxxxxxxxxxxxx>]

On Tue, 29 May 2012, Ian Campbell wrote:
> On Tue, 2012-05-29 at 11:39 +0100, Stefano Stabellini wrote:
> > 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 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>
> > ---
> >  tools/libxl/libxl.c            |   18 +++++++++++++++---
> >  tools/libxl/libxl_bootloader.c |    4 ++--
> >  tools/libxl/libxl_internal.h   |   10 +++++++++-
> >  3 files changed, 26 insertions(+), 6 deletions(-)
> > 
> > diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
> > index cd870c4..762e72a 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)
> 
> Why the weird indent?

Uhm.. I don't know, I think it was just the default on vim.


> >  {
> >      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(NULL, in_disk->pdev_path);
> > +    if (in_disk->script != NULL)
> > +        disk->script = libxl__strdup(NULL, 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:
> > @@ -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.

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