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 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?

I have already tested the patch before sending it.
I think you might find that 1 hunk of patch #8 won't apply automatically
but it should be trivial to fix the conflict.
Let me know if I need to resend.

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