bug#26126: 26.0.50; file-notify-rm-watch removes arbitrary watches

[Andreas Politz]

I think there are two ways to fix the main problem discussed here.

1. Change inotify.c and make it return/receive a unique descriptor per client.

2. Wrap inotify's descriptor inside file-notify-add-watch, similar to
   how it's currently done.  This is what I was refering to as
   boxing/unboxing earlier.

The 2nd approach is problematic in the context of file-name-handler, so
I attempted to solve it the other way: Instead of a single number, use a
cons of

     (INOTIFY-DESCRIPTOR . ID) 

, which is unique across all active watches.  I.e. this makes inotify
work like all the other back-ends/handler.  Here is a first draft of a
corresponding patch, let me know what you think.

diff --git a/lisp/filenotify.el b/lisp/filenotify.el
index 7eb6229976..beae94c2c2 100644
--- a/lisp/filenotify.el
+++ b/lisp/filenotify.el
@@ -55,9 +55,8 @@ file-notify--rm-descriptor
   "Remove DESCRIPTOR from `file-notify-descriptors'.
 DESCRIPTOR should be an object returned by `file-notify-add-watch'.
 If it is registered in `file-notify-descriptors', a stopped event is sent."
-  (let* ((desc (if (consp descriptor) (car descriptor) descriptor))
-         (registered (gethash desc file-notify-descriptors))
-        (file (if (consp descriptor) (cdr descriptor) (cl-caadr registered)))
+  (let* ((registered (gethash descriptor file-notify-descriptors))
+        (file (cl-caadr registered))
         (dir (car registered)))
 
     (when (consp registered)
@@ -69,12 +68,12 @@ file-notify--rm-descriptor
 
       ;; Modify `file-notify-descriptors'.
       (if (not file)
-         (remhash desc file-notify-descriptors)
+         (remhash descriptor file-notify-descriptors)
        (setcdr registered
                (delete (assoc file (cdr registered)) (cdr registered)))
        (if (null (cdr registered))
-           (remhash desc file-notify-descriptors)
-         (puthash desc registered file-notify-descriptors))))))
+           (remhash descriptor file-notify-descriptors)
+         (puthash descriptor registered file-notify-descriptors))))))
 
 ;; This function is used by `inotify', `kqueue', `gfilenotify' and
 ;; `w32notify' events.
@@ -102,9 +101,9 @@ file-notify--pending-event
 (defun file-notify--event-watched-file (event)
   "Return file or directory being watched.
 Could be different from the directory watched by the backend library."
-  (let* ((desc (if (consp (car event)) (caar event) (car event)))
-         (registered (gethash desc file-notify-descriptors))
-        (file (if (consp (car event)) (cdar event) (cl-caadr registered)))
+  (let* ((descriptor (car event))
+         (registered (gethash descriptor file-notify-descriptors))
+        (file (cl-caadr registered))
         (dir (car registered)))
     (if file (expand-file-name file dir) dir)))
 
@@ -133,17 +132,11 @@ file-notify--event-cookie
 ;; `inotify' returns the same descriptor when the file (directory)
 ;; uses the same inode.  We want to distinguish, and apply a virtual
 ;; descriptor which make the difference.
-(defun file-notify--descriptor (desc file)
+(defun file-notify--descriptor (desc _file)
   "Return the descriptor to be used in `file-notify-*-watch'.
 For `gfilenotify' and `w32notify' it is the same descriptor as
 used in the low-level file notification package."
-  (if (and (natnump desc) (eq file-notify--library 'inotify))
-      (cons desc
-            (and (stringp file)
-                 (car (assoc
-                       (file-name-nondirectory file)
-                       (gethash desc file-notify-descriptors)))))
-    desc))
+  desc)
 
 ;; The callback function used to map between specific flags of the
 ;; respective file notifications, and the ones we return.
@@ -396,7 +389,6 @@ file-notify-add-watch
 
     ;; Modify `file-notify-descriptors'.
     (setq file (unless (file-directory-p file) (file-name-nondirectory file))
-         desc (if (consp desc) (car desc) desc)
          registered (gethash desc file-notify-descriptors)
          entry `(,file . ,callback))
     (unless (member entry (cdr registered))
@@ -408,9 +400,8 @@ file-notify-add-watch
 (defun file-notify-rm-watch (descriptor)
   "Remove an existing watch specified by its DESCRIPTOR.
 DESCRIPTOR should be an object returned by `file-notify-add-watch'."
-  (let* ((desc (if (consp descriptor) (car descriptor) descriptor))
-        (file (if (consp descriptor) (cdr descriptor)))
-         (registered (gethash desc file-notify-descriptors))
+  (let* ((file nil)
+         (registered (gethash descriptor file-notify-descriptors))
         (dir (car registered))
         (handler (and (stringp dir)
                        (find-file-name-handler dir 'file-notify-rm-watch))))
@@ -432,7 +423,7 @@ file-notify-rm-watch
                 ((eq file-notify--library 'kqueue) 'kqueue-rm-watch)
                 ((eq file-notify--library 'gfilenotify) 'gfile-rm-watch)
                 ((eq file-notify--library 'w32notify) 'w32notify-rm-watch))
-               desc))
+               descriptor))
           (file-notify-error nil)))
 
       ;; Modify `file-notify-descriptors'.
@@ -441,9 +432,8 @@ file-notify-rm-watch
 (defun file-notify-valid-p (descriptor)
   "Check a watch specified by its DESCRIPTOR.
 DESCRIPTOR should be an object returned by `file-notify-add-watch'."
-  (let* ((desc (if (consp descriptor) (car descriptor) descriptor))
-        (file (if (consp descriptor) (cdr descriptor)))
-         (registered (gethash desc file-notify-descriptors))
+  (let* ((file nil)
+         (registered (gethash descriptor file-notify-descriptors))
         (dir (car registered))
         handler)
 
@@ -464,7 +454,7 @@ file-notify-valid-p
                ((eq file-notify--library 'kqueue) 'kqueue-valid-p)
                ((eq file-notify--library 'gfilenotify) 'gfile-valid-p)
                ((eq file-notify--library 'w32notify) 'w32notify-valid-p))
-              desc))
+              descriptor))
            t))))
 
 ;; The end:
diff --git a/src/inotify.c b/src/inotify.c
index 61ef615328..302f52225e 100644
--- a/src/inotify.c
+++ b/src/inotify.c
@@ -51,10 +51,47 @@ static int inotifyfd = -1;
 static Lisp_Object watch_list;
 
 static Lisp_Object
-make_watch_descriptor (int wd)
+add_watch_descriptor (int wd, Lisp_Object filename, Lisp_Object callback)
 {
-  /* TODO replace this with a Misc Object! */
-  return make_number (wd);
+  Lisp_Object descriptor = make_number (wd);
+  Lisp_Object elt = Fassoc (descriptor, watch_list);
+  Lisp_Object list = Fcdr (elt);
+  Lisp_Object watch, watch_id;
+  int id = 0;
+
+  while (! NILP (list))
+    {
+      id = max (id, 1 + XINT (XCAR (XCAR (list))));
+      list = XCDR (list);
+    }
+
+  watch_id = make_number (id);
+  watch = list3 (watch_id, filename, callback);
+
+  if (NILP (elt))
+    watch_list = Fcons (Fcons (descriptor, Fcons (watch, Qnil)),
+                        watch_list);
+  else
+    XSETCDR (elt, Fcons (watch, XCDR (elt)));
+
+  return Fcons (descriptor, watch_id);
+}
+
+static void
+remove_watch_descriptor (Lisp_Object descriptor, Lisp_Object id)
+{
+  Lisp_Object watches = Fassoc (descriptor, watch_list);
+
+  if (CONSP (watches))
+    {
+      Lisp_Object watch = Fassoc (id, XCDR (watches));
+
+      if (CONSP (watch))
+        XSETCDR (watches, Fdelete (watch, XCDR (watches)));
+
+      if (NILP (XCDR (watches)))
+        watch_list = Fdelete (watches, watch_list);
+    }
 }
 
 static Lisp_Object
@@ -96,7 +133,7 @@ mask_to_aspects (uint32_t mask) {
 }
 
 static Lisp_Object
-inotifyevent_to_event (Lisp_Object watch_object, struct inotify_event const 
*ev)
+inotifyevent_to_event (Lisp_Object watch, struct inotify_event const *ev)
 {
   Lisp_Object name = Qnil;
   if (ev->len > 0)
@@ -106,13 +143,13 @@ inotifyevent_to_event (Lisp_Object watch_object, struct 
inotify_event const *ev)
       name = DECODE_FILE (name);
     }
   else
-    name = XCAR (XCDR (watch_object));
+    name = XCAR (XCDR (watch));
 
-  return list2 (list4 (make_watch_descriptor (ev->wd),
+  return list2 (list4 (Fcons (make_number (ev->wd), XCAR (watch)),
                        mask_to_aspects (ev->mask),
                        name,
                        make_number (ev->cookie)),
-               Fnth (make_number (2), watch_object));
+               Fnth (make_number (2), watch));
 }
 
 /* This callback is called when the FD is available for read.  The inotify
@@ -121,7 +158,6 @@ static void
 inotify_callback (int fd, void *_)
 {
   struct input_event event;
-  Lisp_Object watch_object;
   int to_read;
   char *buffer;
   ssize_t n;
@@ -146,20 +182,23 @@ inotify_callback (int fd, void *_)
   while (i < (size_t)n)
     {
       struct inotify_event *ev = (struct inotify_event *) &buffer[i];
+      Lisp_Object descriptor = make_number (ev->wd);
+      Lisp_Object watches = Fassoc (descriptor, watch_list);
 
-      watch_object = Fassoc (make_watch_descriptor (ev->wd), watch_list);
-      if (!NILP (watch_object))
+      if (CONSP (watches))
         {
-          event.arg = inotifyevent_to_event (watch_object, ev);
+          for (Lisp_Object elt = XCDR (watches); CONSP (elt); elt = XCDR (elt))
+            {
+              event.arg = inotifyevent_to_event (XCAR (elt), ev);
 
+              if (!NILP (event.arg))
+                kbd_buffer_store_event (&event);
+            }
           /* If event was removed automatically: Drop it from watch list.  */
           if (ev->mask & IN_IGNORED)
-            watch_list = Fdelete (watch_object, watch_list);
-
-         if (!NILP (event.arg))
-           kbd_buffer_store_event (&event);
+            for (Lisp_Object elt = XCDR (watches); CONSP (elt); elt = XCDR 
(elt))
+              remove_watch_descriptor (descriptor, XCAR (elt));
         }
-
       i += sizeof (*ev) + ev->len;
     }
 
@@ -292,14 +331,12 @@ renames (moved-from and moved-to).
 See inotify(7) and inotify_add_watch(2) for further information.  The inotify 
fd
 is managed internally and there is no corresponding inotify_init.  Use
 `inotify-rm-watch' to remove a watch.
-             */)
+            */)
      (Lisp_Object file_name, Lisp_Object aspect, Lisp_Object callback)
 {
   uint32_t mask;
-  Lisp_Object watch_object;
   Lisp_Object encoded_file_name;
-  Lisp_Object watch_descriptor;
-  int watchdesc = -1;
+  int wd = -1;
 
   CHECK_STRING (file_name);
 
@@ -314,22 +351,11 @@ is managed internally and there is no corresponding 
inotify_init.  Use
 
   mask = aspect_to_inotifymask (aspect);
   encoded_file_name = ENCODE_FILE (file_name);
-  watchdesc = inotify_add_watch (inotifyfd, SSDATA (encoded_file_name), mask);
-  if (watchdesc == -1)
+  wd = inotify_add_watch (inotifyfd, SSDATA (encoded_file_name), mask);
+  if (wd == -1)
     report_file_notify_error ("Could not add watch for file", file_name);
 
-  watch_descriptor = make_watch_descriptor (watchdesc);
-
-  /* Delete existing watch object.  */
-  watch_object = Fassoc (watch_descriptor, watch_list);
-  if (!NILP (watch_object))
-      watch_list = Fdelete (watch_object, watch_list);
-
-  /* Store watch object in watch list.  */
-  watch_object = list3 (watch_descriptor, encoded_file_name, callback);
-  watch_list = Fcons (watch_object, watch_list);
-
-  return watch_descriptor;
+  return add_watch_descriptor (wd, file_name, callback);
 }
 
 DEFUN ("inotify-rm-watch", Finotify_rm_watch, Sinotify_rm_watch, 1, 1, 0,
@@ -341,16 +367,24 @@ See inotify_rm_watch(2) for more information.
              */)
      (Lisp_Object watch_descriptor)
 {
-  Lisp_Object watch_object;
-  int wd = XINT (watch_descriptor);
 
-  if (inotify_rm_watch (inotifyfd, wd) == -1)
-    report_file_notify_error ("Could not rm watch", watch_descriptor);
+  Lisp_Object descriptor, id;
 
-  /* Remove watch descriptor from watch list.  */
-  watch_object = Fassoc (watch_descriptor, watch_list);
-  if (!NILP (watch_object))
-    watch_list = Fdelete (watch_object, watch_list);
+  if (! (CONSP (watch_descriptor)
+         && INTEGERP (XCAR (watch_descriptor))
+         && INTEGERP (XCDR (watch_descriptor))))
+    report_file_notify_error ("Invalid descriptor ", watch_descriptor);
+
+  descriptor = XCAR (watch_descriptor);
+  id = XCDR (watch_descriptor);
+  remove_watch_descriptor (descriptor, id);
+
+  if (NILP (Fassoc (descriptor, watch_list)))
+    {
+      int wd = XINT (descriptor);
+      if (inotify_rm_watch (inotifyfd, wd) == -1)
+        report_file_notify_error ("Could not rm watch", watch_descriptor);
+    }
 
   /* Cleanup if no more files are watched.  */
   if (NILP (watch_list))
@@ -374,10 +408,34 @@ reason.  Removing the watch by calling `inotify-rm-watch' 
also makes
 it invalid.  */)
      (Lisp_Object watch_descriptor)
 {
-  Lisp_Object watch_object = Fassoc (watch_descriptor, watch_list);
-  return NILP (watch_object) ? Qnil : Qt;
+  Lisp_Object watches;
+
+  if (! (CONSP (watch_descriptor)
+         && INTEGERP (XCAR (watch_descriptor))
+         && INTEGERP (XCDR (watch_descriptor))))
+    return Qnil;
+
+  watches = Fassoc (XCAR (watch_descriptor), watch_list);
+
+  if (! CONSP (watches))
+    return Qnil;
+  return CONSP (Fassoc (XCDR (watch_descriptor), XCDR (watches)));
+}
+
+#ifdef DEBUG
+DEFUN ("inotify-watch-list", Finotify_watch_list, Sinotify_watch_list, 0, 0, 0,
+       doc: /* Return a copy of the internal watch_list. */)
+{
+  return Fcopy_sequence (watch_list);
 }
 
+DEFUN ("inotify-allocated-p", Finotify_allocated_p, Sinotify_allocated_p, 0, 
0, 0,
+       doc: /* Return non-nil, if a inotify instance is allocated. */)
+{
+  return inotifyfd < 0 ? Qnil : Qt;
+}
+#endif
+
 void
 syms_of_inotify (void)
 {
@@ -414,6 +472,10 @@ syms_of_inotify (void)
   defsubr (&Sinotify_rm_watch);
   defsubr (&Sinotify_valid_p);
 
+#ifdef DEBUG
+  defsubr (&Sinotify_watch_list);
+  defsubr (&Sinotify_allocated_p);
+#endif
   staticpro (&watch_list);
 
   Fprovide (intern_c_string ("inotify"), Qnil);

Apart from that, the following is a list containing all the problems
I've found that I still think are relevant.  Some of which we already
discussed earlier.

* Don't discriminate remote handler based on the local library used.
  Already discussed.

diff -u --label /home/politza/src/emacs/lisp/filenotify.el --label \#\<buffer\ 
filenotify.el\> /home/politza/src/emacs/lisp/filenotify.el 
/tmp/buffer-content-142424Gt
--- /home/politza/src/emacs/lisp/filenotify.el
+++ #<buffer filenotify.el>
@@ -342,10 +342,7 @@
        ;; file notification support.
        (setq desc (funcall
                    handler 'file-notify-add-watch
-                    ;; kqueue does not report file changes in
-                    ;; directory monitor.  So we must watch the file
-                    ;; itself.
-                    (if (eq file-notify--library 'kqueue) file dir)
+                    dir
                     flags callback))
 
       ;; Check, whether Emacs has been compiled with file notification

Diff finished.  Sun Mar 19 23:05:00 2017

* The value of file-notify--pending-event is reset after the first
  client was processed in the outer loop in file-notify-callback,
  resulting in clients watching for the same file being treated
  differently.  Note that this problem would be solved by not having
  multiple clients per descriptor.

pending-event-test.el
Description: ERT Test for pending events with 2 clients

* inotify_add_watch internally uses a single watch per directory, which
  may be shared by multiple clients (using filenotify.el).  The problem
  here seems to be that these clients may use different FLAGS as
  argument to file-notify-add-watch.  Currently, the last added client's
  FLAGS become the effective mask for the underlying C-descriptor,
  meaning that clients added before may not receive change or
  attribute-change events if the mask was modified accordingly.

* It seems to me that the right word here is "unified".

diff --git a/doc/lispref/os.texi b/doc/lispref/os.texi
index 9b6752c5e1..4f7d47305f 100644
--- a/doc/lispref/os.texi
+++ b/doc/lispref/os.texi
@@ -2707,7 +2707,7 @@ File Notifications
 
 Since all these libraries emit different events on notified file
 changes, there is the Emacs library @code{filenotify} which provides a
-unique interface.
+unified interface.
 
 @defun file-notify-add-watch file flags callback
 Add a watch for filesystem events pertaining to @var{file}.  This

-ap