[Xen-devel] [PATCH] libxenstore: filter watch events in libxenstore when we unwatch

[Ian Jackson <Ian.Jackson@xxxxxxxxxxxxx>]

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@xxxxxxxxxxxxx>
(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.

> +     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.

> +             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 ?

In any case it's clone-and-hack: the same 3/4 lines occur elsewhere.
If we keep this in the patch it should be made into a function.

Thanks,
Ian.

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