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

[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);

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.

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.

Thanks,
Ian.

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