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

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

The following reply was made to PR port-xen/47193; it has been noted by GNATS.

From: Cherry G. Mathew <cherry%zyx.in@localhost>
To: gnats-bugs%NetBSD.org@localhost
Cc: roger.pau%citrix.com@localhost, port-xen-maintainer%netbsd.org@localhost, 
netbsd-bugs%netbsd.org@localhost
Subject: Re: port-xen/47193: xenwatch handlers are added twice to the list of 
pending watches
Date: Fri, 16 Nov 2012 23:32:04 +0530

 [...]
 
 
 >>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