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

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

[Andreas Politz]

[-- Attachment #1: Type: text/plain, Size: 606 bytes --]


The descriptor returned by file-notify-add-watch is of the form (ID . DIR)
(at least when using inotify) and these descriptors are stored in a
hash-table using equal as comparator.

If multiple clients watch the same file and one of them removes its watch
via file-notify-rm-watch, the result is unpredictable. I.e. the function
removes some watch (I believe it is the last one added), but not necessarily
the one associated with the client calling file-notify-rm-watch on the
descriptor it got from calling file-notify-add-watch.

I've attached a test case for this.

Am I missing something here ?

-ap


[-- Attachment #2: test case --]
[-- Type: application/emacs-lisp, Size: 1462 bytes --]

[-- Attachment #3: Type: text/plain, Size: 17948 bytes --]




In GNU Emacs 26.0.50 (build 1, x86_64-unknown-linux-gnu, GTK+ Version 3.22.9)
 of 2017-03-15 built on luca
Repository revision: 2f972349bdc99d5d9ebf63169c00e24b119aa38d
Windowing system distributor 'The X.Org Foundation', version 11.0.11902000
System Description:	Arch Linux

Recent messages:
Delete all tests? (y or n) y
file-notify-rm-out-of-order
Ran 1 tests, 0 results were as expected
Quit
Undo! [2 times]
Mark set
C-c C-t is undefined
Test result: FAILED ()
Quit
Mark set [2 times]

Configured using:
 'configure --prefix=/usr --sysconfdir=/etc --libexecdir=/usr/lib
 --localstatedir=/var --with-x-toolkit=gtk3 --with-xft --with-xwidgets
 'CFLAGS=-march=x86-64 -mtune=generic -O2 -pipe
 -fstack-protector-strong'
 LDFLAGS=-Wl,-O1,--sort-common,--as-needed,-z,relro
 CPPFLAGS=-D_FORTIFY_SOURCE=2'

Configured features:
XPM JPEG TIFF GIF PNG RSVG IMAGEMAGICK SOUND GPM DBUS GCONF GSETTINGS
NOTIFY ACL GNUTLS LIBXML2 FREETYPE M17N_FLT LIBOTF XFT ZLIB
TOOLKIT_SCROLL_BARS GTK3 X11 XWIDGETS LIBSYSTEMD

Important settings:
  value of $LC_MESSAGES: en_US.UTF-8
  value of $LANG: de_DE.UTF-8
  locale-coding-system: utf-8-unix

Major mode: Ert

Minor modes in effect:
  TeX-PDF-mode: t
  diff-auto-refine-mode: t
  yas-minor-mode: t
  sow-scroll-other-window-mode: t
  synclient-fix-tapping-mode: t
  show-paren-mode: t
  check-parens-mode: t
  pdf-occur-global-minor-mode: t
  override-global-mode: t
  recentf-mode: t
  shell-dirtrack-mode: t
  save-place-mode: t
  ewm-desktop-mode: t
  ewm-mode: t
  ewm-bindings-mode: t
  ewm-compat-mode: t
  savehist-mode: t
  ekey-mode: t
  desktop-save-mode: t
  tooltip-mode: t
  global-eldoc-mode: t
  eldoc-mode: t
  electric-indent-mode: t
  mouse-wheel-mode: t
  file-name-shadow-mode: t
  global-font-lock-mode: t
  font-lock-mode: t
  blink-cursor-mode: t
  auto-composition-mode: t
  auto-encryption-mode: t
  auto-compression-mode: t
  line-number-mode: t
  transient-mark-mode: t

Load-path shadows:
/home/politza/src/ewm/ewm-ruleset hides /home/politza/.emacs.d/elpa/ewm-1.0/ewm-ruleset
/home/politza/src/ewm/dev hides /home/politza/.emacs.d/elpa/ewm-1.0/dev
/home/politza/src/ewm/ewm-layout hides /home/politza/.emacs.d/elpa/ewm-1.0/ewm-layout
/home/politza/src/ewm/ewm-frame hides /home/politza/.emacs.d/elpa/ewm-1.0/ewm-frame
/home/politza/src/ewm/ewm hides /home/politza/.emacs.d/elpa/ewm-1.0/ewm
/home/politza/src/ewm/ewm-buffer hides /home/politza/.emacs.d/elpa/ewm-1.0/ewm-buffer
/home/politza/src/ewm/ewm-configuration hides /home/politza/.emacs.d/elpa/ewm-1.0/ewm-configuration
/home/politza/src/ewm/ewm-util hides /home/politza/.emacs.d/elpa/ewm-1.0/ewm-util
/home/politza/src/ewm/ewm-compat hides /home/politza/.emacs.d/elpa/ewm-1.0/ewm-compat
~/fh_notebook/politza/.emacs.d/plugins/s hides /home/politza/.emacs.d/elpa/s-20160928.636/s
~/fh_notebook/politza/.emacs.d/plugins/term hides /usr/share/emacs/26.0.50/lisp/term
~/fh_notebook/politza/.emacs.d/plugins/imenu hides /usr/share/emacs/26.0.50/lisp/imenu
~/fh_notebook/politza/.emacs.d/plugins/saveplace hides /usr/share/emacs/26.0.50/lisp/saveplace
/home/politza/.emacs.d/elpa/org-20170210/ox-md hides /usr/share/emacs/26.0.50/lisp/org/ox-md
/home/politza/.emacs.d/elpa/org-20170210/org-rmail hides /usr/share/emacs/26.0.50/lisp/org/org-rmail
/home/politza/.emacs.d/elpa/org-20170210/org-capture hides /usr/share/emacs/26.0.50/lisp/org/org-capture
/home/politza/.emacs.d/elpa/org-20170210/ob-fortran hides /usr/share/emacs/26.0.50/lisp/org/ob-fortran
/home/politza/.emacs.d/elpa/org-20170210/org-list hides /usr/share/emacs/26.0.50/lisp/org/org-list
/home/politza/.emacs.d/elpa/org-20170210/org-table hides /usr/share/emacs/26.0.50/lisp/org/org-table
/home/politza/.emacs.d/elpa/org-20170210/ob-screen hides /usr/share/emacs/26.0.50/lisp/org/ob-screen
/home/politza/.emacs.d/elpa/org-20170210/org-pcomplete hides /usr/share/emacs/26.0.50/lisp/org/org-pcomplete
/home/politza/.emacs.d/elpa/org-20170210/ob-ditaa hides /usr/share/emacs/26.0.50/lisp/org/ob-ditaa
/home/politza/.emacs.d/elpa/org-20170210/ox-texinfo hides /usr/share/emacs/26.0.50/lisp/org/ox-texinfo
/home/politza/.emacs.d/elpa/org-20170210/ob-plantuml hides /usr/share/emacs/26.0.50/lisp/org/ob-plantuml
/home/politza/.emacs.d/elpa/org-20170210/org-w3m hides /usr/share/emacs/26.0.50/lisp/org/org-w3m
/home/politza/.emacs.d/elpa/org-20170210/ob-ref hides /usr/share/emacs/26.0.50/lisp/org/ob-ref
/home/politza/.emacs.d/elpa/org-20170210/ob-lilypond hides /usr/share/emacs/26.0.50/lisp/org/ob-lilypond
/home/politza/.emacs.d/elpa/org-20170210/ox-odt hides /usr/share/emacs/26.0.50/lisp/org/ox-odt
/home/politza/.emacs.d/elpa/org-20170210/ob-latex hides /usr/share/emacs/26.0.50/lisp/org/ob-latex
/home/politza/.emacs.d/elpa/org-20170210/ox-ascii hides /usr/share/emacs/26.0.50/lisp/org/ox-ascii
/home/politza/.emacs.d/elpa/org-20170210/ob-sqlite hides /usr/share/emacs/26.0.50/lisp/org/ob-sqlite
/home/politza/.emacs.d/elpa/org-20170210/ob-lisp hides /usr/share/emacs/26.0.50/lisp/org/ob-lisp
/home/politza/.emacs.d/elpa/org-20170210/org-archive hides /usr/share/emacs/26.0.50/lisp/org/org-archive
/home/politza/.emacs.d/elpa/org-20170210/ox-latex hides /usr/share/emacs/26.0.50/lisp/org/ox-latex
/home/politza/.emacs.d/elpa/org-20170210/org-macro hides /usr/share/emacs/26.0.50/lisp/org/org-macro
/home/politza/.emacs.d/elpa/org-20170210/org-macs hides /usr/share/emacs/26.0.50/lisp/org/org-macs
/home/politza/.emacs.d/elpa/org-20170210/ob-makefile hides /usr/share/emacs/26.0.50/lisp/org/ob-makefile
/home/politza/.emacs.d/elpa/org-20170210/ob-core hides /usr/share/emacs/26.0.50/lisp/org/ob-core
/home/politza/.emacs.d/elpa/org-20170210/ox hides /usr/share/emacs/26.0.50/lisp/org/ox
/home/politza/.emacs.d/elpa/org-20170210/ob-awk hides /usr/share/emacs/26.0.50/lisp/org/ob-awk
/home/politza/.emacs.d/elpa/org-20170210/ob-js hides /usr/share/emacs/26.0.50/lisp/org/ob-js
/home/politza/.emacs.d/elpa/org-20170210/ob-table hides /usr/share/emacs/26.0.50/lisp/org/ob-table
/home/politza/.emacs.d/elpa/org-20170210/ox-man hides /usr/share/emacs/26.0.50/lisp/org/ox-man
/home/politza/.emacs.d/elpa/org-20170210/ob-dot hides /usr/share/emacs/26.0.50/lisp/org/ob-dot
/home/politza/.emacs.d/elpa/org-20170210/org-attach hides /usr/share/emacs/26.0.50/lisp/org/org-attach
/home/politza/.emacs.d/elpa/org-20170210/ox-publish hides /usr/share/emacs/26.0.50/lisp/org/ox-publish
/home/politza/.emacs.d/elpa/org-20170210/org-clock hides /usr/share/emacs/26.0.50/lisp/org/org-clock
/home/politza/.emacs.d/elpa/org-20170210/ob-matlab hides /usr/share/emacs/26.0.50/lisp/org/ob-matlab
/home/politza/.emacs.d/elpa/org-20170210/org-habit hides /usr/share/emacs/26.0.50/lisp/org/org-habit
/home/politza/.emacs.d/elpa/org-20170210/ob-picolisp hides /usr/share/emacs/26.0.50/lisp/org/ob-picolisp
/home/politza/.emacs.d/elpa/org-20170210/org-faces hides /usr/share/emacs/26.0.50/lisp/org/org-faces
/home/politza/.emacs.d/elpa/org-20170210/ob-css hides /usr/share/emacs/26.0.50/lisp/org/ob-css
/home/politza/.emacs.d/elpa/org-20170210/org-irc hides /usr/share/emacs/26.0.50/lisp/org/org-irc
/home/politza/.emacs.d/elpa/org-20170210/ob-C hides /usr/share/emacs/26.0.50/lisp/org/ob-C
/home/politza/.emacs.d/elpa/org-20170210/ob-eval hides /usr/share/emacs/26.0.50/lisp/org/ob-eval
/home/politza/.emacs.d/elpa/org-20170210/org-bbdb hides /usr/share/emacs/26.0.50/lisp/org/org-bbdb
/home/politza/.emacs.d/elpa/org-20170210/org-ctags hides /usr/share/emacs/26.0.50/lisp/org/org-ctags
/home/politza/.emacs.d/elpa/org-20170210/ob-sass hides /usr/share/emacs/26.0.50/lisp/org/ob-sass
/home/politza/.emacs.d/elpa/org-20170210/ob-perl hides /usr/share/emacs/26.0.50/lisp/org/ob-perl
/home/politza/.emacs.d/elpa/org-20170210/ox-org hides /usr/share/emacs/26.0.50/lisp/org/ox-org
/home/politza/.emacs.d/elpa/org-20170210/org-docview hides /usr/share/emacs/26.0.50/lisp/org/org-docview
/home/politza/.emacs.d/elpa/org-20170210/ob-maxima hides /usr/share/emacs/26.0.50/lisp/org/ob-maxima
/home/politza/.emacs.d/elpa/org-20170210/org-datetree hides /usr/share/emacs/26.0.50/lisp/org/org-datetree
/home/politza/.emacs.d/elpa/org-20170210/org-mobile hides /usr/share/emacs/26.0.50/lisp/org/org-mobile
/home/politza/.emacs.d/elpa/org-20170210/ox-icalendar hides /usr/share/emacs/26.0.50/lisp/org/ox-icalendar
/home/politza/.emacs.d/elpa/org-20170210/ob-octave hides /usr/share/emacs/26.0.50/lisp/org/ob-octave
/home/politza/.emacs.d/elpa/org-20170210/org-footnote hides /usr/share/emacs/26.0.50/lisp/org/org-footnote
/home/politza/.emacs.d/elpa/org-20170210/ob-scala hides /usr/share/emacs/26.0.50/lisp/org/ob-scala
/home/politza/.emacs.d/elpa/org-20170210/ob hides /usr/share/emacs/26.0.50/lisp/org/ob
/home/politza/.emacs.d/elpa/org-20170210/ob-ocaml hides /usr/share/emacs/26.0.50/lisp/org/ob-ocaml
/home/politza/.emacs.d/elpa/org-20170210/org-eshell hides /usr/share/emacs/26.0.50/lisp/org/org-eshell
/home/politza/.emacs.d/elpa/org-20170210/org-bibtex hides /usr/share/emacs/26.0.50/lisp/org/org-bibtex
/home/politza/.emacs.d/elpa/org-20170210/ob-mscgen hides /usr/share/emacs/26.0.50/lisp/org/ob-mscgen
/home/politza/.emacs.d/elpa/org-20170210/ob-haskell hides /usr/share/emacs/26.0.50/lisp/org/ob-haskell
/home/politza/.emacs.d/elpa/org-20170210/org-timer hides /usr/share/emacs/26.0.50/lisp/org/org-timer
/home/politza/.emacs.d/elpa/org-20170210/org-entities hides /usr/share/emacs/26.0.50/lisp/org/org-entities
/home/politza/.emacs.d/elpa/org-20170210/ob-clojure hides /usr/share/emacs/26.0.50/lisp/org/ob-clojure
/home/politza/.emacs.d/elpa/org-20170210/org-plot hides /usr/share/emacs/26.0.50/lisp/org/org-plot
/home/politza/.emacs.d/elpa/org-20170210/ob-io hides /usr/share/emacs/26.0.50/lisp/org/ob-io
/home/politza/.emacs.d/elpa/org-20170210/ob-java hides /usr/share/emacs/26.0.50/lisp/org/ob-java
/home/politza/.emacs.d/elpa/org-20170210/ob-calc hides /usr/share/emacs/26.0.50/lisp/org/ob-calc
/home/politza/.emacs.d/elpa/org-20170210/ob-emacs-lisp hides /usr/share/emacs/26.0.50/lisp/org/ob-emacs-lisp
/home/politza/.emacs.d/elpa/org-20170210/org-indent hides /usr/share/emacs/26.0.50/lisp/org/org-indent
/home/politza/.emacs.d/elpa/org-20170210/org-src hides /usr/share/emacs/26.0.50/lisp/org/org-src
/home/politza/.emacs.d/elpa/org-20170210/org hides /usr/share/emacs/26.0.50/lisp/org/org
/home/politza/.emacs.d/elpa/org-20170210/ob-lob hides /usr/share/emacs/26.0.50/lisp/org/ob-lob
/home/politza/.emacs.d/elpa/org-20170210/ob-shen hides /usr/share/emacs/26.0.50/lisp/org/ob-shen
/home/politza/.emacs.d/elpa/org-20170210/org-inlinetask hides /usr/share/emacs/26.0.50/lisp/org/org-inlinetask
/home/politza/.emacs.d/elpa/org-20170210/ox-beamer hides /usr/share/emacs/26.0.50/lisp/org/ox-beamer
/home/politza/.emacs.d/elpa/org-20170210/ob-asymptote hides /usr/share/emacs/26.0.50/lisp/org/ob-asymptote
/home/politza/.emacs.d/elpa/org-20170210/org-colview hides /usr/share/emacs/26.0.50/lisp/org/org-colview
/home/politza/.emacs.d/elpa/org-20170210/ob-sql hides /usr/share/emacs/26.0.50/lisp/org/ob-sql
/home/politza/.emacs.d/elpa/org-20170210/org-mhe hides /usr/share/emacs/26.0.50/lisp/org/org-mhe
/home/politza/.emacs.d/elpa/org-20170210/org-agenda hides /usr/share/emacs/26.0.50/lisp/org/org-agenda
/home/politza/.emacs.d/elpa/org-20170210/ob-python hides /usr/share/emacs/26.0.50/lisp/org/ob-python
/home/politza/.emacs.d/elpa/org-20170210/org-crypt hides /usr/share/emacs/26.0.50/lisp/org/org-crypt
/home/politza/.emacs.d/elpa/org-20170210/ob-gnuplot hides /usr/share/emacs/26.0.50/lisp/org/ob-gnuplot
/home/politza/.emacs.d/elpa/org-20170210/ob-org hides /usr/share/emacs/26.0.50/lisp/org/ob-org
/home/politza/.emacs.d/elpa/org-20170210/org-id hides /usr/share/emacs/26.0.50/lisp/org/org-id
/home/politza/.emacs.d/elpa/org-20170210/ob-ledger hides /usr/share/emacs/26.0.50/lisp/org/ob-ledger
/home/politza/.emacs.d/elpa/org-20170210/org-protocol hides /usr/share/emacs/26.0.50/lisp/org/org-protocol
/home/politza/.emacs.d/elpa/org-20170210/ox-html hides /usr/share/emacs/26.0.50/lisp/org/ox-html
/home/politza/.emacs.d/elpa/org-20170210/ob-comint hides /usr/share/emacs/26.0.50/lisp/org/ob-comint
/home/politza/.emacs.d/elpa/org-20170210/org-compat hides /usr/share/emacs/26.0.50/lisp/org/org-compat
/home/politza/.emacs.d/elpa/org-20170210/ob-R hides /usr/share/emacs/26.0.50/lisp/org/ob-R
/home/politza/.emacs.d/elpa/org-20170210/org-feed hides /usr/share/emacs/26.0.50/lisp/org/org-feed
/home/politza/.emacs.d/elpa/org-20170210/org-mouse hides /usr/share/emacs/26.0.50/lisp/org/org-mouse
/home/politza/.emacs.d/elpa/org-20170210/org-info hides /usr/share/emacs/26.0.50/lisp/org/org-info
/home/politza/.emacs.d/elpa/org-20170210/ob-keys hides /usr/share/emacs/26.0.50/lisp/org/ob-keys
/home/politza/.emacs.d/elpa/org-20170210/ob-ruby hides /usr/share/emacs/26.0.50/lisp/org/ob-ruby
/home/politza/.emacs.d/elpa/org-20170210/org-install hides /usr/share/emacs/26.0.50/lisp/org/org-install
/home/politza/.emacs.d/elpa/org-20170210/org-version hides /usr/share/emacs/26.0.50/lisp/org/org-version
/home/politza/.emacs.d/elpa/org-20170210/ob-exp hides /usr/share/emacs/26.0.50/lisp/org/ob-exp
/home/politza/.emacs.d/elpa/org-20170210/ob-scheme hides /usr/share/emacs/26.0.50/lisp/org/ob-scheme
/home/politza/.emacs.d/elpa/org-20170210/org-loaddefs hides /usr/share/emacs/26.0.50/lisp/org/org-loaddefs
/home/politza/.emacs.d/elpa/org-20170210/org-gnus hides /usr/share/emacs/26.0.50/lisp/org/org-gnus
/home/politza/.emacs.d/elpa/org-20170210/ob-tangle hides /usr/share/emacs/26.0.50/lisp/org/ob-tangle
/home/politza/.emacs.d/elpa/org-20170210/org-element hides /usr/share/emacs/26.0.50/lisp/org/org-element

Features:
(shadow sort gnus-cite mail-extr nnir nndraft nnmh utf-7 nnfolder nnnil
gnus-agent gnus-srvr gnus-score score-mode nnvirtual nntp gnus-cache
gnus-msg gnus-art mm-uu mml2015 mm-view mml-smime smime dig gnus-sum
gnus-group gnus-undo gnus-start gnus-cloud gnus-spec emacsbug sendmail
gxref apropos timezone eieio-opt speedbar sb-image ezimage dframe
reposition cl-print edebug help-fns radix-tree xref project ert debug
hippie-exp misearch multi-isearch browse-kill-ring bash colir color ivy
delsel ivy-overlay ffap mm-archive conf-mode image-file org-rmail
org-mhe org-irc org-info org-gnus org-docview doc-view org-bibtex bibtex
org-bbdb org-w3m undo-tree diff vc-dir ewoc vc autorevert filenotify
sh-script smie executable autoconf autoconf-mode vc-dispatcher vc-svn
preview prv-emacs tex-buf font-latex latex tex-ispell tex-style tex dbus
xml crm tex-mode dired-aux vc-git diff-mode map view haskell-snippets
yasnippet network-stream starttls url-http url-gw nsm url-auth sow
which-key url-cache url url-proxy url-privacy url-expand url-methods
url-history url-cookie url-domsuf url-util mailcap paren rtags popup
repeat thingatpt cc-mode cc-fonts cc-guess cc-menus cc-cmds cc-styles
cc-align cc-engine cc-vars cc-defs pdf-dev pdf-occur tablist
tablist-filter semantic/wisent/comp semantic/wisent
semantic/wisent/wisent semantic/util-modes semantic/util semantic
semantic/tag semantic/lex semantic/fw mode-local cedet pdf-isearch
let-alist pdf-misc pdf-tools compile cus-edit cus-start cus-load
pdf-view bookmark pp jka-compr pdf-cache pdf-info tq pdf-util ob-shell
ob-plantuml org-www-bookmark org-protocol org-element avl-tree org
org-macro org-footnote org-pcomplete org-list org-faces org-entities
noutline outline org-version ob-emacs-lisp ob ob-tangle org-src ob-ref
ob-lob ob-table ob-keys ob-exp ob-comint ob-core ob-eval org-compat
org-macs org-loaddefs disp-table image-mode lib-kbd gnus-nnimap-format
nnimap nnmail gnus-int gnus-range mail-source message puny rfc822 mml
mml-sec epa epg mm-decode mm-bodies mm-encode mail-parse rfc2231
mailabbrev gmm-utils mailheader tls gnutls utf7 netrc nnoo
parse-time-rfc2822 info-look gnus-win iedit iedit-lib
multiple-cursors-core rect bind-key graphviz-dot-mode recentf
tree-widget tramp tramp-compat tramp-loaddefs trampver ucs-normalize
shell pcomplete comint ring parse-time format-spec advice derived
saveplace ewm edmacro kmacro ewm-configuration ewm-frame ewm-buffer
ibuf-macs ibuf-ext ibuffer ibuffer-loaddefs ewm-ruleset ewm-layout
find-func ewm-compat gnus nnheader gnus-util rmail rmail-loaddefs
rfc2047 rfc2045 ietf-drums mail-utils mm-util mail-prsvr wid-edit
cal-menu calendar cal-loaddefs calc calc-loaddefs calc-macs ewm-util
savehist server tsdh-dark-theme ekey dash dired-imenu imenu dired-x
dired dired-loaddefs desktop frameset easy-mmode ansi-color finder-inf
tex-site cl info package epg-config url-handlers url-parse auth-source
cl-seq eieio eieio-core cl-macs eieio-loaddefs password-cache url-vars
seq byte-opt subr-x gv bytecomp byte-compile cl-extra help-mode easymenu
cconv cl-loaddefs pcase cl-lib time-date mule-util tooltip eldoc
electric uniquify ediff-hook vc-hooks lisp-float-type mwheel term/x-win
x-win term/common-win x-dnd tool-bar dnd fontset image regexp-opt fringe
tabulated-list replace newcomment text-mode elisp-mode lisp-mode
prog-mode register page menu-bar rfn-eshadow isearch timer select
scroll-bar mouse jit-lock font-lock syntax facemenu font-core
term/tty-colors frame cl-generic cham georgian utf-8-lang misc-lang
vietnamese tibetan thai tai-viet lao korean japanese eucjp-ms cp51932
hebrew greek romanian slovak czech european ethiopic indian cyrillic
chinese composite charscript case-table epa-hook jka-cmpr-hook help
simple abbrev obarray minibuffer cl-preloaded nadvice loaddefs button
faces cus-face macroexp files text-properties overlay sha1 md5 base64
format env code-pages mule custom widget hashtable-print-readable
backquote dbusbind inotify dynamic-setting system-font-setting
font-render-setting xwidget-internal move-toolbar gtk x-toolkit x
multi-tty make-network-process emacs)

Memory information:
((conses 16 1130920 1240746)
 (symbols 48 72148 48)
 (miscs 40 1245 8418)
 (strings 32 198145 372410)
 (string-bytes 1 7746812)
 (vectors 16 94275)
 (vector-slots 8 2251986 397758)
 (floats 8 590 1337)
 (intervals 56 63082 4479)
 (buffers 968 342))

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 the bug report.

> The descriptor returned by file-notify-add-watch is of the form (ID . DIR)
> (at least when using inotify) and these descriptors are stored in a
> hash-table using equal as comparator.

Yes.

> If multiple clients watch the same file and one of them removes its watch
> via file-notify-rm-watch, the result is unpredictable. I.e. the function
> removes some watch (I believe it is the last one added), but not necessarily
> the one associated with the client calling file-notify-rm-watch on the
> descriptor it got from calling file-notify-add-watch.

With the returned descriptors, it cannot be decided which watch has to
be removed, because both descriptors are equal. So we could decide
either to remove all watches for a given file if such a descriptor is
passed to `file-notify-rm-watch', or we must adapt
`file-notify-add-watch' to return different descriptors. I'm undecided
yet.

Note, that this problem seems to be specific to inotify. Other libraries
do not suffer from this problem, I believe.

> I've attached a test case for this.

Thanks. I've added this, modified, to `file-notify-test02-rm-watch' for
my internal tests. Will be pushed to master once I have a solution for
the problem.

> -ap

Best regards, Michael.

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

[Andreas Politz]

Michael Albinus <michael.albinus@gmx.de> writes:

> With the returned descriptors, it cannot be decided which watch has to
> be removed, because both descriptors are equal. So we could decide
> either to remove all watches for a given file if such a descriptor is
> passed to `file-notify-rm-watch', or we must adapt
> `file-notify-add-watch' to return different descriptors. I'm undecided
> yet.

Surely, we want the ability of two or more different packages being able
to watch the same file, without interfering with each other.  Why are
you undecided ?

I could try and find a solution, if you don't have time or anything.

-ap

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

[Michael Albinus]

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

Hi Andreas,

> Michael Albinus <michael.albinus@gmx.de> writes:
>
>> With the returned descriptors, it cannot be decided which watch has to
>> be removed, because both descriptors are equal. So we could decide
>> either to remove all watches for a given file if such a descriptor is
>> passed to `file-notify-rm-watch', or we must adapt
>> `file-notify-add-watch' to return different descriptors. I'm undecided
>> yet.
>
> Surely, we want the ability of two or more different packages being able
> to watch the same file, without interfering with each other.  Why are
> you undecided ?

I need to dig into inotify.c. IIRC, it wasn't simple to achieve this;
that's why we have this crude solution. Maybe it could be solved only on
filenotify.el level, don't remember the details.

> I could try and find a solution, if you don't have time or anything.

I don't have time, indeed. If you want to work on this, I would
appreciate. You could always ping me if you have questions.

Pls don't forget, whatever you propose must work for at least 6
different backends: inotify, kqueue, gfilenotify, w32notify and the
remote gvfs-monitor-dir and inotifywait. Heavy tests, always.

> -ap

Best regards, Michael.

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

[Andreas Politz]

Michael Albinus <michael.albinus@gmx.de> writes:

> I need to dig into inotify.c. IIRC, it wasn't simple to achieve this;
> that's why we have this crude solution. Maybe it could be solved only on
> filenotify.el level, don't remember the details.

What I would do, is box the internal descriptor in file-notify-add-watch,
to make it unique to the caller and unbox it in file-notify-rm-watch and
file-notify-valid-p.  

A different question is, whether file-notify-add-watch should behave
like add-hook with regards to adding a function multiple times.  I don't
think it does at the moment, so I probably leave it as such (?).

-ap

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

[Michael Albinus]

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

> What I would do, is box the internal descriptor in file-notify-add-watch,
> to make it unique to the caller and unbox it in file-notify-rm-watch and
> file-notify-valid-p.  

Sounds OK to me. Do you want to do this only for inotify, or also for
the other backends?

> A different question is, whether file-notify-add-watch should behave
> like add-hook with regards to adding a function multiple times.  I don't
> think it does at the moment, so I probably leave it as such (?).

I believe adding the *same* function twice for the same file/directory
doesn't make sense. Maybe we should document this in the Elisp manual,
and test it also in `file-notify-test01-add-watch'.

> -ap

Best regards, Michael.

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

[Andreas Politz]

Michael Albinus <michael.albinus@gmx.de> writes:

> Andreas Politz <politza@hochschule-trier.de> writes:
>> A different question is, whether file-notify-add-watch should behave
>> like add-hook [...]
>
> I believe adding the *same* function twice for the same file/directory
> doesn't make sense. Maybe we should document this in the Elisp manual,
> and test it also in `file-notify-test01-add-watch'.

There is a check for this in file-notify-add-watch:

    (unless (member entry (cdr registered))
      (puthash desc ... file-notify-descriptors))

So, I should probably stay this way. (Even so, one might favor it being
differently, because file-notify-add-watch returns a cookie and thus has
more like a transactional semantics, for lack of a better word).

I took a deeper look filenotify.el and found some problems/have some
further questions. Note that I use inotify.

+ A watched hard-link for some other file may not receive its events,
  due to string-equal being used for file-comparisons.  Shouldn't
  file-equal be used instead ?

+ Watching a /dir/file may receive events (e.g. touch /dir) for dir.

+ Why the seemingly arbitrary exclusion of backup-files in
  file-notify-callback ?  What if someone wants to track the creation of
  said files ?

+ Why is the existence of kqueue checked for the handler in
  file-notify-add-watch ? After all we don't know how this handler will
  operate.

-ap

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

[Andreas Politz]

I have another question:

Do you remember what this comment from file-notify-callback is talking
about ?

#+BEGIN_SRC emacs-lisp
((memq action '(moved rename))
 ;; The kqueue rename event does not return file1 in
 ;; case a file monitor is established.
 (if (setq file1 (file-notify--event-file1-name event))
     'renamed 'deleted))
#+END_SRC

And file-notify--event-file1-name basically returns nil, if the file1
argument of event is not a string.  But this seems to contradict the
comment, which states that file1 *is* nil, implying that the stored
filename should be used, or not ?

-ap

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

[Michael Albinus]

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

Hi Andreas,

> I took a deeper look filenotify.el and found some problems/have some
> further questions. Note that I use inotify.
>
> + A watched hard-link for some other file may not receive its events,
>   due to string-equal being used for file-comparisons.  Shouldn't
>   file-equal be used instead ?

How does inotify (and the other libraries) work in this case? Does it
support hard links? We should not add more logic than the native
libraries offer.

> + Watching a /dir/file may receive events (e.g. touch /dir) for dir.

Could you pls give an example?

> + Why the seemingly arbitrary exclusion of backup-files in
>   file-notify-callback ?  What if someone wants to track the creation of
>   said files ?

When a file under supervision is renamed during backup, the supervision
might be stopped. This case must be handled.

> + Why is the existence of kqueue checked for the handler in
>   file-notify-add-watch ? After all we don't know how this handler will
>   operate.

Why don't we know what kqueue does?

> -ap

Best rewgards, Michael.

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

[Michael Albinus]

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

Hi Andreas,

> Do you remember what this comment from file-notify-callback is talking
> about ?
>
> #+BEGIN_SRC emacs-lisp
> ((memq action '(moved rename))
>  ;; The kqueue rename event does not return file1 in
>  ;; case a file monitor is established.
>  (if (setq file1 (file-notify--event-file1-name event))
>      'renamed 'deleted))
> #+END_SRC

kqueue is the oldest of he supported native libraries. It is the only
one (IIRC), which does not return source and target file name in case of
a rename. That's why that the kqueue `rename' event is transformed into
a `deleted' event. This is what the comment is speaking about.

I admit it would be more understandable if there would be an overview of
the events and their arguments, the different libraries are sending.

> And file-notify--event-file1-name basically returns nil, if the file1
> argument of event is not a string.  But this seems to contradict the
> comment, which states that file1 *is* nil, implying that the stored
> filename should be used, or not ?

The fourth slot of an event structure could be either a string (file1)
or a cookie, or nil. This is tried to check.

> -ap

Best regards, Michael.

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

[Andreas Politz]

Michael Albinus <michael.albinus@gmx.de> writes:

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

>> + A watched hard-link for some other file may not receive its events,
>>   due to string-equal being used for file-comparisons.  Shouldn't
>>   file-equal be used instead ?
>
> How does inotify (and the other libraries) work in this case? Does it
> support hard links? We should not add more logic than the native
> libraries offer.

inotify seems to be inode-based, so it emits notifications independent
of the filename used.  I think that hard-links are not commonly used
under win32, but I don't know.

I we stay with string-equal, we are watching filenames, not files.
Which is probably sufficient/OK.  Maybe this should be mentioned in the
manual, I made a note of it.

>
>> + Watching a /dir/file may receive events (e.g. touch /dir) for dir.
>
> Could you pls give an example?

With inotify: 

(let ((desc (file-notify-add-watch
             "/tmp/file" '(attribute-change)
             (lambda (_) (cl-assert nil)))))
  (unwind-protect
      (progn
        (shell-command "touch /tmp")
        (sit-for 3))
    (file-notify-rm-watch desc)))

>> + Why the seemingly arbitrary exclusion of backup-files in
>>   file-notify-callback ?  What if someone wants to track the creation of
>>   said files ?
>
> When a file under supervision is renamed during backup, the supervision
> might be stopped. This case must be handled.

Yes, I see.

>> + Why is the existence of kqueue checked for the handler in
>>   file-notify-add-watch ? After all we don't know how this handler will
>>   operate.
>
> Why don't we know what kqueue does?

This:

    (if handler
	;; A file name handler could exist even if there is no local
	;; 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)
                    flags callback))


Why should we assume that handler is somehow related to kqueue ? I think
its just a copy and paste error.  The comment should be moved to the
actual library call below this.

I also wonder, if the passed argument should not always be the filename
for which the watch was requested, as opposed to its directory.  After
all we should not make assumptions about the abilities of the underlying
mechanism.  For example it could work similar to kqueue, i.e. with an
inability to watch directories.

Thanks for you response,
-ap

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

[Andreas Politz]

Michael Albinus <michael.albinus@gmx.de> writes:

> [...] the kqueue `rename' event is transformed into a `deleted' event

OK, I misunderstood the comment then.

> The fourth slot of an event structure could be either a string (file1)
> or a cookie, or nil. This is tried to check.

Yes, I see.

-ap

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

[Michael Albinus]

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

Hi Andreas,

> inotify seems to be inode-based, so it emits notifications independent
> of the filename used.  I think that hard-links are not commonly used
> under win32, but I don't know.

Yes, inotify watches inodes.

> I we stay with string-equal, we are watching filenames, not files.
> Which is probably sufficient/OK.  Maybe this should be mentioned in the
> manual, I made a note of it.

The docstring speaks about filesystems. This is a hint, that the
behaviour of the libraries might differ.

>>> + Watching a /dir/file may receive events (e.g. touch /dir) for dir.
>>
>> Could you pls give an example?
>
> With inotify: 
>
> (let ((desc (file-notify-add-watch
>              "/tmp/file" '(attribute-change)
>              (lambda (_) (cl-assert nil)))))
>   (unwind-protect
>       (progn
>         (shell-command "touch /tmp")
>         (sit-for 3))
>     (file-notify-rm-watch desc)))

Yes, I remember. For all backends except kqueue, we watch the
directory. This is intended.

>>> + Why is the existence of kqueue checked for the handler in
>>>   file-notify-add-watch ? After all we don't know how this handler will
>>>   operate.
>>
>> Why don't we know what kqueue does?
>
> This:
>
>     (if handler
> 	;; A file name handler could exist even if there is no local
> 	;; 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)
>                     flags callback))
>
> Why should we assume that handler is somehow related to kqueue ? I think
> its just a copy and paste error.  The comment should be moved to the
> actual library call below this.

This looks like an error, indeed. I will check.

> I also wonder, if the passed argument should not always be the filename
> for which the watch was requested, as opposed to its directory.  After
> all we should not make assumptions about the abilities of the underlying
> mechanism.  For example it could work similar to kqueue, i.e. with an
> inability to watch directories.

We've discussed this years ago, maybe you find it in the archives. There
are problems when you watch only the file. This doesn't work for example
when you want to watch a file which does not exist yet. Or which
disappears, and reappears.

The agreement was to watch the upper directory. This works for all
backends except kqueue.

> Thanks for you response,
> -ap

Best regards, Michael.

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

[Andreas Politz]

Michael Albinus <michael.albinus@gmx.de> writes:

>>>> + Watching a /dir/file may receive events (e.g. touch /dir) for dir.

> Yes, I remember. For all backends except kqueue, we watch the
> directory. This is intended.
>

Sure, the back-ends mostly watch directories, except for kqueue.  But is
this behavior also intended to be propagated to the clients of
filenotify.el ?

>>>> + Why is the existence of kqueue checked for the handler in
>>>>   file-notify-add-watch ? After all we don't know how this handler will
>>>>   operate.

>> I also wonder, if the passed argument should not always be the filename
>> for which the watch was requested, as opposed to its directory.  After
>> all we should not make assumptions about the abilities of the underlying
>> mechanism.  For example it could work similar to kqueue, i.e. with an
>> inability to watch directories.
>
> We've discussed this years ago, maybe you find it in the archives. There
> are problems when you watch only the file. This doesn't work for example
> when you want to watch a file which does not exist yet. Or which
> disappears, and reappears.
>
> The agreement was to watch the upper directory. This works for all
> backends except kqueue.

Sorry, for not being clear: I was exclusively talking about
file-name-handler here.  Passing the intended filename is more general
then passing the directory only.  Think of some program foonotify, which
is similarly limited like kqueue.  Granted, this scenario probably won't
come up very soon.

-ap

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

[Michael Albinus]

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

> Sure, the back-ends mostly watch directories, except for kqueue.  But is
> this behavior also intended to be propagated to the clients of
> filenotify.el ?

Yes.

>> We've discussed this years ago, maybe you find it in the archives. There
>> are problems when you watch only the file. This doesn't work for example
>> when you want to watch a file which does not exist yet. Or which
>> disappears, and reappears.
>>
>> The agreement was to watch the upper directory. This works for all
>> backends except kqueue.
>
> Sorry, for not being clear: I was exclusively talking about
> file-name-handler here.  Passing the intended filename is more general
> then passing the directory only.  Think of some program foonotify, which
> is similarly limited like kqueue.  Granted, this scenario probably won't
> come up very soon.

We have only two different programs for the remote case, gvfs-monitor-dir
and inotifywait. Coincidentally, I'm also the Tramp maintainer, so I
know what I am saying :-)

> -ap

Best regards, Michael.

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

[Andreas Politz]

[-- Attachment #1: Type: text/plain, Size: 689 bytes --]


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.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: inotify.diff --]
[-- Type: text/x-diff, Size: 12977 bytes --]

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

[-- Attachment #3: Type: text/plain, Size: 250 bytes --]


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.

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #4: remote-handler.diff --]
[-- Type: text/x-diff, Size: 768 bytes --]

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

[-- Attachment #5: Type: text/plain, Size: 303 bytes --]


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


[-- Attachment #6: ERT Test for pending events with 2 clients --]
[-- Type: application/emacs-lisp, Size: 1346 bytes --]

[-- Attachment #7: Type: text/plain, Size: 534 bytes --]


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


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #8: os.texi diff --]
[-- Type: text/x-diff, Size: 482 bytes --]

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

[-- Attachment #9: Type: text/plain, Size: 5 bytes --]


-ap

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

[Andreas Politz]

Michael Albinus <michael.albinus@gmx.de> writes:

> Andreas Politz <politza@hochschule-trier.de> writes:
>
>> Sure, the back-ends mostly watch directories, except for kqueue.  But is
>> this behavior also intended to be propagated to the clients of
>> filenotify.el ?
>
> Yes.

Why is that ?

-ap

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

[Michael Albinus]

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

>>> Sure, the back-ends mostly watch directories, except for kqueue.  But is
>>> this behavior also intended to be propagated to the clients of
>>> filenotify.el ?
>>
>> Yes.
>
> Why is that ?

We've discussed this a while ago, main reason (IIRC) was to achieve
same behaviour for the different libraries.

I cannot find the discussion just now, likely it was in one of the
bugs. The main reason was the w32notify library, which works only
watching directories.

Later on we've added the kqueue library, which works reliably only
watching files. This breaks the unification attempt, but I don't believe
we shall change the behaviour again.

> -ap

Best regards, Michael.

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.

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

[Andreas Politz]

Michael Albinus <michael.albinus@gmx.de> writes:

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

>> 1. Change inotify.c and make it return/receive a unique descriptor per client.
>
> I agree with you, that's the best choice.
>

Ok.

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

Let me work on this a little more. I think, I'm not removing the
descriptors in inotify.c correctly.

>
> Some comments:
>
>> diff --git a/lisp/filenotify.el b/lisp/filenotify.el

>> -(defun file-notify--descriptor (desc file)
>> +(defun file-notify--descriptor (desc _file)

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

Yes, and since (with the patch added) we now have a one-to-one relation
between clients and descriptors across all implementations, we could
also simplify the hash values.


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

Shouldn't be, since kqueue, w32notify and gfilenotify all return a
pointer wrapped in a Lisp-Integer, i.e. for these back-ends the file
value was already nil all the time.

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

We should have a server, doing this sort of thing.

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

Will do.

>> (ert-deftest file-notify-test03c-events () [...]


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

I think it would be better to split those tests into smaller units.  For
once it makes it easier to determine which should-form actually failed.
And secondly, it makes it easier to add a new test (especially for
people not to familiar with the code), without being anxious about
interfering with existing ones.

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

I think, it is important.  For example, auto-revert's file-notify
mechanism (using '(change attribute-change) as flags) would break, if
some other package decides to watch the same file, but for
attribute-changes only.

It seems to me that this only affects inotify, since all other back-ends
return a newly created descriptor, but I haven't explicitly checked
this.

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

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

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

I would translate it in this context as "einzigartig"
vs. "vereinheitlicht".  Native speakers to the rescue !

-ap

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

[Eli Zaretskii]

> From: Michael Albinus <michael.albinus@gmx.de>
> Date: Tue, 21 Mar 2017 09:44:52 +0100
> Cc: 26126@debbugs.gnu.org
> 
> Andreas Politz <politza@hochschule-trier.de> writes:
> 
> >>> Sure, the back-ends mostly watch directories, except for kqueue.  But is
> >>> this behavior also intended to be propagated to the clients of
> >>> filenotify.el ?
> >>
> >> Yes.
> >
> > Why is that ?
> 
> We've discussed this a while ago, main reason (IIRC) was to achieve
> same behaviour for the different libraries.

That discussion was about local notifications, where indeed the
situation is like you describe.

But Andreas asks about calling remote handlers, about which we by
definition know much less.  In that context, it might indeed make
sense to pass the file, not its parent directory, because the handler
can easily reconstruct the parent directory if that's what it needs.
By contrast, there's no way for the handler to intuit the file which
was stripped.

WDYT?

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

[Eli Zaretskii]

> From: Andreas Politz <politza@hochschule-trier.de>
> Date: Tue, 21 Mar 2017 16:06:22 +0100
> Cc: 26126@debbugs.gnu.org
> 
> >> * It seems to me that the right word here is "unified".
> 
> >>  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.
> 
> > My English is not good enough to decide what's better. But I don't
> > object if you want to change.
> 
> I would translate it in this context as "einzigartig"
> vs. "vereinheitlicht".  Native speakers to the rescue !

I'm not a native speaker of English, but "unified" is the correct word
here.

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

[Andreas Politz]

Michael Albinus <michael.albinus@gmx.de> writes:

> Andreas Politz <politza@hochschule-trier.de> writes:
>
>>>> Sure, the back-ends mostly watch directories, except for kqueue.  But is
>>>> this behavior also intended to be propagated to the clients of
>>>> filenotify.el ?

> We've discussed this a while ago, main reason (IIRC) was to achieve
> same behaviour for the different libraries.
>
> I cannot find the discussion just now, likely it was in one of the
> bugs. The main reason was the w32notify library, which works only
> watching directories.
>
> Later on we've added the kqueue library, which works reliably only
> watching files. This breaks the unification attempt, but I don't believe
> we shall change the behaviour again.

Ok, I try to find it, but note that this behavior was introduced with
the following (kqueue-only) commit

commit 7bf54d01159eb09bae3c9cd86f2af0812d9afdf6
Author: Michael Albinus <michael.albinus@gmx.de>
Date:   Fri Jan 22 19:56:09 2016 +0100

    Backport kqueue integration from master


Maybe we can work on unifying the behavior across back-ends at a later
time, while taking some pointers from other projects, e.g.
https://github.com/emcrisostomo/fswatch .

If it stays this way, it should probably be documented.

-ap

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

[Andreas Politz]

Eli Zaretskii <eliz@gnu.org> writes:

> But Andreas asks about calling remote handlers, about which we by
> definition know much less.  In that context, it might indeed make
> sense to pass the file, not its parent directory, because the handler
> can easily reconstruct the parent directory if that's what it needs.
> By contrast, there's no way for the handler to intuit the file which
> was stripped.
>
> WDYT?

Actually, I was talking about both aspects and incidentally, in this
context about the other one.  I refer to packages using filenotify.el as
clients.

But, yes, what you're expressing, is what I was trying to get at before.

-ap

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

[Michael Albinus]

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

Hi Andreas,

>> Later on we've added the kqueue library, which works reliably only
>> watching files. This breaks the unification attempt, but I don't believe
>> we shall change the behaviour again.
>
> Ok, I try to find it, but note that this behavior was introduced with
> the following (kqueue-only) commit
>
> commit 7bf54d01159eb09bae3c9cd86f2af0812d9afdf6
> Author: Michael Albinus <michael.albinus@gmx.de>
> Date:   Fri Jan 22 19:56:09 2016 +0100
>
>     Backport kqueue integration from master

My FreeBSD 10 VM has been reanimated. Last time I've used it was June
2016, so everything is dusty there.

As time permits, I'll check the kqueue vs remote handlers status.

> Maybe we can work on unifying the behavior across back-ends at a later
> time, while taking some pointers from other projects, e.g.
> https://github.com/emcrisostomo/fswatch .

Looks interesting, but I have no idea about its state. Last commit is
from 23 Jul 2016; is it still maintained actively?

A similar approach like fswatch is the already integrated gfilenotify
library. Honestly, this is the most problematic backend.

> -ap

Best regards, Michael.

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

[Michael Albinus]

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

Hi Andreas,

>> 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).
>
> Let me work on this a little more. I think, I'm not removing the
> descriptors in inotify.c correctly.

OK. Take your time.

>> 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.
>
> Shouldn't be, since kqueue, w32notify and gfilenotify all return a
> pointer wrapped in a Lisp-Integer, i.e. for these back-ends the file
> value was already nil all the time.

We shall recheck, once your changed inotify implementation has hit the repo.

>> 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.
>
> I think it would be better to split those tests into smaller units.  For
> once it makes it easier to determine which should-form actually failed.
> And secondly, it makes it easier to add a new test (especially for
> people not to familiar with the code), without being anxious about
> interfering with existing ones.

Likely yes. I have the same feeling, but haven't done due to lack of
energy and time.

For the time being, I have added a modified version of your test
removing watch descriptors out of order to
`file-notify-test02-rm-watch'. Since this fails for inotify, I've added
an :expected-result tag to this test. Can be removed, when fixed in
inotify.c.

>>> * 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.
>
> I think, it is important.  For example, auto-revert's file-notify
> mechanism (using '(change attribute-change) as flags) would break, if
> some other package decides to watch the same file, but for
> attribute-changes only.
>
> It seems to me that this only affects inotify, since all other back-ends
> return a newly created descriptor, but I haven't explicitly checked
> this.

So I would let it for you to implement it in inotify.c. When there is
also a respective test, we will see whether it is a problem for other
backends, too.

> -ap

Best regards, Michael.

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

[Michael Albinus]

Eli Zaretskii <eliz@gnu.org> writes:

>> We've discussed this a while ago, main reason (IIRC) was to achieve
>> same behaviour for the different libraries.
>
> That discussion was about local notifications, where indeed the
> situation is like you describe.
>
> But Andreas asks about calling remote handlers, about which we by
> definition know much less.

Nope. We know exactly which remote handlers are called, and how they
behave. Do you expect other implementations of remote handlers? I don't,
because I'm not aware of other candidates but inotifywait and gvfs-monitor-dir.

> In that context, it might indeed make sense to pass the file, not its
> parent directory, because the handler can easily reconstruct the
> parent directory if that's what it needs.  By contrast, there's no way
> for the handler to intuit the file which was stripped.
>
> WDYT?

I still don't understand what's the difference between local and remote
events in your eyes. I've tried to implement remote handlers to behave
exactly like the local ones. That's the Tramp philosophy.

Best regards, Michael.

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

[Eli Zaretskii]

> From: Michael Albinus <michael.albinus@gmx.de>
> Cc: politza@hochschule-trier.de,  26126@debbugs.gnu.org
> Date: Wed, 22 Mar 2017 14:23:47 +0100
> 
> > But Andreas asks about calling remote handlers, about which we by
> > definition know much less.
> 
> Nope. We know exactly which remote handlers are called, and how they
> behave.

We know that about our handlers, yes.  But that doesn't have to be the
end of the story.  Emacs is extensible.

> Do you expect other implementations of remote handlers?

Yes, why not?  It's much easier to do that in Lisp than in C, where
the local handlers should be implemented.

> > In that context, it might indeed make sense to pass the file, not its
> > parent directory, because the handler can easily reconstruct the
> > parent directory if that's what it needs.  By contrast, there's no way
> > for the handler to intuit the file which was stripped.
> >
> > WDYT?
> 
> I still don't understand what's the difference between local and remote
> events in your eyes.

See above.  Admittedly, this is a minor point, so not worth arguing if
you disagree with my POV.

> I've tried to implement remote handlers to behave exactly like the
> local ones. That's the Tramp philosophy.

Right, but in this case there are 2 flavors of local handlers, and the
question is on which of them to model the remote ones.

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

[Michael Albinus]

Eli Zaretskii <eliz@gnu.org> writes:

>> I've tried to implement remote handlers to behave exactly like the
>> local ones. That's the Tramp philosophy.
>
> Right, but in this case there are 2 flavors of local handlers, and the
> question is on which of them to model the remote ones.

IIRC, it was driven by you to let all handlers behave like directory
monitors. The kqueue case came later, and it is the only exception as of
today.

II also RC, there was the idea to offer another function, which returns
the type of a monitor, being a file monitor or a directory monitor. This
idea was given up after all monitors were forced to behave like a
directory monitor; prior the existence of kqueue. Maybe we shall rethink
about this idea? Is this of practical use?

Best regards, Michael.

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

[Eli Zaretskii]

> From: Michael Albinus <michael.albinus@gmx.de>
> Cc: politza@hochschule-trier.de,  26126@debbugs.gnu.org
> Date: Wed, 22 Mar 2017 17:01:13 +0100
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> >> I've tried to implement remote handlers to behave exactly like the
> >> local ones. That's the Tramp philosophy.
> >
> > Right, but in this case there are 2 flavors of local handlers, and the
> > question is on which of them to model the remote ones.
> 
> IIRC, it was driven by you to let all handlers behave like directory
> monitors.

Yes, that's right.  But I was surely only thinking about local
handlers.

> II also RC, there was the idea to offer another function, which returns
> the type of a monitor, being a file monitor or a directory monitor. This
> idea was given up after all monitors were forced to behave like a
> directory monitor; prior the existence of kqueue. Maybe we shall rethink
> about this idea? Is this of practical use?

I wouldn't start working on this before there's a real need.

Thanks.

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

[Michael Albinus]

Eli Zaretskii <eliz@gnu.org> writes:

>> II also RC, there was the idea to offer another function, which returns
>> the type of a monitor, being a file monitor or a directory monitor. This
>> idea was given up after all monitors were forced to behave like a
>> directory monitor; prior the existence of kqueue. Maybe we shall rethink
>> about this idea? Is this of practical use?
>
> I wouldn't start working on this before there's a real need.

Agreed, until somebody shows the real need for this distinction.

> Thanks.

Best regards, Michael.

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

[Andreas Politz]

Michael Albinus <michael.albinus@gmx.de> writes:

> A similar approach like fswatch is the already integrated gfilenotify
> library. Honestly, this is the most problematic backend.

In terms of "unintegratedness", i.e. does behave differently depending
on the back-end used ? (I know from the tests that it may use polling
every 5 secs.)

-ap

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

[Andreas Politz]

Michael Albinus <michael.albinus@gmx.de> writes:

> Andreas Politz <politza@hochschule-trier.de> writes:
>>>> * 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.  [...]

> So I would let it for you to implement it in inotify.c. When there is
> also a respective test, we will see whether it is a problem for other
> backends, too.

This touches the question whether we're operating under the assumption,
that other Lisp-code (apart from filenotify.el) may freely call
inotify-add-watch or not.  Because if it does, this has to be handled in
inotify.c.  Otherwise we may handle it in filenotify.el .

If I do it in inotify.c: Is it possible to store a bit-mask (of type
uint32_t) in a Lisp_Object ? Since this would be the easiest make this
change.

-ap

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

[Michael Albinus]

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

Hi Andreas,

>> A similar approach like fswatch is the already integrated gfilenotify
>> library. Honestly, this is the most problematic backend.
>
> In terms of "unintegratedness", i.e. does behave differently depending
> on the back-end used ? (I know from the tests that it may use polling
> every 5 secs.)

Yes, this was also a problem. Sometimes, the promised polling on mounted
filesystems didn't work reliably.

There were also bugs which have restricted use of gfilenotify.

And there are also reservations by people, who do not want to use glib /
gio infrastructure.

Due to the existence of inotify and kqueue monitors in Emacs, the only
major use of gfilenotify seems to be in Cygwin.

> -ap

Best regards, Michael.

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

[Michael Albinus]

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

Hi Andreas,

> This touches the question whether we're operating under the assumption,
> that other Lisp-code (apart from filenotify.el) may freely call
> inotify-add-watch or not.  Because if it does, this has to be handled in
> inotify.c.  Otherwise we may handle it in filenotify.el .

It is still possible to call inotify-add-watch and companions. However,
we don't force this use; the Elisp manual decribes only filenoty.el
functionality.

And I don't know any example people are using initofy-add-watch directly.

> If I do it in inotify.c: Is it possible to store a bit-mask (of type
> uint32_t) in a Lisp_Object ? Since this would be the easiest make this
> change.

Sure, but Emacs integers guarantee you only 30 bits. But why do you want
to do this? You could map your bit mask into a complex Lisp object, a
vector with 32 slots, or whatever. This makes access/manipulation in
Lisp much more simple.

> -ap

Best regards, Michael.

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

[Michael Albinus]

Michael Albinus <michael.albinus@gmx.de> writes:

Hi Andreas,

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

I've tested now with kqueue. There was an error indeed; I've fixed this
in master.

There are still some other issues running filenotify-tests.el; looks
like I should spend more time for kqueue related tests.

>> -ap

Best regards, Michael.

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

[Eli Zaretskii]

> From: Michael Albinus <michael.albinus@gmx.de>
> Date: Wed, 22 Mar 2017 19:57:10 +0100
> Cc: 26126@debbugs.gnu.org
> 
> > This touches the question whether we're operating under the assumption,
> > that other Lisp-code (apart from filenotify.el) may freely call
> > inotify-add-watch or not.  Because if it does, this has to be handled in
> > inotify.c.  Otherwise we may handle it in filenotify.el .
> 
> It is still possible to call inotify-add-watch and companions. However,
> we don't force this use; the Elisp manual decribes only filenoty.el
> functionality.

IMO, Lisp programs should always go through filenotify.el, never via
backend-specific APIs.

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

[Michael Albinus]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Michael Albinus <michael.albinus@gmx.de>
>> Date: Wed, 22 Mar 2017 19:57:10 +0100
>> Cc: 26126@debbugs.gnu.org
>> 
>> > This touches the question whether we're operating under the assumption,
>> > that other Lisp-code (apart from filenotify.el) may freely call
>> > inotify-add-watch or not.  Because if it does, this has to be handled in
>> > inotify.c.  Otherwise we may handle it in filenotify.el .
>> 
>> It is still possible to call inotify-add-watch and companions. However,
>> we don't force this use; the Elisp manual decribes only filenoty.el
>> functionality.
>
> IMO, Lisp programs should always go through filenotify.el, never via
> backend-specific APIs.

Then we shall emphasize this in the manual. Like this?

--8<---------------cut here---------------start------------->8---
diff --git a/doc/lispref/os.texi b/doc/lispref/os.texi
index 9b6752c..09f7c18 100644
--- a/doc/lispref/os.texi
+++ b/doc/lispref/os.texi
@@ -2707,7 +2707,8 @@ 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.  It is highly recommended to use this library
+instead of the native ones.
 
 @defun file-notify-add-watch file flags callback
 Add a watch for filesystem events pertaining to @var{file}.  This
--8<---------------cut here---------------end--------------->8---

Best regards, Michael.

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

[Eli Zaretskii]

> From: Michael Albinus <michael.albinus@gmx.de>
> Cc: politza@hochschule-trier.de,  26126@debbugs.gnu.org
> Date: Thu, 23 Mar 2017 08:36:39 +0100
> 
> > IMO, Lisp programs should always go through filenotify.el, never via
> > backend-specific APIs.
> 
> Then we shall emphasize this in the manual. Like this?
> 
> --8<---------------cut here---------------start------------->8---
> diff --git a/doc/lispref/os.texi b/doc/lispref/os.texi
> index 9b6752c..09f7c18 100644
> --- a/doc/lispref/os.texi
> +++ b/doc/lispref/os.texi
> @@ -2707,7 +2707,8 @@ 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.  It is highly recommended to use this library
> +instead of the native ones.

I'd say that even more definitively:

  Lisp programs that want to receive file notifications should always
  use this library in preference to the native ones.

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

[Michael Albinus]

Eli Zaretskii <eliz@gnu.org> writes:

> I'd say that even more definitively:
>
>   Lisp programs that want to receive file notifications should always
>   use this library in preference to the native ones.

Committed to master.

Best regards, Michael.

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

[Andreas Politz]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Michael Albinus <michael.albinus@gmx.de>

>> Do you expect other implementations of remote handlers?
>
> Yes, why not?  It's much easier to do that in Lisp than in C, where
> the local handlers should be implemented.

I just wanted to add, that file-handler do not necessarily have to refer
to files, e.g. think of a /proc kind of handler making internal Emacs
state available via filenames. 

-ap

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

[Andreas Politz]

[-- Attachment #1: Type: text/plain, Size: 1546 bytes --]


Hey !

Below is a second version of the previous patch.  It is somewhat
conservative, since neither did I attempt to

+ further simplify filenotify.el nor

+ handle differing masks in inotify.c .

--

I also thought about the test-cases and more generally about how to
develop a specification for this library, i.e. how do we want this to
behave.  Do we have the desire that it works uniformly across all
participating back-ends ? And is that even possible ?

I think it is to easy to adapt the tests for each back-end, until they
succeed and thereby potentially masking actual bugs.

One way to go about this would be to write a series of definitive
unit-tests which specify the intended behavior. Then, allow them to fail
for a specific back-end, until someone has fixed potential bugs for it
and confirmed that the test succeeds.  This would allow for an
incremental improvement on fairly solid grounds.  I'm assuming that
people of the future are interested in improving their used back-end
(e.g. make kqueue watch directories properly, if that is possible).

Anyway, I was bored today, so I took a look at what events these
libraries actually produce, the result of which you may also find below.

Finally, I'm tempted to suggest to get rid of the flags argument of
file-notify-add-watch.  As it is, things are already complicated enough
and we don't seem to have many people working on this.  I think we could
make it backward-compatible to a certain degree.  Note also, that many
file operations trigger both kinds of events anyway.

--


[-- Attachment #2: A patch --]
[-- Type: test/x-patch, Size: 19290 bytes --]

[-- Attachment #3: Type: text/plain, Size: 5 bytes --]


--


[-- Attachment #4: filenotify as is --]
[-- Type: text/plain, Size: 14598 bytes --]

                            inotify                         w32notify                       kqueue                          gfilenotify                     gvfs-monitor-dir                inotifywait
----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
file-create     file        created file                    created file                    skipped                         created file                    created file                    created file
                            deleted file                    deleted file                                                    deleted file                    attr-changed file               attr-changed file
                            stopped file                    stopped file                                                    stopped file                    deleted file                    deleted file
                                                                                                                                                            stopped file                    stopped file

----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
file-read       file        deleted file                    deleted file                    deleted file                    deleted file                    deleted file                    deleted file
                            stopped file                    stopped file                    stopped file                    stopped file                    stopped file                    stopped file

----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
file-write      file        changed file                    changed file                    attr-changed file               changed file                    changed file                    changed file
                            deleted file                    changed file                    changed file                    changed file                    changed file                    changed file
                            stopped file                    deleted file                    deleted file                    deleted file                    attr-changed file               attr-changed file
                                                            stopped file                    stopped file                    stopped file                    deleted file                    deleted file
                                                                                                                                                            stopped file                    stopped file

----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
file-attrib     file        attr-changed file               changed file                    attr-changed file               attr-changed file               attr-changed file               attr-changed file
                            deleted file                    deleted file                    deleted file                    deleted file                    deleted file                    deleted file
                            stopped file                    stopped file                    stopped file                    stopped file                    stopped file                    stopped file

----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
file-rename     dir         renamed dir/src dir/dest        deleted dir/dest                deleted dir                     renamed dir/src dir/dest        renamed dir/src dir/dest        renamed dir/src dir/dest
                            created dir/dest                renamed dir/src dir/dest        stopped dir                     deleted dir/dest                attr-changed dir/dest           attr-changed dir/dest
                            deleted dir/dest                timedout 12                                                     deleted dir                     attr-changed dir/dest           attr-changed dir/dest
                            deleted dir                     stopped dir                                                     stopped dir                     deleted dir/dest                deleted dir/dest
                            stopped dir                                                                                                                     deleted dir                     deleted dir
                                                                                                                                                            stopped dir                     stopped dir

                dir/src     deleted dir                     timedout 12                     deleted dir/src                 renamed dir/src dir/dest        renamed dir/src dir/dest        renamed dir/src dir/dest
                            stopped dir/src                 stopped dir/src                 stopped dir/src                 stopped dir/src                 stopped dir/src                 stopped dir/src
                                                                                            deleted dir/src

                dir/dest    renamed dir/src dir/dest        deleted dir/dest                deleted dir/dest                renamed dir/src dir/dest        renamed dir/src dir/dest        renamed dir/src dir/dest
                            deleted dir/dest                stopped dir/dest                stopped dir/dest                deleted dir/dest                attr-changed dir/dest           attr-changed dir/dest
                            stopped dir/dest                                                                                stopped dir/dest                attr-changed dir/dest           attr-changed dir/dest
                                                                                                                                                            deleted dir/dest                deleted dir/dest
                                                                                                                                                            stopped dir/dest                stopped dir/dest

----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
dir-create      dir         created dir                     created dir                     skipped                         created dir                     created dir                     created anon-1
                            deleted dir                     deleted dir                                                     deleted dir                     deleted dir                     deleted anon-1
                            stopped dir                     stopped dir                                                     stopped dir                     stopped dir                     timedout 12
                                                                                                                                                                                            stopped dir

----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
dir-read        dir         deleted dir                     timedout 12                     deleted dir                     deleted dir                     deleted dir                     deleted dir
                            stopped dir                     stopped dir                     stopped dir                     stopped dir                     stopped dir                     stopped dir

----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
dir-create-file dir         deleted dir/file                deleted dir/file                deleted dir                     deleted dir/file                deleted dir/file                deleted dir/file
                            deleted dir                     timedout 12                     stopped dir                     deleted dir                     deleted dir                     deleted dir
                            stopped dir                     stopped dir                                                     stopped dir                     stopped dir                     stopped dir

                dir/file    deleted dir/file                deleted dir/file                deleted dir/file                deleted dir/file                deleted dir/file                deleted dir/file
                            stopped dir/file                stopped dir/file                stopped dir/file                stopped dir/file                stopped dir/file                stopped dir/file

----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
dir-create-dir  dir         deleted dir/dir                 deleted dir/dir                 deleted dir                     deleted dir/dir                 deleted dir/dir                 deleted dir
                            deleted dir                     timedout 12                     stopped dir                     deleted dir                     deleted dir                     stopped dir
                            stopped dir                     stopped dir                                                     stopped dir                     stopped dir

                dir/dir     deleted dir/dir                 timedout 12                     deleted dir/dir                 deleted dir/dir                 deleted dir/dir                 deleted dir/dir
                            stopped dir/dir                 stopped dir/dir                 stopped dir/dir                 stopped dir/dir                 stopped dir/dir                 stopped dir/dir

----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
dir-attrib      dir         attr-changed dir                timedout 12                     attr-changed dir                attr-changed dir                attr-changed dir                attr-changed dir
                            deleted dir                     stopped dir                     deleted dir                     deleted dir                     deleted dir                     deleted dir
                            stopped dir                                                     stopped dir                     stopped dir                     stopped dir                     stopped dir

----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
dir-rename      dir         renamed dir/src dir/dest        deleted dir/src                 skipped                         renamed dir/src dir/dest        deleted dir/src                 renamed dir anon-1
                            deleted dir                     timedout 12                                                     deleted dir                     deleted dir                     deleted dir
                            stopped dir                     stopped dir                                                     stopped dir                     stopped dir                     stopped dir

                dir/src     deleted dir/src                 timedout 12                     skipped                         deleted dir/src                 deleted dir/src                 deleted dir/src
                            stopped dir/src                 stopped dir/src                                                 stopped dir/src                 stopped dir/src                 stopped dir/src

                dir/dest    renamed dir/src dir/dest        created dir/dest                skipped                         created dir/dest                created dir/dest                attr-changed anon-1
                            deleted dir/dest                deleted dir/dest                                                deleted dir/dest                attr-changed dir/dest           attr-changed anon-1
                            stopped dir/dest                stopped dir/dest                                                stopped dir/dest                attr-changed dir/dest           deleted anon-1
                                                                                                                                                            deleted dir/dest                deleted anon-1
                                                                                                                                                            stopped dir/dest                timedout 12
                                                                                                                                                                                            stopped dir/dest

----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------

;; Local Variables:
;; truncate-lines: t
;; End:

[-- Attachment #5: Type: text/plain, Size: 5 bytes --]


-ap

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

[Eli Zaretskii]

> From: Andreas Politz <politza@hochschule-trier.de>
> Date: Fri, 24 Mar 2017 21:44:28 +0100
> Cc: 26126@debbugs.gnu.org
> 
> I also thought about the test-cases and more generally about how to
> develop a specification for this library, i.e. how do we want this to
> behave.  Do we have the desire that it works uniformly across all
> participating back-ends ?

In general, yes.

> And is that even possible ?

Almost.  Some corner cases (like what happens when the watched
directory is deleted) behave differently.

> One way to go about this would be to write a series of definitive
> unit-tests which specify the intended behavior. Then, allow them to fail
> for a specific back-end, until someone has fixed potential bugs for it
> and confirmed that the test succeeds.

That's how these tests were developed.  What you see is the result; if
you have specific comments about some known failures, please spell
them out.

> Anyway, I was bored today, so I took a look at what events these
> libraries actually produce, the result of which you may also find below.

Thanks, but I have difficulty reading it.  Could you please provide a
short legend?

> Finally, I'm tempted to suggest to get rid of the flags argument of
> file-notify-add-watch.  As it is, things are already complicated enough
> and we don't seem to have many people working on this.  I think we could
> make it backward-compatible to a certain degree.  Note also, that many
> file operations trigger both kinds of events anyway.

The flags are there for the operations where the differences matter.

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

[Andreas Politz]

Eli Zaretskii <eliz@gnu.org> writes:

> Thanks, but I have difficulty reading it.  Could you please provide a
> short legend?

Sorry, I forgot to do that.

The first column is the name of the test.  The 2nd lists all watched
entities mapped to symbolic names.  Further columns list the events for
specific back ends, which may refer to the same names.

Because kqueue does not support watching nonexistent files, in most
cases I did create them before starting to watch. That's why there are
usually no creation events.

In every test, I deleted all participating files as the last step.  A
`(timeout n)' event means just that: We waited for n seconds without
receiving a stopped event.

If the event lists a name anon-xyz, it means that it contained the
filename of a non-watched file.  This may signify a bug.

All test ran with the last patched I've posted applied.

>> Finally, I'm tempted to suggest to get rid of the flags argument of
>> file-notify-add-watch. 

> The flags are there for the operations where the differences matter.

When does it matter ? The callback can just ignore the events its not
interested in.  The sole advantage would be reduced complexity, but, of
course, it shouldn't be done, if this is not seen as a sufficient reason.

Also: I think in the end we want to add a layer above filenotify.el,
with the added ability of multiplexing events in a directory to multiple
watches, watching files in that directory.  The current implementation
(including the patch) returns a new descriptor for every watch.  This is
good for a low-level api, because it is easy to manage.

The downside is, that it does not scale very well.   For example every
tramp watch starts a new process and (it seems to me) every w32 watch a
new thread.

And, coming back to the original point, in this case we usually need to
watch with all flags enabled anyway.

-ap

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

[Michael Albinus]

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

Hi Andreas,

> I just wanted to add, that file-handler do not necessarily have to refer
> to files, e.g. think of a /proc kind of handler making internal Emacs
> state available via filenames. 

I don't understand. File name handlers in Emacs provide alternative
implementations of basic file manipulation operations, see the list in
(info "(elisp) Magic File Names")

Do you propose to add a new file name handler for the "/proc" virtual
file system?

> -ap

Best regards, Michael.

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

[Andreas Politz]

Michael Albinus <michael.albinus@gmx.de> writes:

> Do you propose to add a new file name handler for the "/proc" virtual
> file system?

No, I propose that it is possible to do that.

And it seems that this is already a reality, e.g. url-file-handler.

-ap

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

[Michael Albinus]

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

> Hey !

Hi Andreas,

> Below is a second version of the previous patch.  It is somewhat
> conservative, since neither did I attempt to
>
> + further simplify filenotify.el nor
>
> + handle differing masks in inotify.c .

Thanks for this. Next time you provide a patch, could you pls merge
emacs recent changes from master first? Your patch was rejected partly,
and I had to apply rejected hunks manually. Comments:

> diff --git a/lisp/filenotify.el b/lisp/filenotify.el
> index 7eb6229976..5dea67b580 100644
> --- a/lisp/filenotify.el
> +++ b/lisp/filenotify.el
> @@ -25,6 +25,20 @@
>  ;; file notification packages `inotify', `kqueue', `gfilenotify' and
>  ;; `w32notify'.
>
> +;; TODO:

Pls move TODOs at the end of the file.

> +;; * "inotify_add_watch adds a new watch, or modifies an existing watch"
> +;;   We need to make sure that different watches for the same directory
> +;;   don't set the mask in a conflicting way regarding changed/attribute-changed
> +;; * Also check which other inotify flags are problematic
> +;;   for concurrent use of the underlying descriptor

Well, I had always the hope to modify inotify watches in this case. If
there is a watch with flags f1, and a new watch for the same file is
requested with flags f2, and f2 contains a flag which is not part of f1,
then either the existing watch shall be adapted, or the existing watch
shall be removed, and a new shall be installed. Don't know what's
possible in inotify.

> @@ -48,16 +62,14 @@ file-notify-descriptors
>
>    (DIR (FILE . CALLBACK) (FILE . CALLBACK) ...)
>
> -Several values for a given DIR happen only for `inotify', when
> -different files from the same directory are watched.")
> +Several values for a given DIR should currently not occur.")

Remove "currently". Every docstring speaks about current state.

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

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

In both functions I believe we don't need to bind `file'. The code could
be simplified, because (or (not file) ...) always succeeds.

Your changes look good; "make -C test filenotify-tests
SELECTOR='$(SELECTOR_DEFAULT)'" passes all tests. Even if there is room
for improvement I believe you could push your patch to master now, in
order to get feedback from other developers.

> I also thought about the test-cases and more generally about how to
> develop a specification for this library, i.e. how do we want this to
> behave.  Do we have the desire that it works uniformly across all
> participating back-ends ? And is that even possible ?

As Eli said, that's the intention. But we cannot reach this goal
completely due to the different behaviour of the libraries.

> I think it is to easy to adapt the tests for each back-end, until they
> succeed and thereby potentially masking actual bugs.

That's what file-notify-test.el intends to do. Well, the code has
evolved over the time, and it is somehow hard to read.  Improvements are
welcome!

> One way to go about this would be to write a series of definitive
> unit-tests which specify the intended behavior. Then, allow them to fail
> for a specific back-end, until someone has fixed potential bugs for it
> and confirmed that the test succeeds.  This would allow for an
> incremental improvement on fairly solid grounds.  I'm assuming that
> people of the future are interested in improving their used back-end
> (e.g. make kqueue watch directories properly, if that is possible).

Could you show an example how this shall look like?

> Anyway, I was bored today, so I took a look at what events these
> libraries actually produce, the result of which you may also find below.

Thanks; I'll review it next days.

> Finally, I'm tempted to suggest to get rid of the flags argument of
> file-notify-add-watch.  As it is, things are already complicated enough
> and we don't seem to have many people working on this.  I think we could
> make it backward-compatible to a certain degree.  Note also, that many
> file operations trigger both kinds of events anyway.

I agree with you. I haven't seen any different use of the flags yet (but
I maybe wrong).

Best regards, Michael.

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

[Michael Albinus]

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

> And it seems that this is already a reality, e.g. url-file-handler.

Which works also over file names, like "http://host/path/to/file".

> -ap

Best regards, Michael.

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

[Eli Zaretskii]

> From: Andreas Politz <politza@hochschule-trier.de>
> Cc: michael.albinus@gmx.de,  26126@debbugs.gnu.org
> Date: Sat, 25 Mar 2017 09:57:52 +0100
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> > Thanks, but I have difficulty reading it.  Could you please provide a
> > short legend?
> 
> Sorry, I forgot to do that.
> 
> The first column is the name of the test.  The 2nd lists all watched
> entities mapped to symbolic names.  Further columns list the events for
> specific back ends, which may refer to the same names.

That still leaves a lot of unclear entries there.  maybe it isn't
important.

> >> Finally, I'm tempted to suggest to get rid of the flags argument of
> >> file-notify-add-watch. 
> 
> > The flags are there for the operations where the differences matter.
> 
> When does it matter ?

Why should we care?  That's for the programmer to decide; we just give
them the tools.

> Also: I think in the end we want to add a layer above filenotify.el,
> with the added ability of multiplexing events in a directory to multiple
> watches, watching files in that directory.

That was the original plan, AFAIR, but this facility still has too few
users to try abstracting that higher layer, IMO.

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

[Andreas Politz]

[-- Attachment #1: Type: text/plain, Size: 2199 bytes --]

Michael Albinus <michael.albinus@gmx.de> writes:

> Andreas Politz <politza@hochschule-trier.de> writes:
>> + further simplify filenotify.el nor
>>
>> + handle differing masks in inotify.c .
>
> Thanks for this. Next time you provide a patch, could you pls merge
> emacs recent changes from master first? Your patch was rejected partly,
> and I had to apply rejected hunks manually. Comments:

Sorry, I forgot to pull.

Anyway, here is the more progressive version of the patch, adding both
of the above points.  (I guess, I'm to conservative sometimes and/or
seeing only problems everywhere.)

>> +;; * "inotify_add_watch adds a new watch, or modifies an existing watch"
>> +;;   We need to make sure that different watches for the same directory
>> +;;   don't set the mask in a conflicting way regarding changed/attribute-changed
>> +;; * Also check which other inotify flags are problematic
>> +;;   for concurrent use of the underlying descriptor
>
> Well, I had always the hope to modify inotify watches in this case. If
> there is a watch with flags f1, and a new watch for the same file is
> requested with flags f2, and f2 contains a flag which is not part of f1,
> then either the existing watch shall be adapted, or the existing watch
> shall be removed, and a new shall be installed. Don't know what's
> possible in inotify.

I implemented it by always using constantly watching for all events
(IN_ALL_EVENTS) and storing the given user-flags with the callback etc.
When an event occurs, I check whether it matches the given mask.

For that to work, I had to restrict the flag-usage by the user to those
not having an effect on the shared descriptor.

I also added IN_EXCL_UNLINK as a default.  This avoids reporting events
for already deleted filenames, which are still opened by some process,
which seems what we want as a default.  

> Your changes look good; "make -C test filenotify-tests
> SELECTOR='$(SELECTOR_DEFAULT)'" passes all tests. Even if there is room
> for improvement I believe you could push your patch to master now, in
> order to get feedback from other developers.

I ran the non-expensive tests for inotify, kqueue and they succeeded.

I have no push privileges.


[-- Attachment #2: Draft 3 --]
[-- Type: test/x-patch, Size: 37369 bytes --]

[-- Attachment #3: Type: text/plain, Size: 5 bytes --]


-ap

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

[Andreas Politz]

Michael Albinus <michael.albinus@gmx.de> writes:

> That's what file-notify-test.el intends to do. Well, the code has
> evolved over the time, and it is somehow hard to read.  Improvements are
> welcome!

OK, I'm going to work a little bit on this.

-ap

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

[Andreas Politz]

Michael Albinus <michael.albinus@gmx.de> writes:

> Andreas Politz <politza@hochschule-trier.de> writes:
>
>> And it seems that this is already a reality, e.g. url-file-handler.
>
> Which works also over file names, like "http://host/path/to/file".

If you think that's a file, how about this:

(url-handler-mode)
(find-file "http://www.google.com/?s=foo")
(file-notify-add-watch "http://www.google.com/?s=foo" '(change) #'ignore)
=> File notification error: "Directory does not exist", "http://www.google.com"

or this

(hypothetical-emacsfs-mode)
(find-file "/efs/recentf/some-recent-file")
(find-file "/efs/bookmarks/some-bookmark")

-ap

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

[Andreas Politz]

[-- Attachment #1: Type: text/plain, Size: 737 bytes --]

Eli Zaretskii <eliz@gnu.org> writes:

> That still leaves a lot of unclear entries there.  maybe it isn't
> important.

Mmh. I attached the file which generated the table.

>> >> Finally, I'm tempted to suggest to get rid of the flags argument of
>> >> file-notify-add-watch. 
>> 
>> > The flags are there for the operations where the differences matter.
>> 
>> When does it matter ?
>
> Why should we care?  That's for the programmer to decide; we just give
> them the tools.

But this is not a restriction on the part of the programmer, as he can
still filter out the events he's not interested in.

>> Also: I think in the end we want to add a layer above filenotify.el,

> That was the original plan, ...

That's good to hear.

-ap


[-- Attachment #2: check-filenotify.el --]
[-- Type: application/emacs-lisp, Size: 15769 bytes --]

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

[Michael Albinus]

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

>> Which works also over file names, like "http://host/path/to/file".
>
> If you think that's a file, how about this:

I don't think it's a file; beause I don't know anything about it. I
think it's a file name, which makes a difference.

> (url-handler-mode)
> (find-file "http://www.google.com/?s=foo")
> (file-notify-add-watch "http://www.google.com/?s=foo" '(change) #'ignore)
> => File notification error: "Directory does not exist", "http://www.google.com"

What's wrong with this? `url-handler-mode' hasn't implemented
`file-notify-add-watch'. In such cases, the original implementation is
applied. And `file-notify-add-watch' is right in saying that this
"directory" doesn't exist, from its pov.

> or this
>
> (hypothetical-emacsfs-mode)
> (find-file "/efs/recentf/some-recent-file")
> (find-file "/efs/bookmarks/some-bookmark")

No problem. Write a file name handler which takes responsibility for all
files whose names start with "/efs", and here we are. It would still be
a handler which is called based on the file *name*.

> -ap

Best regards, Michael.

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

[Michael Albinus]

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

> Anyway, here is the more progressive version of the patch, adding both
> of the above points.  (I guess, I'm to conservative sometimes and/or
> seeing only problems everywhere.)

Thanks!

>> Well, I had always the hope to modify inotify watches in this case. If
>> there is a watch with flags f1, and a new watch for the same file is
>> requested with flags f2, and f2 contains a flag which is not part of f1,
>> then either the existing watch shall be adapted, or the existing watch
>> shall be removed, and a new shall be installed. Don't know what's
>> possible in inotify.
>
> I implemented it by always using constantly watching for all events
> (IN_ALL_EVENTS) and storing the given user-flags with the callback etc.
> When an event occurs, I check whether it matches the given mask.

Sounds good.

> For that to work, I had to restrict the flag-usage by the user to those
> not having an effect on the shared descriptor.

What does this mean in practice? Any restriction we need to document?

> I also added IN_EXCL_UNLINK as a default.  This avoids reporting events
> for already deleted filenames, which are still opened by some process,
> which seems what we want as a default.  

OK.

> I have no push privileges.

I'm willing to push the patch in your name, if you provide me a ChangeLog
style commit message. For the future, I recommend to obtain push privileges.

Some nitpicks:

> --- a/lisp/filenotify.el
> +++ b/lisp/filenotify.el
> +(defun file-notify--watch-absolute-filename (watch)

This deserves a docstring.

> +handler.  The value in the hash table is file-notify--watch
> +struct.")

Please quote `file-notify--watch'.

>  (defun file-notify--rm-descriptor (descriptor)
> +DESCRIPTOR should be an object returned by
> +`file-notify-add-watch'.  If it is registered in
> +`file-notify-descriptors', a stopped event is sent."

Don't reformat the docstring, keep the first line as complete sentence.

> -      (dolist (action actions)

> +      (while actions
> +        (let ((action (pop actions)))

Being curious: why did you change this?

> --- a/src/inotify.c
> +++ b/src/inotify.c
> @@ -264,10 +360,6 @@ close
>  The following symbols can also be added to a list of aspects:
>  
>  dont-follow
> -excl-unlink
> -mask-add
> -oneshot
> -onlydir

Maybe we shall say explicitely, that those inotify events are not supported.

> -COOKIE is an object that can be compared using `equal' to identify two matching
> +COOKIE is an object that can be compared using `equal' to identify two matchingt

Typo.

> -ap

Best regards, Michael.

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

[Andreas Politz]

Michael Albinus <michael.albinus@gmx.de> writes:

> Andreas Politz <politza@hochschule-trier.de> writes:
>
>>> Which works also over file names, like "http://host/path/to/file".
>>
>> If you think that's a file, how about this:
>
> I don't think it's a file; beause I don't know anything about it. 

Then I'm confused, since I didn't mean to suggest that file-name-handler
work on anything other then strings.

>> (file-notify-add-watch "http://www.google.com/?s=foo" '(change) #'ignore)
>> => File notification error: "Directory does not exist", "http://www.google.com"
>
> What's wrong with this? `url-handler-mode' hasn't implemented
> `file-notify-add-watch'. 

Yes, but the point is how would you ?  By restricting watches to
directories, you are also forcing every handler to have a concept of
such an entity and to be able to watch it; rather then the original
"filename".

>> (hypothetical-emacsfs-mode)

> No problem.

Let's backtrack.  

I was suggesting that providing the given filename as an argument to the
file-handler is more general than using it's directory.  Your point as I
understood it, was, that the existing file-notify-add-watch
tramp-handler work just fine with directories, therefore such a change
is not needed.

My argument is that there may be other ways of using the
file-name-handler machinery, and I was presenting some examples.

I also fail to see any disadvantages (of using the given filename).

-ap

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

[Andreas Politz]

Michael Albinus <michael.albinus@gmx.de> writes:

>> For that to work, I had to restrict the flag-usage by the user to those
>> not having an effect on the shared descriptor.
>
> What does this mean in practice? Any restriction we need to document?

I don't think so, apart from inotify-add-watch's doc-string.  It means a
user of inotify can't use the following flags

IN_EXCL_UNLINK - already mentioned
IN_MASK_ADD - modifies the (shared) descriptor
IN_ONESHOT - monitor for one event only
IN_ONLYDIR - From the man page:
"Watch  pathname  only if it is a directory.  Using this flag provides an applica‐
tion with a race-free way of ensuring that the monitored object is a directory."

This sounded esoteric enough for it to be excluded, i.e. I don't know
the exact behavior on a already existing descriptor and it does not
seam to be useful for our use-case.

> I'm willing to push the patch in your name, if you provide me a ChangeLog
> style commit message. 

OK, will do.

> This deserves a docstring.
>
> Please quote `file-notify--watch'.
>
> Don't reformat the docstring, keep the first line as complete
> sentence.

OK.

>> -      (dolist (action actions)
>
>> +      (while actions
>> +        (let ((action (pop actions)))
>
> Being curious: why did you change this?

actions is set to nil at one point inside the loop, but dolist creates
an alias for it, such that setting the variable would have no effect.

>> -excl-unlink
>> -mask-add
>> -oneshot
>> -onlydir
>
> Maybe we shall say explicitely, that those inotify events are not supported.

> Typo.

OK.

Thanks for the feedback,

-ap

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

[Andreas Politz]

Michael Albinus <michael.albinus@gmx.de> writes:

>>  (defun file-notify--rm-descriptor (descriptor)
>> +DESCRIPTOR should be an object returned by
>> +`file-notify-add-watch'.  If it is registered in
>> +`file-notify-descriptors', a stopped event is sent."
>
> Don't reformat the docstring, keep the first line as complete sentence.

It's the second line:

 (defun file-notify--rm-descriptor (descriptor)
   "Remove DESCRIPTOR from `file-notify-descriptors'.
-DESCRIPTOR should be an object returned by `file-notify-add-watch'.
+DESCRIPTOR should be an object returned by
 
-ap

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

[Michael Albinus]

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

>>> (file-notify-add-watch "http://www.google.com/?s=foo" '(change) #'ignore)
>>> => File notification error: "Directory does not exist",
>>> "http://www.google.com"
>>
>> What's wrong with this? `url-handler-mode' hasn't implemented
>> `file-notify-add-watch'. 
>
> Yes, but the point is how would you ?  By restricting watches to
> directories, you are also forcing every handler to have a concept of
> such an entity and to be able to watch it; rather then the original
> "filename".

The error message says, that the upper directory of
"http://www.google.com/?s=foo", "http://www.google.com", does not
exist. This would be a problem for any file notification library. This
does not mean that the handler is forced to watch a directory only.

>>> (hypothetical-emacsfs-mode)
>
>> No problem.
>
> Let's backtrack.  
>
> I was suggesting that providing the given filename as an argument to the
> file-handler is more general than using it's directory.  Your point as I
> understood it, was, that the existing file-notify-add-watch
> tramp-handler work just fine with directories, therefore such a change
> is not needed.
>
> My argument is that there may be other ways of using the
> file-name-handler machinery, and I was presenting some examples.
>
> I also fail to see any disadvantages (of using the given filename).

Then I completely misunderstood your argumentation. What's your point
with (hypothetical-emacsfs-mode) ?

> -ap

Best regards, Michael.

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

[Michael Albinus]

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

>> Don't reformat the docstring, keep the first line as complete sentence.
>
> It's the second line:

Ah, yes. But still, the docstring doesn't need to be
reformatted. Nitpick, as said ...

> -ap

Best regards, Michael.

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

[Andreas Politz]

Michael Albinus <michael.albinus@gmx.de> writes:

> This does not mean that the handler is forced to watch a directory
> only.

But it is:

#+BEGIN_SRC emacs-lisp
(defun file-notify-add-watch (file flags callback)
  ;;....
  (let* ((handler (find-file-name-handler file 'file-notify-add-watch))
	 (dir (directory-file-name
	       (if (file-directory-p file)
		   file
		 (file-name-directory file))))
         desc func l-flags)

    (unless (file-directory-p dir)
      (signal 'file-notify-error `("Directory does not exist" ,dir)))

    (if handler
	;; A file name handler could exist even if there is no local
	;; file notification support.
	(setq desc (funcall handler 'file-notify-add-watch dir flags callback))
;;---------------------------------------------------------^
#+END_SRC

-ap

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

[Michael Albinus]

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

> Michael Albinus <michael.albinus@gmx.de> writes:
>
>> This does not mean that the handler is forced to watch a directory
>> only.
>
> But it is:

I was speaking solely about the message.

And yes, the file name handlers watch a directory. I wouldn't change
this, until there is the need to support a file name handler which
cannot implement it.

> -ap

Best regards, Michael.

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

[Paul Eggert]

[-- Attachment #1: Type: text/plain, Size: 333 bytes --]

Thanks for writing and reviewing that code. I looked at inotify.c after 
the patch was applied and found a few races and integer-overflow issues, 
most of which have likely been lurking there for a while. I fixed the 
problems that I found by installing the attached patch into master; 
please give it a try when you have the time.


[-- Attachment #2: 0001-Some-inotify-cleanup.txt --]
[-- Type: text/plain, Size: 17801 bytes --]

From 68d70c1634132423da8070c06a3024738c904f83 Mon Sep 17 00:00:00 2001
From: Paul Eggert <eggert@cs.ucla.edu>
Date: Thu, 30 Mar 2017 11:08:23 -0700
Subject: [PATCH] Some inotify cleanup
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

This catches some problems with integer overflow and races
that I noticed in inotify.c after reviewing the changes
installed to fix Bug#26126.
* src/fns.c, src/lisp.h (equal_no_quit): Now extern.
* src/inotify.c (aspect_to_inotifymask):
Check for cycles and for improper lists.
(make_lispy_mask, lispy_mask_match_p): Remove.
All callers changed to use INTEGER_TO_CONS and CONS_TO_INTEGER.
(inotifyevent_to_event, add_watch):
Don’t assume watch descriptors and cookies fit in fixnums.
(add_watch): Use assoc_no_quit, not Fassoc.
Avoid integer overflow in (very!) long-running processes where
the Emacs watch ID could overflow.  Avoid some duplicate code.
(find_descriptor): New function.
(remove_descriptor): First arg is now the returned value from
find_descriptor, rather than the descriptor.  This way, the
value can be removed without calling Fdelete, which might quit.
Wait until the end (when watch_list is consistent) before signaling
any errors.
(remove_watch, inotify_callback):
Use find_descriptor to avoid the need for Fdelete.
(inotify_callback): Use simpler tests for ioctl failure.
Free temporary buffer if signaled, and put it on the stack if small.
Use ssize_t to index through read results, to avoid a cast.
(valid_watch_descriptor): New function, with a tighter check.
(Finotify_rm_watch, Finotify_valid_p): Use it.
(Finotify_valid_p): Use assoc_no_quit and ass_no_quit instead
of Fassoc.  Do not assume the first assoc succeeds.
* test/src/inotify-tests.el (inotify-valid-p-simple):
Add inotify-valid-p tests, some of which dump core without
the fixes noted above.
---
 src/fns.c                 |   3 +-
 src/inotify.c             | 250 +++++++++++++++++++++++++---------------------
 src/lisp.h                |   1 +
 test/src/inotify-tests.el |   9 ++
 4 files changed, 149 insertions(+), 114 deletions(-)

diff --git a/src/fns.c b/src/fns.c
index 42e2eec..de7fc1b 100644
--- a/src/fns.c
+++ b/src/fns.c
@@ -38,7 +38,6 @@ along with GNU Emacs.  If not, see <http://www.gnu.org/licenses/>.  */
 
 static void sort_vector_copy (Lisp_Object, ptrdiff_t,
 			      Lisp_Object *restrict, Lisp_Object *restrict);
-static bool equal_no_quit (Lisp_Object, Lisp_Object);
 enum equal_kind { EQUAL_NO_QUIT, EQUAL_PLAIN, EQUAL_INCLUDING_PROPERTIES };
 static bool internal_equal (Lisp_Object, Lisp_Object,
 			    enum equal_kind, int, Lisp_Object);
@@ -2121,7 +2120,7 @@ of strings.  (`equal' ignores text properties.)  */)
    Use this only on arguments that are cycle-free and not too large and
    are not window configurations.  */
 
-static bool
+bool
 equal_no_quit (Lisp_Object o1, Lisp_Object o2)
 {
   return internal_equal (o1, o2, EQUAL_NO_QUIT, 0, Qnil);
diff --git a/src/inotify.c b/src/inotify.c
index 004689b..a0a89aa 100644
--- a/src/inotify.c
+++ b/src/inotify.c
@@ -41,16 +41,16 @@ along with GNU Emacs.  If not, see <http://www.gnu.org/licenses/>.  */
 #ifndef IN_ONLYDIR
 # define IN_ONLYDIR 0
 #endif
-#define INOTIFY_DEFAULT_MASK (IN_ALL_EVENTS|IN_EXCL_UNLINK)
+#define INOTIFY_DEFAULT_MASK (IN_ALL_EVENTS | IN_EXCL_UNLINK)
 
 /* File handle for inotify.  */
 static int inotifyfd = -1;
 
 /* Alist of files being watched.  We want the returned descriptor to
    be unique for every watch, but inotify returns the same descriptor
-   for multiple calls to inotify_add_watch with the same file.  In
-   order to solve this problem, we add a ID, uniquely identifying a
-   watch/file combination.
+   WD for multiple calls to inotify_add_watch with the same file.
+   Supply a nonnegative integer ID, so that WD and ID together
+   uniquely identify a watch/file combination.
 
    For the same reason, we also need to store the watch's mask and we
    can't allow the following flags to be used.
@@ -60,12 +60,21 @@ static int inotifyfd = -1;
    IN_ONESHOT
    IN_ONLYDIR
 
-   Format: (descriptor . ((id filename callback mask) ...))
-*/
+   Each element of this list is of the form (DESCRIPTOR . WATCHES)
+   where no two DESCRIPTOR values are the same.  DESCRIPTOR represents
+   the inotify watch descriptor and WATCHES is a list with elements of
+   the form (ID FILENAME CALLBACK MASK), where ID is the integer
+   described above, FILENAME names the file being watched, CALLBACK is
+   invoked when the event occurs, and MASK represents the aspects
+   being watched.  The WATCHES list is sorted by ID.  Although
+   DESCRIPTOR and MASK are ordinarily integers, they are conses when
+   representing integers outside of fixnum range.  */
+
 static Lisp_Object watch_list;
 
 static Lisp_Object
-mask_to_aspects (uint32_t mask) {
+mask_to_aspects (uint32_t mask)
+{
   Lisp_Object aspects = Qnil;
   if (mask & IN_ACCESS)
     aspects = Fcons (Qaccess, aspects);
@@ -149,15 +158,13 @@ symbol_to_inotifymask (Lisp_Object symb)
 static uint32_t
 aspect_to_inotifymask (Lisp_Object aspect)
 {
-  if (CONSP (aspect))
+  if (CONSP (aspect) || NILP (aspect))
     {
       Lisp_Object x = aspect;
       uint32_t mask = 0;
-      while (CONSP (x))
-        {
-          mask |= symbol_to_inotifymask (XCAR (x));
-          x = XCDR (x);
-        }
+      FOR_EACH_TAIL (x)
+	mask |= symbol_to_inotifymask (XCAR (x));
+      CHECK_LIST_END (x, aspect);
       return mask;
     }
   else
@@ -165,25 +172,13 @@ aspect_to_inotifymask (Lisp_Object aspect)
 }
 
 static Lisp_Object
-make_lispy_mask (uint32_t mask)
-{
-  return Fcons (make_number (mask & 0xffff),
-                make_number (mask >> 16));
-}
-
-static bool
-lispy_mask_match_p (Lisp_Object mask, uint32_t other)
-{
-  return (XINT (XCAR (mask)) & other)
-    || ((XINT (XCDR (mask)) << 16) & other);
-}
-
-static Lisp_Object
 inotifyevent_to_event (Lisp_Object watch, struct inotify_event const *ev)
 {
-  Lisp_Object name = Qnil;
+  Lisp_Object name;
+  uint32_t mask;
+  CONS_TO_INTEGER (Fnth (make_number (3), watch), uint32_t, mask);
 
-  if (! lispy_mask_match_p (Fnth (make_number (3), watch), ev->mask))
+  if (! (mask & ev->mask))
     return Qnil;
 
   if (ev->len > 0)
@@ -195,10 +190,10 @@ inotifyevent_to_event (Lisp_Object watch, struct inotify_event const *ev)
   else
     name = XCAR (XCDR (watch));
 
-  return list2 (list4 (Fcons (make_number (ev->wd), XCAR (watch)),
+  return list2 (list4 (Fcons (INTEGER_TO_CONS (ev->wd), XCAR (watch)),
                        mask_to_aspects (ev->mask),
                        name,
-                       make_number (ev->cookie)),
+		       INTEGER_TO_CONS (ev->cookie)),
 		Fnth (make_number (2), watch));
 }
 
@@ -209,55 +204,88 @@ static Lisp_Object
 add_watch (int wd, Lisp_Object filename,
 	   Lisp_Object aspect, Lisp_Object callback)
 {
-  Lisp_Object descriptor = make_number (wd);
-  Lisp_Object elt = Fassoc (descriptor, watch_list);
-  Lisp_Object watches = Fcdr (elt);
+  Lisp_Object descriptor = INTEGER_TO_CONS (wd);
+  Lisp_Object tail = assoc_no_quit (descriptor, watch_list);
   Lisp_Object watch, watch_id;
-  Lisp_Object mask = make_lispy_mask (aspect_to_inotifymask (aspect));
+  uint32_t imask = aspect_to_inotifymask (aspect);
+  Lisp_Object mask = INTEGER_TO_CONS (imask);
 
-  int id = 0;
-
-  while (! NILP (watches))
+  EMACS_INT id = 0;
+  if (NILP (tail))
+    {
+      tail = list1 (descriptor);
+      watch_list = Fcons (tail, watch_list);
+    }
+  else
     {
-      id = max (id, 1 + XINT (XCAR (XCAR (watches))));
-      watches = XCDR (watches);
+      /* Assign a watch ID that is not already in use, by looking
+	 for a gap in the existing sorted list.  */
+      for (; ! NILP (XCDR (tail)); tail = XCDR (tail), id++)
+	if (!EQ (XCAR (XCAR (XCDR (tail))), make_number (id)))
+	  break;
+      if (MOST_POSITIVE_FIXNUM < id)
+	emacs_abort ();
     }
 
   watch_id = make_number (id);
   watch = list4 (watch_id, filename, callback, mask);
-
-  if (NILP (elt))
-    watch_list = Fcons (Fcons (descriptor, Fcons (watch, Qnil)),
-                        watch_list);
-  else
-    XSETCDR (elt, Fcons (watch, XCDR (elt)));
+  XSETCDR (tail, Fcons (watch, XCDR (tail)));
 
   return Fcons (descriptor, watch_id);
 }
 
-/*  Remove all watches associated with descriptor.  If INVALID_P is
-    true, the descriptor is already invalid, i.e. it received a
-    IN_IGNORED event. In this case skip calling inotify_rm_watch.  */
+/* Find the watch list element (if any) matching DESCRIPTOR.  Return
+   nil if not found.  If found, return t if the first element matches
+   DESCRIPTOR; otherwise, return the cons whose cdr matches
+   DESCRIPTOR.  This lets the caller easily remove the element
+   matching DESCRIPTOR without having to search for it again, and
+   without calling Fdelete (which might quit).  */
+
+static Lisp_Object
+find_descriptor (Lisp_Object descriptor)
+{
+  Lisp_Object tail, prevtail = Qt;
+  for (tail = watch_list; !NILP (tail); prevtail = tail, tail = XCDR (tail))
+    if (equal_no_quit (XCAR (XCAR (tail)), descriptor))
+      return prevtail;
+  return Qnil;
+}
+
+/*  Remove all watches associated with the watch list element after
+    PREVTAIL, or after the first element if PREVTAIL is t.  If INVALID_P
+    is true, the descriptor is already invalid, i.e., it received a
+    IN_IGNORED event.  In this case skip calling inotify_rm_watch.  */
 static void
-remove_descriptor (Lisp_Object descriptor, bool invalid_p)
+remove_descriptor (Lisp_Object prevtail, bool invalid_p)
 {
-  Lisp_Object elt = Fassoc (descriptor, watch_list);
+  Lisp_Object tail = CONSP (prevtail) ? XCDR (prevtail) : watch_list;
 
-  if (! NILP (elt))
+  int inotify_errno = 0;
+  if (! invalid_p)
     {
-      int wd = XINT (descriptor);
+      int wd;
+      CONS_TO_INTEGER (XCAR (XCAR (tail)), int, wd);
+      if (inotify_rm_watch (inotifyfd, wd) != 0)
+	inotify_errno = errno;
+    }
 
-      watch_list = Fdelete (elt, watch_list);
-      if (! invalid_p)
-        if (inotify_rm_watch (inotifyfd, wd) == -1)
-          report_file_notify_error ("Could not rm watch", descriptor);
+  if (CONSP (prevtail))
+    XSETCDR (prevtail, XCDR (tail));
+  else
+    {
+      watch_list = XCDR (tail);
+      if (NILP (watch_list))
+	{
+	  delete_read_fd (inotifyfd);
+	  emacs_close (inotifyfd);
+	  inotifyfd = -1;
+	}
     }
-  /* Cleanup if no more files are watched.  */
-  if (NILP (watch_list))
+
+  if (inotify_errno != 0)
     {
-      emacs_close (inotifyfd);
-      delete_read_fd (inotifyfd);
-      inotifyfd = -1;
+      errno = inotify_errno;
+      report_file_notify_error ("Could not rm watch", XCAR (tail));
     }
 }
 
@@ -265,19 +293,19 @@ remove_descriptor (Lisp_Object descriptor, bool invalid_p)
 static void
 remove_watch (Lisp_Object descriptor, Lisp_Object id)
 {
-  Lisp_Object elt = Fassoc (descriptor, watch_list);
-
-  if (! NILP (elt))
-    {
-      Lisp_Object watch = Fassoc (id, XCDR (elt));
-
-      if (! NILP (watch))
-        XSETCDR (elt, Fdelete (watch, XCDR (elt)));
-
-      /* Remove the descriptor if noone is watching it.  */
-      if (NILP (XCDR (elt)))
-        remove_descriptor (descriptor, false);
-    }
+  Lisp_Object prevtail = find_descriptor (descriptor);
+  if (NILP (prevtail))
+    return;
+
+  Lisp_Object elt = XCAR (CONSP (prevtail) ? XCDR (prevtail) : watch_list);
+  for (Lisp_Object prev = elt; !NILP (XCDR (prev)); prev = XCDR (prev))
+    if (EQ (id, XCAR (XCAR (XCDR (prev)))))
+      {
+	XSETCDR (prev, XCDR (XCDR (prev)));
+	if (NILP (XCDR (elt)))
+	  remove_descriptor (prevtail, false);
+	break;
+      }
 }
 
 /* This callback is called when the FD is available for read.  The inotify
@@ -285,52 +313,44 @@ remove_watch (Lisp_Object descriptor, Lisp_Object id)
 static void
 inotify_callback (int fd, void *_)
 {
-  struct input_event event;
   int to_read;
-  char *buffer;
-  ssize_t n;
-  size_t i;
-
-  to_read = 0;
-  if (ioctl (fd, FIONREAD, &to_read) == -1)
+  if (ioctl (fd, FIONREAD, &to_read) < 0)
     report_file_notify_error ("Error while retrieving file system events",
 			      Qnil);
-  buffer = xmalloc (to_read);
-  n = read (fd, buffer, to_read);
+  USE_SAFE_ALLOCA;
+  char *buffer = SAFE_ALLOCA (to_read);
+  ssize_t n = read (fd, buffer, to_read);
   if (n < 0)
-    {
-      xfree (buffer);
-      report_file_notify_error ("Error while reading file system events", Qnil);
-    }
+    report_file_notify_error ("Error while reading file system events", Qnil);
 
+  struct input_event event;
   EVENT_INIT (event);
   event.kind = FILE_NOTIFY_EVENT;
 
-  i = 0;
-  while (i < (size_t)n)
+  for (ssize_t i = 0; i < n; )
     {
       struct inotify_event *ev = (struct inotify_event *) &buffer[i];
-      Lisp_Object descriptor = make_number (ev->wd);
-      Lisp_Object elt = Fassoc (descriptor, watch_list);
+      Lisp_Object descriptor = INTEGER_TO_CONS (ev->wd);
+      Lisp_Object prevtail = find_descriptor (descriptor);
 
-      if (! NILP (elt))
+      if (! NILP (prevtail))
         {
-          Lisp_Object watches = XCDR (elt);
-          while (! NILP (watches))
+	  Lisp_Object tail = CONSP (prevtail) ? XCDR (prevtail) : watch_list;
+	  for (Lisp_Object watches = XCDR (XCAR (tail)); ! NILP (watches);
+	       watches = XCDR (watches))
             {
               event.arg = inotifyevent_to_event (XCAR (watches), ev);
               if (!NILP (event.arg))
                 kbd_buffer_store_event (&event);
-              watches = XCDR (watches);
             }
           /* If event was removed automatically: Drop it from watch list.  */
           if (ev->mask & IN_IGNORED)
-            remove_descriptor (descriptor, true);
+	    remove_descriptor (prevtail, true);
         }
       i += sizeof (*ev) + ev->len;
     }
 
-  xfree (buffer);
+  SAFE_FREE ();
 }
 
 DEFUN ("inotify-add-watch", Finotify_add_watch, Sinotify_add_watch, 3, 3, 0,
@@ -407,7 +427,7 @@ IN_ONLYDIR  */)
 
   if (inotifyfd < 0)
     {
-      inotifyfd = inotify_init1 (IN_NONBLOCK|IN_CLOEXEC);
+      inotifyfd = inotify_init1 (IN_NONBLOCK | IN_CLOEXEC);
       if (inotifyfd < 0)
 	report_file_notify_error ("File watching is not available", Qnil);
       watch_list = Qnil;
@@ -416,12 +436,24 @@ IN_ONLYDIR  */)
 
   encoded_file_name = ENCODE_FILE (filename);
   wd = inotify_add_watch (inotifyfd, SSDATA (encoded_file_name), mask);
-  if (wd == -1)
+  if (wd < 0)
     report_file_notify_error ("Could not add watch for file", filename);
 
   return add_watch (wd, filename, aspect, callback);
 }
 
+static bool
+valid_watch_descriptor (Lisp_Object wd)
+{
+  return (CONSP (wd)
+	  && (RANGED_INTEGERP (0, XCAR (wd), INT_MAX)
+	      || (CONSP (XCAR (wd))
+		  && RANGED_INTEGERP ((MOST_POSITIVE_FIXNUM >> 16) + 1,
+				      XCAR (XCAR (wd)), INT_MAX >> 16)
+		  && RANGED_INTEGERP (0, XCDR (XCAR (wd)), (1 << 16) - 1)))
+	  && NATNUMP (XCDR (wd)));
+}
+
 DEFUN ("inotify-rm-watch", Finotify_rm_watch, Sinotify_rm_watch, 1, 1, 0,
        doc: /* Remove an existing WATCH-DESCRIPTOR.
 
@@ -433,9 +465,7 @@ See inotify_rm_watch(2) for more information.  */)
 
   Lisp_Object descriptor, id;
 
-  if (! (CONSP (watch_descriptor)
-         && INTEGERP (XCAR (watch_descriptor))
-         && INTEGERP (XCDR (watch_descriptor))))
+  if (! valid_watch_descriptor (watch_descriptor))
     report_file_notify_error ("Invalid descriptor ", watch_descriptor);
 
   descriptor = XCAR (watch_descriptor);
@@ -456,16 +486,12 @@ reason.  Removing the watch by calling `inotify-rm-watch' also makes
 it invalid.  */)
      (Lisp_Object watch_descriptor)
 {
-  Lisp_Object elt, watch;
-
-  if (! (CONSP (watch_descriptor)
-         && INTEGERP (XCAR (watch_descriptor))
-         && INTEGERP (XCDR (watch_descriptor))))
+  if (! valid_watch_descriptor (watch_descriptor))
     return Qnil;
-
-  elt = Fassoc (XCAR (watch_descriptor), watch_list);
-  watch = Fassoc (XCDR (watch_descriptor), XCDR (elt));
-
+  Lisp_Object tail = assoc_no_quit (XCAR (watch_descriptor), watch_list);
+  if (NILP (tail))
+    return Qnil;
+  Lisp_Object watch = assq_no_quit (XCDR (watch_descriptor), XCDR (tail));
   return ! NILP (watch) ? Qt : Qnil;
 }
 
diff --git a/src/lisp.h b/src/lisp.h
index 4b9cd3c..3125bd2 100644
--- a/src/lisp.h
+++ b/src/lisp.h
@@ -3376,6 +3376,7 @@ extern Lisp_Object merge (Lisp_Object, Lisp_Object, Lisp_Object);
 extern Lisp_Object do_yes_or_no_p (Lisp_Object);
 extern Lisp_Object concat2 (Lisp_Object, Lisp_Object);
 extern Lisp_Object concat3 (Lisp_Object, Lisp_Object, Lisp_Object);
+extern bool equal_no_quit (Lisp_Object, Lisp_Object);
 extern Lisp_Object nconc2 (Lisp_Object, Lisp_Object);
 extern Lisp_Object assq_no_quit (Lisp_Object, Lisp_Object);
 extern Lisp_Object assoc_no_quit (Lisp_Object, Lisp_Object);
diff --git a/test/src/inotify-tests.el b/test/src/inotify-tests.el
index f30aecc..987e1fc 100644
--- a/test/src/inotify-tests.el
+++ b/test/src/inotify-tests.el
@@ -28,6 +28,13 @@
 (declare-function inotify-add-watch "inotify.c" (file-name aspect callback))
 (declare-function inotify-rm-watch "inotify.c" (watch-descriptor))
 
+(ert-deftest inotify-valid-p-simple ()
+  "Simple tests for `inotify-valid-p'."
+  (skip-unless (featurep 'inotify))
+  (should-not (inotify-valid-p 0))
+  (should-not (inotify-valid-p nil))
+  (should-not (inotify-valid-p '(0 . 0))))
+
 ;; (ert-deftest filewatch-file-watch-aspects-check ()
 ;;   "Test whether `file-watch' properly checks the aspects."
 ;;   (let ((temp-file (make-temp-file "filewatch-aspects")))
@@ -56,7 +63,9 @@
 	      (insert "Foo\n"))
 	    (read-event nil nil 5)
 	    (should (> events 0)))
+	(should (inotify-valid-p wd))
 	(inotify-rm-watch wd)
+	(should-not (inotify-valid-p wd))
 	(delete-file temp-file)))))
 
 (provide 'inotify-tests)
-- 
2.9.3