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: Tue, 21 Mar 2017 16:06:22 +0100 Message-ID: <87efxqmzyp.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> <8737e8excq.fsf@luca> <87bmsuixuj.fsf@detlef> NNTP-Posting-Host: blaine.gmane.org Mime-Version: 1.0 Content-Type: text/plain X-Trace: blaine.gmane.org 1490108849 11386 195.159.176.226 (21 Mar 2017 15:07:29 GMT) X-Complaints-To: usenet@blaine.gmane.org NNTP-Posting-Date: Tue, 21 Mar 2017 15:07:29 +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 Tue Mar 21 16:07:19 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 1cqLNV-00022L-4U for geb-bug-gnu-emacs@m.gmane.org; Tue, 21 Mar 2017 16:07:17 +0100 Original-Received: from localhost ([::1]:40653 helo=lists.gnu.org) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1cqLNX-0006zT-Nw for geb-bug-gnu-emacs@m.gmane.org; Tue, 21 Mar 2017 11:07:19 -0400 Original-Received: from eggs.gnu.org ([2001:4830:134:3::10]:46160) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1cqLNM-0006yZ-DA for bug-gnu-emacs@gnu.org; Tue, 21 Mar 2017 11:07:14 -0400 Original-Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1cqLNH-0006cn-8E for bug-gnu-emacs@gnu.org; Tue, 21 Mar 2017 11:07:08 -0400 Original-Received: from debbugs.gnu.org ([208.118.235.43]:39893) by eggs.gnu.org with esmtps (TLS1.0:RSA_AES_128_CBC_SHA1:16) (Exim 4.71) (envelope-from ) id 1cqLNG-0006ci-TI for bug-gnu-emacs@gnu.org; Tue, 21 Mar 2017 11:07:03 -0400 Original-Received: from Debian-debbugs by debbugs.gnu.org with local (Exim 4.84_2) (envelope-from ) id 1cqLNG-0006S1-EN for bug-gnu-emacs@gnu.org; Tue, 21 Mar 2017 11:07:02 -0400 X-Loop: help-debbugs@gnu.org Resent-From: Andreas Politz Original-Sender: "Debbugs-submit" Resent-CC: bug-gnu-emacs@gnu.org Resent-Date: Tue, 21 Mar 2017 15:07:02 +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.149010880524771 (code B ref 26126); Tue, 21 Mar 2017 15:07:02 +0000 Original-Received: (at 26126) by debbugs.gnu.org; 21 Mar 2017 15:06:45 +0000 Original-Received: from localhost ([127.0.0.1]:38092 helo=debbugs.gnu.org) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1cqLMy-0006RT-Qs for submit@debbugs.gnu.org; Tue, 21 Mar 2017 11:06:45 -0400 Original-Received: from gateway-a.fh-trier.de ([143.93.54.181]:45029) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1cqLMw-0006RE-B4 for 26126@debbugs.gnu.org; Tue, 21 Mar 2017 11:06:43 -0400 X-Virus-Scanned: by Amavisd-new + McAfee uvscan + ClamAV [Rechenzentrum Hochschule Trier (RZ/HT)] Original-Received: from localhost (unknown [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 30BC3179B384; Tue, 21 Mar 2017 16:06:23 +0100 (CET) DKIM-Signature: v=1; a=rsa-sha1; c=simple/simple; d=hochschule-trier.de; s=default; t=1490108783; bh=u/QkFz5/glB0lwYaSKvLifvGYXM=; h=From:To:Cc:Subject:References:Date:In-Reply-To:Message-ID: MIME-Version:Content-Type; b=hsAUYYvjosYOvqQePntmTWLXqpJlpRctVRVvDI6DiWNeD5nwfZ/buSp6+AWK3yfYJ s7qzpmGuXIaLvvJk/kcuMphu8kazqzwUJH2EUVDeB7rtYeFEJJeGVnectbOA3mhBiO niE2ox43uFmmm9vdOTeZZvXff5Eyfk2TpXlqMWSU= In-Reply-To: <87bmsuixuj.fsf@detlef> (Michael Albinus's message of "Tue, 21 Mar 2017 14:05:40 +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:130781 Archived-At: Michael Albinus writes: > Andreas Politz 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