Re: [PATCH] libxenstore: filter watch events in libxenstore when we unwatch

[Julien Grall <julien.grall@citrix.com>]

On 09/21/2012 04:45 PM, Ian Jackson wrote:

> Julien Grall writes ("[PATCH] libxenstore: filter watch events in libxenstore when we unwatch"):
>> XenStore puts in queued watch events via a thread and notifies the user.
>> Sometimes xs_unwatch is called before all related message is read. The use
>> case is non-threaded libevent, we have two event A and B:
>>     - Event A will destroy something and call xs_unwatch;
>>     - Event B is used to notify that a node has changed in XenStore.
>> As the event is called one by one, event A can be handled before event B.
>> So on next xs_watch_read the user could retrieve an unwatch token and
>> a segfault occured if the token store the pointer of the structure
>> (ie: "backend:0xcafe").
> 
> Missing from the explanation here is why your patch is sufficient to
> avoid the race.  The answer is as follows (and should probably be in
> the commit message):
> 
> While on entry to xs_unwatch, there may be an event on its way from
> xenstored (eg in the ring or in the local kernel), all such events
> will definitely come before the reply to the unwatch command.  So at
> the point where the unwatch reply has been processed (after xs_talkv),
> any such now-deleted watch events will definitely have made it to
> libxenstore's queue where we can remove them.
> 
> As for other threads in the same process: if two threads call
> xs_read_watch and xs_unwatch, it is acceptable for the xs_read_watch
> to return the event being deleted.  What is not allowed is for an
> xs_read_watch entered after xs_unwatch returns to return the deleted
> event, and this code does indeed ensure that.
> 
> Signed-off-by: Ian Jackson <ian.jackson@eu.citrix.com>
> (for the explanation)
> 
> Now for some comments on the patch:
> 
>> +	/* Filter the watch list to remove potential message */
>> +	mutex_lock(&h->watch_mutex);
>> +
>> +	if (list_empty(&h->watch_list)) {
>> +		mutex_unlock(&h->watch_mutex);
>> +		return res;
>> +	}
> 
> I think this check is unnecessary.  If the list is empty then walking
> it is trivially a no-op.

It's usefull, if we need to clean the pipe (see explanation below).
Otherwise, the read will block and we want to avoid that on
mono-threaded (I don't consider xenstore thread).

>> +	list_for_each_entry_safe(msg, tmsg, &h->watch_list, list) {
>> +		assert(msg->hdr.type == XS_WATCH_EVENT);
>> +
>> +		strings = msg->body;
>> +		num_strings = xs_count_strings(strings, msg->hdr.len);
> 
> The num_strings thing is obsolete.  There will always be two strings.
> Also xs_count_strings walks the array.

We can't assume that XS_WATCH_EVENT will always equal to 2. So we need
to browse until we find the right string. I use xs_count_strings because
it allows to not take care of the length message in the loop.

>> +		for (i = 0; i < num_strings; i++) {
>> +			if (i == XS_WATCH_TOKEN && !strcmp (token, strings)) {
> 
> This is rather odd.  It amounts to:
> 
>    for (i= blah blah) { if (i==FIXED_VALUE) { ..i.. } }
> 
>> +				list_del(&msg->list);
>> +				free(msg);
>> +				break;
>> +			}
>> +			strings = strings + strlen (strings) + 1;
> 
> You need to check the path as well as the token, since it is legal to
> set up multiple watches on different paths with the same token.
> 
> I think you can then do away with the calculation of "strings", at
> least mostly.
> 
>> +	/* Clear the pipe token if there are no more pending watches. */
>> +	if (list_empty(&h->watch_list) && (h->watch_pipe[0] != -1)) {
>> +		while (read(h->watch_pipe[0], &c, 1) != 1)
>> +			continue;
>> +	}
> 
> I'm not convinced this is necessary.  Don't callers already need to
> cope with potential spurious signallings of the watch pipe ?


I base my code on xs_read_watch. As I understand xs_read_watch, it will
wait on a condition until the list is not empty.
So if the list is empty and not the pipe, an event can occur and block
the application (with xs_read_watch).

Sincerely yours,

Julien Grall