[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
bug#26126: 26.0.50; file-notify-rm-watch removes arbitrary watches
From: |
Michael Albinus |
Subject: |
bug#26126: 26.0.50; file-notify-rm-watch removes arbitrary watches |
Date: |
Wed, 22 Mar 2017 14:17:10 +0100 |
User-agent: |
Gnus/5.13 (Gnus v5.13) Emacs/26.0.50 (gnu/linux) |
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, (continued)
- bug#26126: 26.0.50; file-notify-rm-watch removes arbitrary watches, Andreas Politz, 2017/03/21
- bug#26126: 26.0.50; file-notify-rm-watch removes arbitrary watches, Michael Albinus, 2017/03/22
- bug#26126: 26.0.50; file-notify-rm-watch removes arbitrary watches, Andreas Politz, 2017/03/22
- bug#26126: 26.0.50; file-notify-rm-watch removes arbitrary watches, Michael Albinus, 2017/03/22
- bug#26126: 26.0.50; file-notify-rm-watch removes arbitrary watches, Andreas Politz, 2017/03/19
- bug#26126: 26.0.50; file-notify-rm-watch removes arbitrary watches, Michael Albinus, 2017/03/21
- bug#26126: 26.0.50; file-notify-rm-watch removes arbitrary watches, Andreas Politz, 2017/03/21
- bug#26126: 26.0.50; file-notify-rm-watch removes arbitrary watches, Eli Zaretskii, 2017/03/21
- bug#26126: 26.0.50; file-notify-rm-watch removes arbitrary watches,
Michael Albinus <=
- bug#26126: 26.0.50; file-notify-rm-watch removes arbitrary watches, Andreas Politz, 2017/03/22
- bug#26126: 26.0.50; file-notify-rm-watch removes arbitrary watches, Michael Albinus, 2017/03/22
- bug#26126: 26.0.50; file-notify-rm-watch removes arbitrary watches, Eli Zaretskii, 2017/03/22
- bug#26126: 26.0.50; file-notify-rm-watch removes arbitrary watches, Michael Albinus, 2017/03/23
- bug#26126: 26.0.50; file-notify-rm-watch removes arbitrary watches, Eli Zaretskii, 2017/03/23
- bug#26126: 26.0.50; file-notify-rm-watch removes arbitrary watches, Michael Albinus, 2017/03/23
- bug#26126: 26.0.50; file-notify-rm-watch removes arbitrary watches, Michael Albinus, 2017/03/22
- bug#26126: 26.0.50; file-notify-rm-watch removes arbitrary watches, Andreas Politz, 2017/03/24
- bug#26126: 26.0.50; file-notify-rm-watch removes arbitrary watches, Eli Zaretskii, 2017/03/25
- bug#26126: 26.0.50; file-notify-rm-watch removes arbitrary watches, Andreas Politz, 2017/03/25