From mboxrd@z Thu Jan 1 00:00:00 1970 Path: news.gmane.org!.POSTED!not-for-mail From: Michael Albinus Newsgroups: gmane.emacs.bugs Subject: bug#26126: 26.0.50; file-notify-rm-watch removes arbitrary watches Date: Tue, 21 Mar 2017 14:05:40 +0100 Message-ID: <87bmsuixuj.fsf@detlef> 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> <8737e8excq.fsf@luca> NNTP-Posting-Host: blaine.gmane.org Mime-Version: 1.0 Content-Type: text/plain X-Trace: blaine.gmane.org 1490101582 12017 195.159.176.226 (21 Mar 2017 13:06:22 GMT) X-Complaints-To: usenet@blaine.gmane.org NNTP-Posting-Date: Tue, 21 Mar 2017 13:06:22 +0000 (UTC) User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/26.0.50 (gnu/linux) Cc: 26126@debbugs.gnu.org To: Andreas Politz Original-X-From: bug-gnu-emacs-bounces+geb-bug-gnu-emacs=m.gmane.org@gnu.org Tue Mar 21 14:06:15 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 1cqJUK-0001qX-QM for geb-bug-gnu-emacs@m.gmane.org; Tue, 21 Mar 2017 14:06:13 +0100 Original-Received: from localhost ([::1]:40217 helo=lists.gnu.org) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1cqJUN-0007hH-Ee for geb-bug-gnu-emacs@m.gmane.org; Tue, 21 Mar 2017 09:06:15 -0400 Original-Received: from eggs.gnu.org ([2001:4830:134:3::10]:35265) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1cqJUE-0007fP-CZ for bug-gnu-emacs@gnu.org; Tue, 21 Mar 2017 09:06:07 -0400 Original-Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1cqJUA-0003Zu-9z for bug-gnu-emacs@gnu.org; Tue, 21 Mar 2017 09:06:06 -0400 Original-Received: from debbugs.gnu.org ([208.118.235.43]:39126) by eggs.gnu.org with esmtps (TLS1.0:RSA_AES_128_CBC_SHA1:16) (Exim 4.71) (envelope-from ) id 1cqJUA-0003Zb-68 for bug-gnu-emacs@gnu.org; Tue, 21 Mar 2017 09:06:02 -0400 Original-Received: from Debian-debbugs by debbugs.gnu.org with local (Exim 4.84_2) (envelope-from ) id 1cqJU9-0003Tv-VA for bug-gnu-emacs@gnu.org; Tue, 21 Mar 2017 09:06:01 -0400 X-Loop: help-debbugs@gnu.org In-Reply-To: <87r31x9ulw.fsf@luca> Resent-From: Michael Albinus Original-Sender: "Debbugs-submit" Resent-CC: bug-gnu-emacs@gnu.org Resent-Date: Tue, 21 Mar 2017 13: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.149010155213368 (code B ref 26126); Tue, 21 Mar 2017 13:06:01 +0000 Original-Received: (at 26126) by debbugs.gnu.org; 21 Mar 2017 13:05:52 +0000 Original-Received: from localhost ([127.0.0.1]:37325 helo=debbugs.gnu.org) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1cqJTz-0003TY-Nz for submit@debbugs.gnu.org; Tue, 21 Mar 2017 09:05:52 -0400 Original-Received: from mout.gmx.net ([212.227.17.22]:50181) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1cqJTx-0003TK-PU for 26126@debbugs.gnu.org; Tue, 21 Mar 2017 09:05:50 -0400 Original-Received: from detlef.gmx.de ([93.209.71.173]) by mail.gmx.com (mrgmx102 [212.227.17.168]) with ESMTPSA (Nemesis) id 0M0Kp7-1c2kfL3H6j-00ubuP; Tue, 21 Mar 2017 14:05:43 +0100 X-Provags-ID: V03:K0:+YS2ylWhfG1nogtH2rSgbrHVKC5/f4f4K0JsMDzDQRxlwlZSwGO QXO6rK69m1S+paYzQhZFDc1p+o+ROOaws4T+LePyj1WmUo48lly7Ty75i9yCt0y2YBFb/uh 49l+FRe9OZXYNlK+lO23G84qt/0Nrwm9+g0QzWLgZ7MLKGtwAZOpIGRGlhYgRMCpwO7XhUB LBw3QmaZVnALjqjumUFaQ== X-UI-Out-Filterresults: notjunk:1;V01:K0:ctbkbFEEHko=:8YDu+cpVIRxynb28XJTXiw egEL/dSqvFVtdiUm9YBnc9fHZHRirArfD5vyZvVdinZCZEZCnOBoINX9h33KSsCfz+S5r1gLx LukALb1+4q7ulRdh2btrvibhF0iyW480WIWyUPKsWdjpDPkCv/lLYVtgcy83wo+YX79EK9P03 p+Kw/wGcEJs/obD1aoun4CEwlkdMKpMmvg6NC/uakbwnHGZzzb8jOxYm4HpLGm4s0kumKIoxn Mjfr03tJbUL+UN6MmUA4o1Js//1BYhC/BiuuCDfXVeze4sY1UlORHf6W9fSfg2yqnoT03Nijt TaodKNVjl2M5nXhklHWIy6x8A6G3xwgm7+gUo3VIdZO/b0qFO0vA0f2XJeg9L/3XxMYq7xCVl hd/ICdmdUTvQGW+JNqAhhzVkG6PICIqoBg9GmbX+hY95fwAlWW9rggmY5h+p9ltRG6IpISSdU GGasZWFQ6dfUo8DDZ2NlIbQc4cgbPjAY3IU3mumuZx1zAsPVLRik7oRC/nNQE+EkR/DS+8zwn U+eGtSH8XJz9n8BOgPUeALMaTNi2pkyMLML7vwHKf40tkfcurG4TZ1+Qmgg7Aes9NRmubDrJ5 DqbY0peU6zvmLtkgWNNlkGK0btddsV0Kv51wEbJtkZjer2jo6r4MFxNBJi6jclXnBOYI8UUjc 8XE3KGFY7ry0Yy6HS7fZd1xlWUeLca5vV0ASGwianpwf7xH1ycsIA2eQW2hP/xxp+naAolPYa 3b7sTmM1TdvjyhflZKSNqAsNP3FiWW5ZsdrZPfh3KcPO9ZEpxXKg6heDPhrbE6GZqlt72I+x 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:130780 Archived-At: Andreas Politz 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.