kqueue: Make EVFILT_USER event behaviour more consistent.
authorImre Vadász <imre@vdsz.com>
Sat, 19 Aug 2017 19:50:28 +0000 (21:50 +0200)
committerImre Vadász <imre@vdsz.com>
Wed, 23 Aug 2017 19:03:14 +0000 (21:03 +0200)
* Stop abusing the kn->kn_sfflags value for storing the current state of
  the EVFILT_USER filter. Instead use kn->kn_fflags like other filters.
  Similarly store the data value in kn->kn_data instead of kn->kn_sdata.
  This means that the fflags value gets reset when EV_CLEAR was specified
  when adding the event, and the event is received by userspace. This
  behaviour is consistent with existing kqueue filters, and allows using
  EVFILT_USER properly as an edge-triggered event when using the fflags,
  and not just level-triggered.

* Don't clear kn->kn_fflags when the event is modified with EV_CLEAR. Doing
  this wasn't affecting the actual state of the EVFILT_USER event before
  this change (since the state was kept in kn->kn_sfflags instead).

* All this also avoids blindly copying the fflags value that was specified
  when adding the event. Instead the NOTE_FFLAGSMASK mask is applied, and
  the NOTE_FF* options are used, so the returned fflags value should now
  always only have the lower 24 bits set.

* Make setting the fflags and data value when adding the event work as
  might be expected.

sys/kern/kern_event.c

index cdd4471..e776ce5 100644 (file)
@@ -521,11 +521,39 @@ filt_timer(struct knote *kn, long hint)
 static int
 filt_userattach(struct knote *kn)
 {
+       u_int ffctrl;
+
        kn->kn_hook = NULL;
        if (kn->kn_sfflags & NOTE_TRIGGER)
                kn->kn_ptr.hookid = 1;
        else
                kn->kn_ptr.hookid = 0;
+
+       ffctrl = kn->kn_sfflags & NOTE_FFCTRLMASK;
+       kn->kn_sfflags &= NOTE_FFLAGSMASK;
+       switch (ffctrl) {
+       case NOTE_FFNOP:
+               break;
+
+       case NOTE_FFAND:
+               kn->kn_fflags &= kn->kn_sfflags;
+               break;
+
+       case NOTE_FFOR:
+               kn->kn_fflags |= kn->kn_sfflags;
+               break;
+
+       case NOTE_FFCOPY:
+               kn->kn_fflags = kn->kn_sfflags;
+               break;
+
+       default:
+               /* XXX Return error? */
+               break;
+       }
+       /* We just happen to copy this value as well. Undocumented. */
+       kn->kn_data = kn->kn_sdata;
+
        return 0;
 }
 
@@ -558,22 +586,23 @@ filt_usertouch(struct knote *kn, struct kevent *kev, u_long type)
                        break;
 
                case NOTE_FFAND:
-                       kn->kn_sfflags &= kev->fflags;
+                       kn->kn_fflags &= kev->fflags;
                        break;
 
                case NOTE_FFOR:
-                       kn->kn_sfflags |= kev->fflags;
+                       kn->kn_fflags |= kev->fflags;
                        break;
 
                case NOTE_FFCOPY:
-                       kn->kn_sfflags = kev->fflags;
+                       kn->kn_fflags = kev->fflags;
                        break;
 
                default:
                        /* XXX Return error? */
                        break;
                }
-               kn->kn_sdata = kev->data;
+               /* We just happen to copy this value as well. Undocumented. */
+               kn->kn_data = kev->data;
 
                /*
                 * This is not the correct use of EV_CLEAR in an event
@@ -586,15 +615,22 @@ filt_usertouch(struct knote *kn, struct kevent *kev, u_long type)
                 */
                if (kev->flags & EV_CLEAR) {
                        kn->kn_ptr.hookid = 0;
+                       /*
+                        * Clearing kn->kn_data is fine, since it gets set
+                        * every time anyway. We just shouldn't clear
+                        * kn->kn_fflags here, since that would limit the
+                        * possible uses of this API. NOTE_FFAND or
+                        * NOTE_FFCOPY should be used for explicitly clearing
+                        * kn->kn_fflags.
+                        */
                        kn->kn_data = 0;
-                       kn->kn_fflags = 0;
                }
                break;
 
         case EVENT_PROCESS:
                *kev = kn->kn_kevent;
-               kev->fflags = kn->kn_sfflags;
-               kev->data = kn->kn_sdata;
+               kev->fflags = kn->kn_fflags;
+               kev->data = kn->kn_data;
                if (kn->kn_flags & EV_CLEAR) {
                        kn->kn_ptr.hookid = 0;
                        /* kn_data, kn_fflags handled by parent */