bug#28066: eshell-tramp sudo ignores C-c

bug#28066: eshell-tramp sudo ignores C-c

[Yegor Timoshenko]

[-- Attachment #1: Type: text/plain, Size: 487 bytes --]

GNU Emacs 25.2.1 (x86_64-unknown-linux-gnu, GTK+ Version 3.22.16)
 of 2017-08-09

To reproduce, you'll need to add eshell-tramp module to configuration. Add
the following form to your Emacs configuration file:

(eval-after-load 'esh-module
  '(add-to-list 'eshell-modules-list 'eshell-tramp))

Then, M-x eshell.
$ which sudo
eshell/sudo is a compiled Lisp function in ‘em-tramp.el’
$ sudo sleep 10

Now, if you try to leave interrupt `sleep` using C-c, it won't work.

[-- Attachment #2: Type: text/html, Size: 699 bytes --]

bug#28066: eshell-tramp sudo ignores C-c

[Michael Albinus]

Yegor Timoshenko <yegortimoshenko@gmail.com> writes:

Hi Yegor,

> To reproduce, you'll need to add eshell-tramp module to configuration.
> Add the following form to your Emacs configuration file:
>
> (eval-after-load 'esh-module
>   '(add-to-list 'eshell-modules-list 'eshell-tramp))
>
> Then, M-x eshell.
> $ which sudo
> eshell/sudo is a compiled Lisp function in ‘em-tramp.el’
> $ sudo sleep 10
>
> Now, if you try to leave interrupt `sleep` using C-c, it won't work.

Unfortunately, it is not possible to send signals to the remote process
Tramp is running on. This has been discussed already on the Tramp ML,
with no result.

See also the TODO list on the bottom of tramp-sh.el.

Best regards, Michael.

bug#28066: eshell-tramp sudo ignores C-c

[Richard Stallman]

[[[ To any NSA and FBI agents reading my email: please consider    ]]]
[[[ whether defending the US Constitution against all enemies,     ]]]
[[[ foreign or domestic, requires you to follow Snowden's example. ]]]

  > > Now, if you try to leave interrupt `sleep` using C-c, it won't work.

  > Unfortunately, it is not possible to send signals to the remote process
  > Tramp is running on. This has been discussed already on the Tramp ML,
  > with no result.

Maybe eshell could detect this case
and display a message to explain to the user why it doesn't work.

-- 
Dr Richard Stallman
President, Free Software Foundation (gnu.org, fsf.org)
Internet Hall-of-Famer (internethalloffame.org)
Skype: No way! See stallman.org/skype.html.

bug#28066: eshell-tramp sudo ignores C-c

[Michael Albinus]

Richard Stallman <rms@gnu.org> writes:

>   > > Now, if you try to leave interrupt `sleep` using C-c, it won't work.
>
>   > Unfortunately, it is not possible to send signals to the remote process
>   > Tramp is running on. This has been discussed already on the Tramp ML,
>   > with no result.
>
> Maybe eshell could detect this case
> and display a message to explain to the user why it doesn't work.

Finally, I have implemented `interrupt-process' for remote
processes. I've committed 296472f5c5 to the Emacs repository. Yegor, do
you have a chance to test Emacs 26.1, whether this works for you?

I've implemented this as advice on `interrupt-process', but this is
discouraged for primitives. Maybe we should spend `interrupt-process' a
hook where Tramp could enter? Or even a file name handler, based on
default-directory of the related process-buffer?

Best regards, Michael.

bug#28066: eshell-tramp sudo ignores C-c

[Eli Zaretskii]

> From: Michael Albinus <michael.albinus@gmx.de>
> Date: Sun, 20 Aug 2017 21:29:33 +0200
> Cc: 28066@debbugs.gnu.org, yegortimoshenko@gmail.com
> 
> I've implemented this as advice on `interrupt-process', but this is
> discouraged for primitives. Maybe we should spend `interrupt-process' a
> hook where Tramp could enter? Or even a file name handler, based on
> default-directory of the related process-buffer?

In such cases, we usually provide a variable whose value is a
function.  The default value is a function that does whatever
interrupt-process is doing now, and Tramp can replace it with its own
function.

Does this make sense in your case?

bug#28066: eshell-tramp sudo ignores C-c

[Michael Albinus]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Michael Albinus <michael.albinus@gmx.de>
>> Date: Sun, 20 Aug 2017 21:29:33 +0200
>> Cc: 28066@debbugs.gnu.org, yegortimoshenko@gmail.com
>> 
>> I've implemented this as advice on `interrupt-process', but this is
>> discouraged for primitives. Maybe we should spend `interrupt-process' a
>> hook where Tramp could enter? Or even a file name handler, based on
>> default-directory of the related process-buffer?
>
> In such cases, we usually provide a variable whose value is a
> function.  The default value is a function that does whatever
> interrupt-process is doing now, and Tramp can replace it with its own
> function.
>
> Does this make sense in your case?

Perfect. As long as no other package replaces Tramp's function.

Maybe the variable shall keep a list of functions, which are tried until
one of the functions reports success (returns t, for example). The
default function would be the last in the queue.

Best regards, Michael.

bug#28066: eshell-tramp sudo ignores C-c

[Yegor Timoshenko]

> Finally, I have implemented `interrupt-process' for remote
> processes. I've committed 296472f5c5 to the Emacs repository. Yegor, do
> you have a chance to test Emacs 26.1, whether this works for you?

I've built 296472f and it does work for me! Now I can interrupt
eshell/sudo processes. Eshell is the only shell that I directly use,
so that means a lot to me.

Thank you very much, Michael!

bug#28066: eshell-tramp sudo ignores C-c

[Michael Albinus]

Version: 26.1

Yegor Timoshenko <yegortimoshenko@gmail.com> writes:

Hi Yegor,

> I've built 296472f and it does work for me! Now I can interrupt
> eshell/sudo processes. Eshell is the only shell that I directly use,
> so that means a lot to me.

Thanks for confirmation, I'm closing the bug. We'll might change
implementation details, but this shouldn't hurt you.

> Thank you very much, Michael!

Best regards, Michael.

bug#28066: eshell-tramp sudo ignores C-c

[Michael Albinus]

[-- Attachment #1: Type: text/plain, Size: 355 bytes --]

Eli Zaretskii <eliz@gnu.org> writes:

Hi Eli,

> In such cases, we usually provide a variable whose value is a
> function.  The default value is a function that does whatever
> interrupt-process is doing now, and Tramp can replace it with its own
> function.
>
> Does this make sense in your case?

What about the appended patch?

Best regards, Michael.


[-- Attachment #2: Type: text/plain, Size: 2805 bytes --]

diff --git a/src/process.c b/src/process.c
index 1900951533..e7ee99ab3d 100644
--- a/src/process.c
+++ b/src/process.c
@@ -6677,6 +6677,18 @@ process_send_signal (Lisp_Object process, int signo, Lisp_Object current_group,
   unblock_child_signal (&oldset);
 }
 
+DEFUN ("internal-default-interrupt-process",
+       Finternal_default_interrupt_process,
+       Sinternal_default_interrupt_process, 0, 2, 0,
+       doc: /* Default function to interrupt process PROCESS.
+It shall be the last element in list `interrupt-process-functions'.
+See function `interrupt-process' for more details on usage.  */)
+  (Lisp_Object process, Lisp_Object current_group)
+{
+  process_send_signal (process, SIGINT, current_group, 0);
+  return process;
+}
+
 DEFUN ("interrupt-process", Finterrupt_process, Sinterrupt_process, 0, 2, 0,
        doc: /* Interrupt process PROCESS.
 PROCESS may be a process, a buffer, or the name of a process or buffer.
@@ -6688,11 +6700,14 @@ If the process is a shell, this means interrupt current subjob
 rather than the shell.
 
 If CURRENT-GROUP is `lambda', and if the shell owns the terminal,
-don't send the signal.  */)
+don't send the signal.
+
+This function calls the functions of `interrupt-process-functions' in
+the order of the list, until one of them returns non-`nil'.  */)
   (Lisp_Object process, Lisp_Object current_group)
 {
-  process_send_signal (process, SIGINT, current_group, 0);
-  return process;
+  return CALLN (Frun_hook_with_args_until_success, Qinterrupt_process_functions,
+		process, current_group);
 }
 
 DEFUN ("kill-process", Fkill_process, Skill_process, 0, 2, 0,
@@ -8176,6 +8191,17 @@ non-nil value means that the delay is not reset on write.
 The variable takes effect when `start-process' is called.  */);
   Vprocess_adaptive_read_buffering = Qt;
 
+  DEFVAR_LISP ("interrupt-process-functions", Vinterrupt_process_functions,
+	       doc: /* List of functions to be called for `interrupt-function'.
+The arguments of the functions are the same as for `interrupt-function'.
+These functions are called in the order of the list, until one of them
+returns non-`nil'.  */);
+  Vinterrupt_process_functions = list1 (Qinternal_default_interrupt_process);
+
+  DEFSYM (Qinternal_default_interrupt_process,
+	  "internal-default-interrupt-process");
+  DEFSYM (Qinterrupt_process_functions, "interrupt-process-functions");
+
   defsubr (&Sprocessp);
   defsubr (&Sget_process);
   defsubr (&Sdelete_process);
@@ -8218,6 +8244,7 @@ The variable takes effect when `start-process' is called.  */);
   defsubr (&Saccept_process_output);
   defsubr (&Sprocess_send_region);
   defsubr (&Sprocess_send_string);
+  defsubr (&Sinternal_default_interrupt_process);
   defsubr (&Sinterrupt_process);
   defsubr (&Skill_process);
   defsubr (&Squit_process);

bug#28066: eshell-tramp sudo ignores C-c

[Eli Zaretskii]

> From: Michael Albinus <michael.albinus@gmx.de>
> Cc: rms@gnu.org,  28066@debbugs.gnu.org,  yegortimoshenko@gmail.com
> Date: Mon, 21 Aug 2017 13:36:06 +0200
> 
> > In such cases, we usually provide a variable whose value is a
> > function.  The default value is a function that does whatever
> > interrupt-process is doing now, and Tramp can replace it with its own
> > function.
> >
> > Does this make sense in your case?
> 
> What about the appended patch?

LGTM, but did you consider to define the variable in Lisp?

bug#28066: eshell-tramp sudo ignores C-c

[Michael Albinus]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Michael Albinus <michael.albinus@gmx.de>
>> Cc: rms@gnu.org,  28066@debbugs.gnu.org,  yegortimoshenko@gmail.com
>> Date: Mon, 21 Aug 2017 13:36:06 +0200
>> 
>> > In such cases, we usually provide a variable whose value is a
>> > function.  The default value is a function that does whatever
>> > interrupt-process is doing now, and Tramp can replace it with its own
>> > function.
>> >
>> > Does this make sense in your case?
>> 
>> What about the appended patch?
>
> LGTM, but did you consider to define the variable in Lisp?

Sure, Vinterrupt_process_functions is declared with DEFVAR_LISP, see the
end of the patch. And it works, I've tested already my changed Tramp
implementation :-)

Anyway, I'll commit it to the master. People could raise their concerns
then, if any. Updating the documentation will be the next step afterwards.

Best regards, Michael.