[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
bug#36591: 26.2; Term's pager seems broken
From: |
Eli Zaretskii |
Subject: |
bug#36591: 26.2; Term's pager seems broken |
Date: |
Wed, 24 Jul 2019 18:07:53 +0300 |
> From: Noam Postavsky <npostavs@gmail.com>
> Cc: abliss@gmail.com, 36591@debbugs.gnu.org
> Date: Tue, 23 Jul 2019 22:07:24 -0400
>
> static void
> set_process_filter_masks (struct Lisp_Process *p)
> {
> if (EQ (p->filter, Qt) && !EQ (p->status, Qlisten))
> delete_read_fd (p->infd);
> else if (EQ (p->filter, Qt)
> /* Network or serial process not stopped: */
> && !EQ (p->command, Qt))
> add_process_read_fd (p->infd);
> }
>
> In Emacs 25 it looks like the 'if' cases are the same, but there is a
> subtle difference: the first 'if' checks 'filter', while the second
> checks 'p->filter'. And furthermore note that pset_filter is called
> after this check against 'p->filter', so it is checking the "original"
> 'p->filter' value (which means that Emacs 25 has a bug that the fd is
> incorrectly enabled if setting the filter to t twice, i.e., (progn
> (set-process-filter PROC t) (set-process-filter PROC t))).
>
> DEFUN ("set-process-filter", Fset_process_filter, Sset_process_filter,
> ...
> (register Lisp_Object process, Lisp_Object filter)
> {
> ...
> if (p->infd >= 0)
> {
> if (EQ (filter, Qt) && !EQ (p->status, Qlisten))
> {
> FD_CLR (p->infd, &input_wait_mask);
> FD_CLR (p->infd, &non_keyboard_wait_mask);
> }
> else if (EQ (p->filter, Qt)
> /* Network or serial process not stopped: */
> && !EQ (p->command, Qt))
> {
> FD_SET (p->infd, &input_wait_mask);
> FD_SET (p->infd, &non_keyboard_wait_mask);
> }
> }
>
> pset_filter (p, filter);
I see that it's a small mess, but I don't think I understand your
reasoning about setting the filter to t twice: it looks to me that
both calls will clear the bit of p->infd, because they both will
trigger this clause:
current emacs-26:
if (EQ (p->filter, Qt) && !EQ (p->status, Qlisten))
delete_read_fd (p->infd);
previous code:
if (EQ (filter, Qt) && !EQ (p->status, Qlisten))
{
FD_CLR (p->infd, &input_wait_mask);
FD_CLR (p->infd, &non_keyboard_wait_mask);
}
Am I missing something?
> The patch found by Adam's bisect put the pset_filter call before this
> check, so that Emacs 26 checks the 'p->filter' after it's been set
> (i.e., the value of the 'filter' parameter). So the second case is no
> longer entered when calling (set-filter-process PROC FILTER).
Right, this part is clear. My problem was with the solution that just
reverses the 2nd test. See below.
> > Also, the same function is called in another
> > place, so what will this change do to that other caller?
>
> Hmm, it's difficult to say, there are a lot of optional code paths. I
> suspect socket subprocesses might suffer from the same bug, but there
> are also some (redundant?) calls add_process_read_fd that might cover it
> up (notice that all calls are guarded by !EQ (p->filter, Qt), i.e., the
> corrected version of the check in set_process_filter_masks).
Yes, that's another small mess.
I think we need to be more careful here if we want to have this fixed
in Emacs 26.3. The problem with your proposal is that it has
potentially far-reaching consequences:
. if the filter is not t, we will now call add_process_read_fd every
time, for no good reason
. the patch changes how connect_network_socket works in ways that we
don't sufficiently understand
I would like to leave connect_network_socket alone on the release
branch, and just fix the problem with set-process-filter. I think the
easiest way is simply not to call set_process_filter_masks in
set-process-filter, and instead restore the way that code worked
before 9755b753, i.e. let it test the argument FILTER _before_ that
filter is installed. Do you see any problems with this fix?
On the master branch we should clean up the confusing set of if
clauses, both in set-process-filter and in connect_network_socket.
Perhaps Lars could describe his reasoning for making the change which
introduced set_process_filter_masks and what problem it tried to
solve. (Btw, the log message for that change seems to imply that
set-process-filter should not have called set_process_filter_masks,
something that the change itself disagrees with. An omission?)
- bug#36591: 26.2; Term's pager seems broken, Adam Bliss, 2019/07/11
- bug#36591: 26.2; Term's pager seems broken, Noam Postavsky, 2019/07/23
- bug#36591: 26.2; Term's pager seems broken, Eli Zaretskii, 2019/07/23
- bug#36591: 26.2; Term's pager seems broken, Adam Bliss, 2019/07/23
- bug#36591: 26.2; Term's pager seems broken, Noam Postavsky, 2019/07/23
- bug#36591: 26.2; Term's pager seems broken,
Eli Zaretskii <=
- bug#36591: 26.2; Term's pager seems broken, Noam Postavsky, 2019/07/24
- bug#36591: 26.2; Term's pager seems broken, Lars Ingebrigtsen, 2019/07/25
- bug#36591: 26.2; Term's pager seems broken, Eli Zaretskii, 2019/07/25
- bug#36591: 26.2; Term's pager seems broken, Lars Ingebrigtsen, 2019/07/25
- bug#36591: 26.2; Term's pager seems broken, Noam Postavsky, 2019/07/25
- bug#36591: 26.2; Term's pager seems broken, Lars Ingebrigtsen, 2019/07/25
- bug#36591: 26.2; Term's pager seems broken, Noam Postavsky, 2019/07/25
- bug#36591: 26.2; Term's pager seems broken, Lars Ingebrigtsen, 2019/07/25
- bug#36591: 26.2; Term's pager seems broken, Noam Postavsky, 2019/07/25
- bug#36591: 26.2; Term's pager seems broken, Eli Zaretskii, 2019/07/25