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

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>
Signed-off-by: Julien Grall <julien.grall@citrix.com>
---
 tools/xenstore/xs.c |   73 +++++++++++++++++++++++++++++++++++++++++++++-----
 1 files changed, 65 insertions(+), 8 deletions(-)

diff --git a/tools/xenstore/xs.c b/tools/xenstore/xs.c
index b951015..df89e37 100644
--- a/tools/xenstore/xs.c
+++ b/tools/xenstore/xs.c
@@ -753,6 +753,19 @@ bool xs_watch(struct xs_handle *h, const char *path, const
char *token)
 				ARRAY_SIZE(iov), NULL));
 }
 
+
+/* Clear the pipe token if there are no more pending watchs.
+ * We suppose the watch_mutex is already taken.
+ */
+static void xs_clear_watch_pipe(struct xs_handle *h)
+{
+	char c;
+
+	if (list_empty(&h->watch_list) && (h->watch_pipe[0] != -1))
+		while (read(h->watch_pipe[0], &c, 1) != 1)
+			continue;
+}
+
 /* Find out what node change was on (will block if nothing pending).
  * Returns array of two pointers: path and token, or NULL.
  * Call free() after use.
@@ -761,7 +774,7 @@ static char **read_watch_internal(struct xs_handle *h,
unsigned int *num,
 				  int nonblocking)
 {
 	struct xs_stored_msg *msg;
-	char **ret, *strings, c = 0;
+	char **ret, *strings;
 	unsigned int num_strings, i;
 
 	mutex_lock(&h->watch_mutex);
@@ -798,11 +811,7 @@ static char **read_watch_internal(struct xs_handle *h,
unsigned int *num,
 	msg = list_top(&h->watch_list, struct xs_stored_msg, list);
 	list_del(&msg->list);
 
-	/* 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;
-
+	xs_clear_watch_pipe(h);
 	mutex_unlock(&h->watch_mutex);
 
 	assert(msg->hdr.type == XS_WATCH_EVENT);
@@ -855,14 +864,62 @@ char **xs_read_watch(struct xs_handle *h, unsigned int
*num)
 bool xs_unwatch(struct xs_handle *h, const char *path, const char *token)
 {
 	struct iovec iov[2];
+	struct xs_stored_msg *msg, *tmsg;
+	bool res;
+	char *s, *p;
+	unsigned int i;
+	char *l_token, *l_path;
 
 	iov[0].iov_base = (char *)path;
 	iov[0].iov_len = strlen(path) + 1;
 	iov[1].iov_base = (char *)token;
 	iov[1].iov_len = strlen(token) + 1;
 
-	return xs_bool(xs_talkv(h, XBT_NULL, XS_UNWATCH, iov,
-				ARRAY_SIZE(iov), NULL));
+	res = xs_bool(xs_talkv(h, XBT_NULL, XS_UNWATCH, iov,
+						   ARRAY_SIZE(iov), NULL));
+
+	/* 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;
+	}
+
+	list_for_each_entry_safe(msg, tmsg, &h->watch_list, list) {
+		assert(msg->hdr.type == XS_WATCH_EVENT);
+
+		s = msg->body;
+
+		l_token = NULL;
+		l_path = NULL;
+
+		for (p = s, i = 0; p < msg->body + msg->hdr.len; p++) {
+			if (*p == ''\0'')
+			{
+				if (i == XS_WATCH_TOKEN)
+					l_token = s;
+				else if (i == XS_WATCH_PATH)
+					l_path = s;
+				i++;
+				s = p + 1;
+			}
+		}
+
+		if (l_token && !strcmp(token, l_token)
+			/* Use strncmp because we can have a watch fired on sub-directory */
+			&& l_path && !strncmp(path, l_path, strlen(path))) {
+			fprintf (stderr, "DELETE\n");
+			list_del(&msg->list);
+			free(msg);
+		}
+	}
+
+	xs_clear_watch_pipe(h);
+
+	mutex_unlock(&h->watch_mutex);
+
+	return res;
 }
 
 /* Start a transaction: changes by others will not be seen during this
-- 
Julien Grall

Julien Grall writes ("[PATCH V2] libxenstore: filter watch events in
libxenstore when we unwatch"):
> 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.
Thanks.
> +
> +/* Clear the pipe token if there are no more pending watchs.
> + * We suppose the watch_mutex is already taken.
> + */
> +static void xs_clear_watch_pipe(struct xs_handle *h)
I think this would be better called xs_maybe_clear_watch_pipe or
something.  Since it doesn''t always clear the watch pipe, it makes the
call sites confusing.
> @@ -855,14 +864,62 @@ char **xs_read_watch(struct xs_handle *h, unsigned
int *num)
>  bool xs_unwatch(struct xs_handle *h, const char *path, const char *token)
>  {
>  	struct iovec iov[2];
> +	struct xs_stored_msg *msg, *tmsg;
> +	bool res;
> +	char *s, *p;
> +	unsigned int i;
> +	char *l_token, *l_path;
>  
>  	iov[0].iov_base = (char *)path;
>  	iov[0].iov_len = strlen(path) + 1;
>  	iov[1].iov_base = (char *)token;
>  	iov[1].iov_len = strlen(token) + 1;
>  
> -	return xs_bool(xs_talkv(h, XBT_NULL, XS_UNWATCH, iov,
> -				ARRAY_SIZE(iov), NULL));
> +	res = xs_bool(xs_talkv(h, XBT_NULL, XS_UNWATCH, iov,
> +						   ARRAY_SIZE(iov), NULL));
> +
> +	/* 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 still think this is redundant, so it should not be here.
> +	list_for_each_entry_safe(msg, tmsg, &h->watch_list, list) {
> +		assert(msg->hdr.type == XS_WATCH_EVENT);
> +
> +		s = msg->body;
> +
> +		l_token = NULL;
> +		l_path = NULL;
> +
> +		for (p = s, i = 0; p < msg->body + msg->hdr.len; p++) {
> +			if (*p == ''\0'')
> +			{
> +				if (i == XS_WATCH_TOKEN)
> +					l_token = s;
> +				else if (i == XS_WATCH_PATH)
> +					l_path = s;
> +				i++;
> +				s = p + 1;
> +			}
> +		}
> +
> +		if (l_token && !strcmp(token, l_token)
> +			/* Use strncmp because we can have a watch fired on sub-directory */
Oh bum.  I see a problem.  This is quite a bad problem.

It is legal to do this:

   client:

     XS_WATCH /foo token1
     XS_WATCH /foo/bar token1

Now if you get a watch event you get the token and the actual path.
So suppose we have, simultaneously:

   our client:                          another client:
     XS_UNWATCH /foo/bar token1          WRITE /foo/bar/zonk sponge

Then xenstored will generate two watch events:
                                WATCH_EVENT /foo/bar/zonk token1
                                WATCH_EVENT /foo/bar/zonk token1

With your patch, both of these would be thrown away.  Whereas in fact
one of them should still be presented.

How confident are we that there are no clients which rely on this not
changing ?

At the very least this needs a documentation patch explaining the
undesirable consequences of setting multiple watches with the same
token.  Arguably it needs a flag to the library (or a new function
name) to ask for this new behaviour.

I would have to think, for example, about whether the new libxl event
sub-library would make any mistakes as a result of this proposed
change.  I think it wouldn''t...

Ian.

On 09/26/2012 04:03 PM, Ian Jackson wrote:
> Julien Grall writes ("[PATCH V2] libxenstore: filter watch events in
libxenstore when we unwatch"):
>> +
>> +/* Clear the pipe token if there are no more pending watchs.
>> + * We suppose the watch_mutex is already taken.
>> + */
>> +static void xs_clear_watch_pipe(struct xs_handle *h)
> 
> I think this would be better called xs_maybe_clear_watch_pipe or
> something.  Since it doesn''t always clear the watch pipe, it makes
the
> call sites confusing.
I will fix on the next patch version.
>> @@ -855,14 +864,62 @@ char **xs_read_watch(struct xs_handle *h,
unsigned int *num)
>>  bool xs_unwatch(struct xs_handle *h, const char *path, const char
*token)
>>  {
>>  	struct iovec iov[2];
>> +	struct xs_stored_msg *msg, *tmsg;
>> +	bool res;
>> +	char *s, *p;
>> +	unsigned int i;
>> +	char *l_token, *l_path;
>>  
>>  	iov[0].iov_base = (char *)path;
>>  	iov[0].iov_len = strlen(path) + 1;
>>  	iov[1].iov_base = (char *)token;
>>  	iov[1].iov_len = strlen(token) + 1;
>>  
>> -	return xs_bool(xs_talkv(h, XBT_NULL, XS_UNWATCH, iov,
>> -				ARRAY_SIZE(iov), NULL));
>> +	res = xs_bool(xs_talkv(h, XBT_NULL, XS_UNWATCH, iov,
>> +						   ARRAY_SIZE(iov), NULL));
>> +
>> +	/* 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 still think this is redundant, so it should not be here.

The piece of code I moved in the new function expects to read one
character in the pipe. So if the pipe is empty it will either block (if
the file descriptor is not set to NONBLOCK) either loop as read return 0
or -1.

This code is definitely usefull if we want to avoid this annoying
problem. Perhaps we should modify the behavior of this piece of code.
>> +	list_for_each_entry_safe(msg, tmsg, &h->watch_list, list) {
>> +		assert(msg->hdr.type == XS_WATCH_EVENT);
>> +
>> +		s = msg->body;
>> +
>> +		l_token = NULL;
>> +		l_path = NULL;
>> +
>> +		for (p = s, i = 0; p < msg->body + msg->hdr.len; p++) {
>> +			if (*p == ''\0'')
>> +			{
>> +				if (i == XS_WATCH_TOKEN)
>> +					l_token = s;
>> +				else if (i == XS_WATCH_PATH)
>> +					l_path = s;
>> +				i++;
>> +				s = p + 1;
>> +			}
>> +		}
>> +
>> +		if (l_token && !strcmp(token, l_token)
>> +			/* Use strncmp because we can have a watch fired on sub-directory
*/
> 
> Oh bum.  I see a problem.  This is quite a bad problem.
> 
> It is legal to do this:
> 
>    client:
> 
>      XS_WATCH /foo token1
>      XS_WATCH /foo/bar token1
> 
> Now if you get a watch event you get the token and the actual path.
> So suppose we have, simultaneously:
> 
>    our client:                          another client:
>      XS_UNWATCH /foo/bar token1          WRITE /foo/bar/zonk sponge
> 
> Then xenstored will generate two watch events:
>                                 WATCH_EVENT /foo/bar/zonk token1
>                                 WATCH_EVENT /foo/bar/zonk token1
> 
> With your patch, both of these would be thrown away.  Whereas in fact
> one of them should still be presented.
> 
> How confident are we that there are no clients which rely on this not
> changing ?
>
> At the very least this needs a documentation patch explaining the
> undesirable consequences of setting multiple watches with the same
> token.  Arguably it needs a flag to the library (or a new function
> name) to ask for this new behaviour.
> 
> I would have to think, for example, about whether the new libxl event
> sub-library would make any mistakes as a result of this proposed
> change.  I think it wouldn''t...

If a client decides to use the same token for multiple path (including
one which is the sub-directory), it''s not possible to know which
WATCH_EVENT we need to remove.

The current problem that the patch tries to resolve is mainly for
clients who use token with a pointer inside (as QEMU made).

Adding a flag to xs_open could be a good solution as:
#define XS_UNWATCH_SAFE 1UL<<2

If the flag is not enabled, xs_unwatch has the same (buggy?) behaviour
as before. When it''s enabled, xs_unwatch will browse the watch list and
will remove spurious watch event.

What do you think?

Sincerely yours,

-- 
Julien Grall