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

[Stefano Stabellini <stefano.stabellini@xxxxxxxxxxxxx>]

On Tue, 22 May 2012, Ian Jackson wrote:
> Stefano Stabellini writes ("[PATCH v7 02/11] 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.
> 
> Thanks.
> 
> So comparing the comment:
> 
> > +    /* 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;
> 
> with the code in the caller: on entry it is indeed zeroed by the GCNEW
> of the whole domain_create_state.  However on exit:
> 
> >      if (bl->diskpath) {
> > -        libxl__device_disk_local_detach(gc, bl->disk);
> > +        libxl__device_disk_local_detach(gc, &bl->localdisk);
> > +        free(bl->localdisk.pdev_path);
> > +        free(bl->localdisk.script);
> >          free(bl->diskpath);
> 
> This does not correspond exactly to the comment.  Not only do we call
> detach but apparently we need to free some things too.
> 
> It's not clear to me why these things haven't come from a suitable
> gc.  But if they can't come from a gc then the comment needs to
> mention that they need to be freed.
> 
> > -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 = strdup(in_disk->pdev_path);
> > +    if (in_disk->script != NULL)
> > +        disk->script = strdup(in_disk->script);
> > +    disk->vdev = NULL;
> 
> Re my comment about the gc, do we call this function anywhere else ?
> Could it take the ao for the benefit of its gc ?  Would that violate
> some rules about the usual contents of a libxl_device_disk ?

I am not sure I want to get into the whole AO thing at this point.

Am I supposed to just change the libxl__gc *gc argument into libxl__ao
*ao and then call STATE_AO_GC on function entry?
Then remove the free(bl->localdisk.script) and
free(bl->localdisk.pdev_path)?
Anything else is required?

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