Patchwork [3,of,3] chg: forward job control signals to worker process (issue5051)

login
register
mail settings
Submitter Yuya Nishihara
Date Feb. 11, 2016, 2:05 p.m.
Message ID <63251841f5f927f677b9.1455199513@mimosa>
Download mbox | patch
Permalink /patch/13114/
State Accepted
Headers show

Comments

Yuya Nishihara - Feb. 11, 2016, 2:05 p.m.
# HG changeset patch
# User Yuya Nishihara <yuya@tcha.org>
# Date 1453210319 -32400
#      Tue Jan 19 22:31:59 2016 +0900
# Node ID 63251841f5f927f677b90957eec1230017f3a515
# Parent  def82508e22c33bdfe042fd372bddf6dc3f93e2c
chg: forward job control signals to worker process (issue5051)

This is necessary to suspend/resume long pulls, interactive curses session,
etc.

The implementation is based on emacsclient, but our version doesn't test if
chg process is foreground or not before propagating SIGCONT. This is because
chg isn't always an interactive session. If we copy the SIGTTIN/SIGTTOU
emulation from emacsclient, non-interactive session can't be moved to a
background job.

  $ chg pull
  ^Z
  suspended
  $ bg %1
  [1] continued
  [1] suspended (tty input)  # wrong

https://github.com/emacs-mirror/emacs/blob/0e96320/lib-src/emacsclient.c#L1094
Sean Farley - Feb. 11, 2016, 8:05 p.m.
Yuya Nishihara <yuya@tcha.org> writes:

> # HG changeset patch
> # User Yuya Nishihara <yuya@tcha.org>
> # Date 1453210319 -32400
> #      Tue Jan 19 22:31:59 2016 +0900
> # Node ID 63251841f5f927f677b90957eec1230017f3a515
> # Parent  def82508e22c33bdfe042fd372bddf6dc3f93e2c
> chg: forward job control signals to worker process (issue5051)
>
> This is necessary to suspend/resume long pulls, interactive curses session,
> etc.
>
> The implementation is based on emacsclient, but our version doesn't test if
> chg process is foreground or not before propagating SIGCONT. This is because
> chg isn't always an interactive session. If we copy the SIGTTIN/SIGTTOU
> emulation from emacsclient, non-interactive session can't be moved to a
> background job.
>
>   $ chg pull
>   ^Z
>   suspended
>   $ bg %1
>   [1] continued
>   [1] suspended (tty input)  # wrong
>
> https://github.com/emacs-mirror/emacs/blob/0e96320/lib-src/emacsclient.c#L1094

Took me a while but I was finally able to verify the before behavior and
that these patches do indeed fix it. Great job, Yuya!
Yuya Nishihara - Feb. 12, 2016, 3:26 a.m.
On Thu, 11 Feb 2016 12:05:28 -0800, Sean Farley wrote:
> Yuya Nishihara <yuya@tcha.org> writes:
> > # HG changeset patch
> > # User Yuya Nishihara <yuya@tcha.org>
> > # Date 1453210319 -32400
> > #      Tue Jan 19 22:31:59 2016 +0900
> > # Node ID 63251841f5f927f677b90957eec1230017f3a515
> > # Parent  def82508e22c33bdfe042fd372bddf6dc3f93e2c
> > chg: forward job control signals to worker process (issue5051)
> >
> > This is necessary to suspend/resume long pulls, interactive curses session,
> > etc.
> >
> > The implementation is based on emacsclient, but our version doesn't test if
> > chg process is foreground or not before propagating SIGCONT. This is because
> > chg isn't always an interactive session. If we copy the SIGTTIN/SIGTTOU
> > emulation from emacsclient, non-interactive session can't be moved to a
> > background job.
> >
> >   $ chg pull
> >   ^Z
> >   suspended
> >   $ bg %1
> >   [1] continued
> >   [1] suspended (tty input)  # wrong
> >
> > https://github.com/emacs-mirror/emacs/blob/0e96320/lib-src/emacsclient.c#L1094
> 
> Took me a while but I was finally able to verify the before behavior and
> that these patches do indeed fix it. Great job, Yuya!

This problem was mostly investigated by Jun Wu. So kudos to him!
Pierre-Yves David - Feb. 12, 2016, 2:47 p.m.
On 02/11/2016 02:05 PM, Yuya Nishihara wrote:
> # HG changeset patch
> # User Yuya Nishihara <yuya@tcha.org>
> # Date 1453210319 -32400
> #      Tue Jan 19 22:31:59 2016 +0900
> # Node ID 63251841f5f927f677b90957eec1230017f3a515
> # Parent  def82508e22c33bdfe042fd372bddf6dc3f93e2c
> chg: forward job control signals to worker process (issue5051)
>
> This is necessary to suspend/resume long pulls, interactive curses session,
> etc.
>
> The implementation is based on emacsclient, but our version doesn't test if
> chg process is foreground or not before propagating SIGCONT. This is because
> chg isn't always an interactive session. If we copy the SIGTTIN/SIGTTOU
> emulation from emacsclient, non-interactive session can't be moved to a
> background job.


This seems reasonable to me and I'm adding Jun, Yuya and Sean for 
confidence. I've this pushed to the clowncopter.

Thanks.

Patch

diff --git a/contrib/chg/chg.c b/contrib/chg/chg.c
--- a/contrib/chg/chg.c
+++ b/contrib/chg/chg.c
@@ -231,6 +231,38 @@  static void forwardsignal(int sig)
 	debugmsg("forward signal %d", sig);
 }
 
+static void handlestopsignal(int sig)
+{
+	sigset_t unblockset, oldset;
+	struct sigaction sa, oldsa;
+	if (sigemptyset(&unblockset) < 0)
+		goto error;
+	if (sigaddset(&unblockset, sig) < 0)
+		goto error;
+	memset(&sa, 0, sizeof(sa));
+	sa.sa_handler = SIG_DFL;
+	sa.sa_flags = SA_RESTART;
+	if (sigemptyset(&sa.sa_mask) < 0)
+		goto error;
+
+	forwardsignal(sig);
+	if (raise(sig) < 0)  /* resend to self */
+		goto error;
+	if (sigaction(sig, &sa, &oldsa) < 0)
+		goto error;
+	if (sigprocmask(SIG_UNBLOCK, &unblockset, &oldset) < 0)
+		goto error;
+	/* resent signal will be handled before sigprocmask() returns */
+	if (sigprocmask(SIG_SETMASK, &oldset, NULL) < 0)
+		goto error;
+	if (sigaction(sig, &oldsa, NULL) < 0)
+		goto error;
+	return;
+
+error:
+	abortmsg("failed to handle stop signal (errno = %d)", errno);
+}
+
 static void setupsignalhandler(pid_t pid)
 {
 	if (pid <= 0)
@@ -253,6 +285,17 @@  static void setupsignalhandler(pid_t pid
 	sa.sa_flags |= SA_RESETHAND;
 	if (sigaction(SIGTERM, &sa, NULL) < 0)
 		goto error;
+
+	/* propagate job control requests to worker */
+	sa.sa_handler = forwardsignal;
+	sa.sa_flags = SA_RESTART;
+	if (sigaction(SIGCONT, &sa, NULL) < 0)
+		goto error;
+	sa.sa_handler = handlestopsignal;
+	sa.sa_flags = SA_RESTART;
+	if (sigaction(SIGTSTP, &sa, NULL) < 0)
+		goto error;
+
 	return;
 
 error: