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

[Michael Albinus]

Andreas Politz <politza@hochschule-trier.de> writes:

Hi Andreas,

Thanks for your hard work!

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

I agree with you, that's the best choice.

> Here is a first draft of a
> corresponding patch, let me know what you think.

I've applied the patch, and filenotify-tests.el passes all tests
(except `file-notify-test04-autorevert-remote', but that's another
story). So I believe it is OK to apply it to master, and see how it goes
(waiting for feedback).

Some comments:

> diff --git a/lisp/filenotify.el b/lisp/filenotify.el
> index 7eb6229976..beae94c2c2 100644
> --- a/lisp/filenotify.el
> +++ b/lisp/filenotify.el
> @@ -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)

In this case, we shall remove `file-notify--descriptor', and replace all
calls by the `desc' argument.

> @@ -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))

I'm not sure we can eliminate the `file' binding. I believe, it is
needed for the kqueue library. Maybe you add a TODO comment for
retesting instead.

(My virtual machine running BSD is in a bad shape. I should reanimate it.)

> @@ -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))

dito.

> diff --git a/src/inotify.c b/src/inotify.c
> index 61ef615328..302f52225e 100644
> --- a/src/inotify.c
> +++ b/src/inotify.c
> +#ifdef DEBUG

Please use a more specific flag, like INOTIFY_DEBUG.

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

In general I agree it looks like a bug. But prior removing, I would like
to retest with the kqueue library.

> * 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.

Makes sense.

> (ert-deftest file-notify-test03c-events ()
>   "Check that rename events are propagated to all watches."
>   (skip-unless (file-notify--test-local-enabled))
>   (unwind-protect
>       (progn
>         (setq file-notify--test-tmpfile (file-notify--test-make-temp-name)
>               file-notify--test-tmpfile1 (file-notify--test-make-temp-name))
>         (with-temp-file file-notify--test-tmpfile1)
>         (setq file-notify--test-desc (file-notify-add-watch
>                                       file-notify--test-tmpfile
>                                       '(change attribute-change)
>                                       #'file-notify--test-event-handler)
>               file-notify--test-desc1 (file-notify-add-watch
>                                        file-notify--test-tmpfile
>                                        '(change attribute-change)
>                                        (lambda (event)
>                                          (file-notify--test-event-handler 
> event))))
>         (file-notify--test-with-events '(renamed renamed)
>           (rename-file file-notify--test-tmpfile1
>                        file-notify--test-tmpfile))
>         (file-notify-rm-watch file-notify--test-desc)
>         (file-notify-rm-watch file-notify--test-desc1)
>         (file-notify--test-cleanup-p))
>     (file-notify--test-cleanup)))

I'm a little bit undecided, whether we shall add this as extra test
case, or whether we shall integrate it into
`file-notify-test03-events'. The former might be better, but it would
also mean that we shall break down `file-notify-test03-events' into
smaller tests.

> * 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.

I'm aware of this problem (it happens also for other libraries, I
believe). No idea yet whether it is important to fix it. But maybe you
add a TODO entry at the end of filenotify.el.

> * 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

My English is not good enough to decide what's better. But I don't
object if you want to change.

> -ap

Best regards, Michael.