Re: [Xen-devel] [PATCH 1/2] libxl: fix race in libxl__devices_destroy

[Ian Jackson <Ian.Jackson@xxxxxxxxxxxxx>]

Ian Campbell writes ("Re: [PATCH 1/2] libxl: fix race in 
libxl__devices_destroy"):
> On Thu, 2012-07-26 at 20:12 +0100, Roger Pau Monne wrote:
...
> >  out:
> > -    if (!numdev) drs->callback(egc, drs, rc);
> > +    if (!aodevs->size) drs->callback(egc, drs, rc);
> > +    else {
> > +        /*
> > +         * Create a dummy aodev to check if all the other aodevs are 
> > finished,
> > +         * and call the callback.
> > +         */
> > +        GCNEW(aodev);
> > +        libxl__prepare_ao_device(ao, aodev);
> > +        aodev->aodevs = aodevs;
> > +        aodevs->in_addition = 0;
> > +        libxl__ao_devices_callback(egc, aodev);
> > +    }
> 
> This dummy event strikes me as a bit odd.

It was my suggestion.  However Roger hasn't done quite what I intended
to suggest.

If we were to _first_ create an aodev which represents the scan for
devices, then when we complete the scan we can simply complete that
aodev in the entirely normal way and everything will work correctly.

> Would it not be possible to loop over xenstore creating the aodevs array
> and only then have a second loop which calls
> libxl__initiate_device_remove(egc, aodev) for each in turn? Probably you
> would encapsulate this into a helper libxl__as_devices_initiate which
> would call the callback if size == 0.

IMO it's generally better to do everything at once, rather than
multiple parallel loops.  That avoids (a) races (b) mistakes arising
from changes in one place but not the other.

> That would do away with this whole "in_addition" phase since you
> wouldn't actually initiate anything until you had created the entire
> list.

Using an aodev to represent this, instead of an ad hoc "in_addition",
would regularise this too.

I'll reply more fully to Roger's patches later.

Ian.

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