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

[Roger Pau Monné <roger.pau%citrix.com@localhost>]

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

From: =?ISO-8859-1?Q?Roger_Pau_Monn=E9?= <roger.pau%citrix.com@localhost>
To: Manuel Bouyer <bouyer%antioche.eu.org@localhost>
Cc: Cherry G.Mathew <cherry%zyx.in@localhost>, "gnats-bugs%NetBSD.org@localhost"
        <gnats-bugs%NetBSD.org@localhost>, 
"port-xen-maintainer%netbsd.org@localhost"
        <port-xen-maintainer%NetBSD.org@localhost>, 
"netbsd-bugs%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: Mon, 19 Nov 2012 19:49:44 +0100

 On 19/11/12 19:39, Manuel Bouyer wrote:
 > On Sat, Nov 17, 2012 at 09:26:46AM +0100, Roger Pau Monné wrote:
 >> [...]
 >> diff --git a/sys/arch/xen/xenbus/xenbus_xs.c 
 >> b/sys/arch/xen/xenbus/xenbus_xs.c
 >> index c09e7c4..1405807 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,
 >> @@ -798,12 +798,22 @@ process_msg(void)
 >>             found = (msg->u.watch.handle != NULL);
 >>             if (found) {
 >>                     mutex_enter(&watch_events_lock);
 >> -                   SIMPLEQ_INSERT_TAIL(&watch_events, msg, msg_next);
 >> -                   cv_broadcast(&watch_cv);
 >> +                   repeated = false;
 >> +                   /* Don't add duplicate events to the queue of pending 
 >> watches */
 >> +                   SIMPLEQ_FOREACH(s_msg, &watch_events, msg_next) {
 >> +                           if (s_msg->u.watch.handle == 
 >> msg->u.watch.handle) {
 >> +                                   repeated = true;
 >> +                                   break;
 >> +                           }
 >> +                   }
 >> +                   if (!repeated) {
 >> +                           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);
 >>             }
 > 
 > I don't know why gcc doesn't complain about repeated being possibly
 > uninitialised here. AFAIK repeated is initialised only in the found case.
 > repeated = false should probably be moved upper.
 
 I guess because if "repeated" is not initialized, it is not even
 checked, since the first part of the if condition "if (!found)" will
 always be true. I will move the initialization upper.