From mboxrd@z Thu Jan 1 00:00:00 1970 From: Julien Grall Subject: Re: [PATCH] libxenstore: filter watch events in libxenstore when we unwatch Date: Fri, 21 Sep 2012 17:21:51 +0100 Message-ID: <505C941F.6080903@citrix.com> References: <1348049147-16752-1-git-send-email-julien.grall@citrix.com> <20572.35759.42063.973125@mariner.uk.xensource.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <20572.35759.42063.973125@mariner.uk.xensource.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Ian Jackson Cc: "xen-devel@lists.xen.org" List-Id: xen-devel@lists.xenproject.org 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 > (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