From mboxrd@z Thu Jan 1 00:00:00 1970 Path: news.gmane.org!.POSTED!not-for-mail From: Andreas Politz Newsgroups: gmane.emacs.bugs Subject: bug#26126: 26.0.50; file-notify-rm-watch removes arbitrary watches Date: Sun, 19 Mar 2017 23:05:25 +0100 Message-ID: <8737e8excq.fsf@luca> References: <87r31x9ulw.fsf@luca> <87shmcney8.fsf@detlef> <87efxw7xvc.fsf@luca> <87mvcjophx.fsf@detlef> <87tw6rssoi.fsf@luca> <87pohfkmvh.fsf@detlef> <87lgs2sobr.fsf@luca> <87y3w2gywc.fsf@detlef> NNTP-Posting-Host: blaine.gmane.org Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="=-=-=" X-Trace: blaine.gmane.org 1489961176 25319 195.159.176.226 (19 Mar 2017 22:06:16 GMT) X-Complaints-To: usenet@blaine.gmane.org NNTP-Posting-Date: Sun, 19 Mar 2017 22:06:16 +0000 (UTC) User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/26.0.50 (gnu/linux) Cc: 26126@debbugs.gnu.org To: Michael Albinus Original-X-From: bug-gnu-emacs-bounces+geb-bug-gnu-emacs=m.gmane.org@gnu.org Sun Mar 19 23:06:11 2017 Return-path: Envelope-to: geb-bug-gnu-emacs@m.gmane.org Original-Received: from lists.gnu.org ([208.118.235.17]) by blaine.gmane.org with esmtp (Exim 4.84_2) (envelope-from ) id 1cpixm-0005ln-JT for geb-bug-gnu-emacs@m.gmane.org; Sun, 19 Mar 2017 23:06:10 +0100 Original-Received: from localhost ([::1]:58290 helo=lists.gnu.org) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1cpixp-0004EL-AN for geb-bug-gnu-emacs@m.gmane.org; Sun, 19 Mar 2017 18:06:13 -0400 Original-Received: from eggs.gnu.org ([2001:4830:134:3::10]:35293) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1cpixh-0004E3-Tx for bug-gnu-emacs@gnu.org; Sun, 19 Mar 2017 18:06:07 -0400 Original-Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1cpixe-0004Oe-JY for bug-gnu-emacs@gnu.org; Sun, 19 Mar 2017 18:06:05 -0400 Original-Received: from debbugs.gnu.org ([208.118.235.43]:37367) by eggs.gnu.org with esmtps (TLS1.0:RSA_AES_128_CBC_SHA1:16) (Exim 4.71) (envelope-from ) id 1cpixe-0004Nx-0u for bug-gnu-emacs@gnu.org; Sun, 19 Mar 2017 18:06:02 -0400 Original-Received: from Debian-debbugs by debbugs.gnu.org with local (Exim 4.84_2) (envelope-from ) id 1cpixd-00073U-No for bug-gnu-emacs@gnu.org; Sun, 19 Mar 2017 18:06:01 -0400 X-Loop: help-debbugs@gnu.org Resent-From: Andreas Politz Original-Sender: "Debbugs-submit" Resent-CC: bug-gnu-emacs@gnu.org Resent-Date: Sun, 19 Mar 2017 22:06:01 +0000 Resent-Message-ID: Resent-Sender: help-debbugs@gnu.org X-GNU-PR-Message: followup 26126 X-GNU-PR-Package: emacs X-GNU-PR-Keywords: Original-Received: via spool by 26126-submit@debbugs.gnu.org id=B26126.148996114927100 (code B ref 26126); Sun, 19 Mar 2017 22:06:01 +0000 Original-Received: (at 26126) by debbugs.gnu.org; 19 Mar 2017 22:05:49 +0000 Original-Received: from localhost ([127.0.0.1]:35566 helo=debbugs.gnu.org) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1cpixP-00072x-Mt for submit@debbugs.gnu.org; Sun, 19 Mar 2017 18:05:49 -0400 Original-Received: from gateway-a.fh-trier.de ([143.93.54.181]:59878) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1cpixN-00072j-4R for 26126@debbugs.gnu.org; Sun, 19 Mar 2017 18:05:46 -0400 X-Virus-Scanned: by Amavisd-new + McAfee uvscan + ClamAV [Rechenzentrum Hochschule Trier (RZ/HT)] Original-Received: from localhost (ip5f5bdecf.dynamic.kabel-deutschland.de [95.91.222.207]) (using TLSv1 with cipher AES256-SHA (256/256 bits)) (No client certificate requested) (Authenticated sender: politza) by gateway-a.fh-trier.de (Postfix) with ESMTPSA id 8CAEC179AB34; Sun, 19 Mar 2017 23:05:25 +0100 (CET) DKIM-Signature: v=1; a=rsa-sha1; c=simple/simple; d=hochschule-trier.de; s=default; t=1489961125; bh=uQZMMk2ZVeZjsLbsRT5/Aq8EAvA=; h=From:To:Cc:Subject:In-Reply-To:References:Date:Message-ID: MIME-Version:Content-Type; b=RF/YoD1kXHOWUJySkL1MHPNUVS/r8XDBTJgGpqkYHUA/T7zmwhvCiXJG15VatHCBP rpTU+eQhfqMyXpkbeaCJ/POQMJr6oimoSPWv+96SkjOqpuhWqmaSMPDWDcqj7GvyXm uzJQ1IXVDBjxtF6YQW+OebVfCLEVTVtJPk8tCKXM= In-Reply-To: <87y3w2gywc.fsf@detlef> (Michael Albinus's message of "Sat, 18 Mar 2017 20:36:51 +0100") X-BeenThere: debbugs-submit@debbugs.gnu.org X-Mailman-Version: 2.1.18 Precedence: list X-detected-operating-system: by eggs.gnu.org: GNU/Linux 2.2.x-3.x [generic] X-Received-From: 208.118.235.43 X-BeenThere: bug-gnu-emacs@gnu.org List-Id: "Bug reports for GNU Emacs, the Swiss army knife of text editors" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: bug-gnu-emacs-bounces+geb-bug-gnu-emacs=m.gmane.org@gnu.org Original-Sender: "bug-gnu-emacs" Xref: news.gmane.org gmane.emacs.bugs:130734 Archived-At: --=-=-= Content-Type: text/plain 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. --=-=-= Content-Type: text/x-diff Content-Disposition: inline Content-Description: inotify.diff 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); --=-=-= Content-Type: text/plain 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. --=-=-= Content-Type: text/x-diff Content-Disposition: inline Content-Description: remote-handler.diff diff -u --label /home/politza/src/emacs/lisp/filenotify.el --label \#\ /home/politza/src/emacs/lisp/filenotify.el /tmp/buffer-content-142424Gt --- /home/politza/src/emacs/lisp/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 --=-=-= Content-Type: text/plain * 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. --=-=-= Content-Type: application/emacs-lisp Content-Disposition: inline; filename=pending-event-test.el Content-Transfer-Encoding: quoted-printable Content-Description: ERT Test for pending events with 2 clients (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 e= vent)))) (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))) --=-=-= Content-Type: text/plain * 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". --=-=-= Content-Type: text/x-diff Content-Disposition: inline Content-Description: os.texi diff 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 --=-=-= Content-Type: text/plain -ap --=-=-=--