Re: port-xen/47193: xenwatch handlers are added twice to the list of pending watches

[Cherry G. Mathew <cherry%zyx.in@localhost>]

[...]


>>Fix:
>The fix is easy, we need to check if the watch handler has already been added 
>to the queue of pending events before adding it. Here is a proposed patch.
>
>---
>diff --git a/sys/arch/xen/xenbus/xenbus_xs.c b/sys/arch/xen/xenbus/xenbus_xs.c
>index c09e7c4..8cef74a 100644
>--- a/sys/arch/xen/xenbus/xenbus_xs.c
>+++ b/sys/arch/xen/xenbus/xenbus_xs.c
>@@ -751,7 +751,7 @@ xenwatch_thread(void *unused)
> static int
> process_msg(void)
> {
>-      struct xs_stored_msg *msg;
>+      struct xs_stored_msg *msg, *s_msg;
>       char *body;
>       int err;
> 
>@@ -782,7 +782,7 @@ process_msg(void)
>       body[msg->hdr.len] = '\0';
> 
>       if (msg->hdr.type == XS_WATCH_EVENT) {
>-              bool found;
>+              bool found, repeated;
> 
>               DPRINTK("process_msg: XS_WATCH_EVENT");
>               msg->u.watch.vec = split(body, msg->hdr.len,
>@@ -797,13 +797,20 @@ process_msg(void)
>                   msg->u.watch.vec[XS_WATCH_TOKEN]);
>               found = (msg->u.watch.handle != NULL);
>               if (found) {
>+                      repeated = false;
>+                      SIMPLEQ_FOREACH(s_msg, &watch_events, msg_next) {
>+                              if (s_msg->u.watch.handle == 
>msg->u.watch.handle)
>+                                      repeated = true;
>+                      }
>+              }

This bit is missing locking (mutex_enter/exit(&watch_events_lock))

You could avoid a second set of locking by re-using 'found' a bit like
this:

        if (found) {
                mutex_enter(&watch_events_lock);
                SIMPLEQ_FOREACH(s_msg, &watch_events, msg_next) {
                        if (s_msg->u.watch.handle == msg->u.watch.handle) {
                                found = 0;
                                goto isduplicate;
                }

                SIMPLEQ_INSERT_TAIL(&watch_events, msg, msg_next);

                cv_broadcast(&watch_cv);
        isduplicate:
                mutex_exit(&watch_events_lock);
        }


   
>+              if (found && !repeated) {
>                       mutex_enter(&watch_events_lock);
>                       SIMPLEQ_INSERT_TAIL(&watch_events, msg, msg_next);
>                       cv_broadcast(&watch_cv);
>                       mutex_exit(&watch_events_lock);
>               }
>               mutex_exit(&watches_lock);
>-              if (!found) {
>+              if (!found || repeated) {
>                       free(msg->u.watch.vec, M_DEVBUF);
>                       free(msg, M_DEVBUF);
>               }
>

Not related to your code but is there a potential leak with malloc() for
body = malloc(...) if (!found) {} ?

Cheers,
-- 
Cherry