Re: [PATCH v3 09/15] libxl: synchronise configuration when we hotplug a device

[Wei Liu <wei.liu2@citrix.com>]

On Tue, Sep 09, 2014 at 12:11:52PM +0100, Ian Campbell wrote:
[...]
> > + * All device hotplug routines should comply to following pattern:
> > + *   lock json config (json_lock)
> > + *       read json config
> > + *       update in-memory json config with new entry, rewrite
> > + *           if there's stale entry
> 
> Does "rewrite" here mean back to disk, or just the in-memory copy? I
> think it is important for the protocol that it is the in-memory one
> only.
> 
> I think you can just say "replacing any stale entry".

It means replacing the entry in in-memory copy.

I will rephrase this line.

> 
> > + *       for loop -- xs transaction
> > + *           open xs transaction
> > + *           check device existence, abort if it exists
> > + *           write in-memory json config to disk
> > + *           commit xs transaction
> > + *       end for loop
> > + *   unlock json config
> > + *
> > + * Device removal routines are not touched.
> > + *
> > + * Here is the proof that we always maintain that invariant and we
> > + * don't leak files during interaction of hotplug thread and other
> > + * threads / processes.
> > + *
> > + * # Safe against parallel add
> > + *
> > + * When another thread / process tries to add same device, it's
> > + * blocked by json_lock. The loser of two threads will bail at
> > + * existence check, so that we don't overwrite anything.
> > + *
> > + * # Safe against domain destruction
> > + *
> > + * When another thread / process tries to destroy domain, it's blocked
> > + * by json_lock. If domain destruction thread is loser, it deletes
> > + * every userdata file after it requires the lock. If hotplug thread
> > + * is loser, it bails at acquiring lock, no device is added. Either
> > + * way, no file is leaked.
> 
> I don't follow this one.
> 
> For the destructor loses case I think all you need to say is that the
> json lock prevents the destruction process from removing the userdata
> while the add is ongoing, the reference to deleting userdata after it
> acquires the lock is just confusing.
> 
> In the "hotplug thread loses" case I think you should explain why it
> bails (the existence check I suppose).
> 

How about this:

  If the thread / process trying to destroy domain loses the rase, it's
  blocked by json_lock. If the hotplug thread is loser, it bails at
  acquiring lock because lock acquisition function checks existence of
  the domain.

Re typos, I will fix them in next round.

Wei.