Re: [Xen-devel] [PATCH v2] libxl: Atomicaly check backend state and set it to 5 at device_remove

[Roger Pau Monnà <roger.pau@xxxxxxxxxxxxx>]

2012/2/20 Ian Jackson <Ian.Jackson@xxxxxxxxxxxxx>:
> Roger Pau Monne writes ("[Xen-devel] [PATCH v2] libxl: Atomicaly check 
> backend state and set it to 5 at device_remove"):
>> libxl: Atomicaly check backend state and set it to 5 at device_remove
>>
>> libxl__device_remove was setting backend state to 5, which could
>> create a race condition, since the previous check for state != 4 and
>> setting state to 5 was not done inside the same transaction, so the
>> kernel could change the state to 6 in the space between the check for
>> state != 4 and setting it to 5.
>>
>> The state != 4 check and setting it to 5 should happen in the same
>> transaction, to assure that nobody is modifying it behind our back.
>
> This looks like a correct fix to me. ÂHowever, the code in libxl has
> changed now: libxl__device_remove is now
> libxl__initiate_device_remove.
>
> Also can you please arrange to use "goto out" style error handling ?
>
> Eg something like:
>
> Â+ Â Â xs_transaction_t t = 0;
> Â Â...
> Â Âout:
> Â+ Â Â if (t) xs_transaction_end(ctx->xsh, t, 0);
> Â Â Â device_remove_cleanup(gc, aorm);

I will commit an updated version of this patch with my hotplug series,
that matches the current state of libxl__initiate_device_remove.

>
> TBH the meat of this function is in general rather unreconstructed.
> Lots of unchecked xs_write etc. ÂI don't know if you want to fix that
> while you're there. ÂI wimped out on doing that in my recent event
> series. ÂUp to you.

Will decide that later, depending on the time frame.

> Really we should have some kind of more cooked xenstore transaction
> arrangements, probably in the gc, and certainly with a suitable loop
> macro so we can say:
> ÂIN_XENSTORE_TRANSACTION {
> Â Â Âread, write, etc.
> Â}
> But that's a task for another day and it's not on the critical path
> for 4.2 so I'm putting it off.

Also I've found myself using a lot a construction like the following:

- check xenstore path
- If path value is different than x
- Set up a watch
- Wait for events.

And I'm afraid it's not concurrent safe, since someone can perform a
bunch of xenstore changes in the space between setting the watch, and
wait for events. The watcher will only get notified of the last
xenstore change, but not all changes that happened between.

> Thanks,
> Ian.

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