diff mbox series

Allow enabling job control without a tty in non-interactive mode..

Message ID dedaa3fa370ea9c4aeb1771b5568a7bef4065b04.1675113321.git.steffen@sdaoden.eu (mailing list archive)
State Superseded
Delegated to: Herbert Xu
Headers show
Series Allow enabling job control without a tty in non-interactive mode.. | expand

Commit Message

Steffen Nurpmeso Jan. 30, 2023, 9:15 p.m. UTC
This is a take-over of the FreeBSD bin/sh

  commit cd60e2c67d52e1f957841af19128c7227880743a
  Author:     Jilles Tjoelker <jilles@FreeBSD.org>
  AuthorDate: 2014-09-04 21:48:33 +0000
  Commit:     Jilles Tjoelker <jilles@FreeBSD.org>
  CommitDate: 2014-09-04 21:48:33 +0000

      sh: Allow enabling job control without a tty in non-interactive mode.

      If no tty is available, 'set -m' is still useful to put jobs in their own
      process groups.
---

Dear Ganael, it seems it requires an inline patch?
Let me try this -- thanks!!
Ciao.

 src/jobs.c | 114 +++++++++++++++++++++++++++++++----------------------
 1 file changed, 67 insertions(+), 47 deletions(-)

Comments

Herbert Xu April 6, 2024, 7:22 a.m. UTC | #1
On Mon, Jan 30, 2023 at 10:15:40PM +0100, Steffen Nurpmeso wrote:
> This is a take-over of the FreeBSD bin/sh
> 
>   commit cd60e2c67d52e1f957841af19128c7227880743a
>   Author:     Jilles Tjoelker <jilles@FreeBSD.org>
>   AuthorDate: 2014-09-04 21:48:33 +0000
>   Commit:     Jilles Tjoelker <jilles@FreeBSD.org>
>   CommitDate: 2014-09-04 21:48:33 +0000
> 
>       sh: Allow enabling job control without a tty in non-interactive mode.
> 
>       If no tty is available, 'set -m' is still useful to put jobs in their own
>       process groups.

I don't see why dash needs this.  Please keep in mind that one
of the primary goals of dash it to be minimal.

Thanks,
Steffen Nurpmeso April 6, 2024, 11:07 p.m. UTC | #2
Herbert Xu wrote in
 <ZhD4JcqNct+rmxca@gondor.apana.org.au>:
 |On Mon, Jan 30, 2023 at 10:15:40PM +0100, Steffen Nurpmeso wrote:
 |> This is a take-over of the FreeBSD bin/sh
 |> 
 |>   commit cd60e2c67d52e1f957841af19128c7227880743a
 |>   Author:     Jilles Tjoelker <jilles@FreeBSD.org>
 |>   AuthorDate: 2014-09-04 21:48:33 +0000
 |>   Commit:     Jilles Tjoelker <jilles@FreeBSD.org>
 |>   CommitDate: 2014-09-04 21:48:33 +0000
 |> 
 |>       sh: Allow enabling job control without a tty in non-interactive \
 |>       mode.
 |> 
 |>       If no tty is available, 'set -m' is still useful to put jobs \
 |>       in their own
 |>       process groups.
 |
 |I don't see why dash needs this.  Please keep in mind that one
 |of the primary goals of dash it to be minimal.

Isn't that standardized answer a bit misplaced for that patch?
It was about Ganael Laplanche having problems with creating
process groups in backgrounded jobs (2023-01-12):

  The problem is when the main script is started *in the
  background*. Can your test script successfully be run in the
  background with dash if $JOBMON is not empty ? I think you'll
  face the same problem as I do.

  I could not find Jilles' commit (cd60e2c) equivalent in dash
  code, so I presume enabling job control without a tty in
  non-interactive mode is just not possible with dash, am I right

And i stated that another indirection fixes that problem and he
then said

  It is not clear for me why that extra fork fixes the problem,
  but it works, thanks!

  Unfortunately, my initial goal is to get a new process group for
  later children processes, but FreeBSD's sh (as well as dash)
  requires 'set -m' to be executed from the first process [1]. The
  extra fork breaks that requirement.  The following example shows
  that the new process' PGID remains the same as the initial
  shell:

which let me baffled

  Thanks for this information, that i did not expect.
  Indeed .. it seems no new process group is used for the child
  shell.  That thoroughly i have not looked.

and

  So i think i am out of ideas except doing what Jilles suggested
[Tjoelker]
  in the message, enwrapping the inner thing with sh -c '..'.  And
  that seems to work a bit as

and Ganael ended (without the patch) like

  Yes, I am afraid it's the only syntax that works (but it is not
  very convenient).

etc. etc. etc.

Full stop.

Please consider this script:

  cat > t.sh <<'_EOT'
  set -m
  (
  sleep 1
  ) &
  i=$!
  set +m
  echo >&2 "inner Main shell has: $(ps -o pid,pgid $$)"
  echo >&2 "inner Sub-shell has: $(ps -o pid,pgid $i)"
  wait $i
  _EOT

Current dash:

  #?0|kent:dash.git$ dash t.sh </dev/null >.X 2>&1 &
  [3] 28719
  #?0|kent:dash.git$
^RETURN

  [3]+  Stopped                 dash t.sh < /dev/null > .X 2>&1
  #?0|kent:dash.git$ fg
  dash t.sh < /dev/null > .X 2>&1

Patched dash:

  #?0|kent:dash.git$ src/dash t.sh </dev/null >.X 2>&1 &
  [3] 28749
  #?0|kent:dash.git$
^RETURN
  [3]   Done                    src/dash t.sh < /dev/null > .X 2>&1

Comparison:

  #?0|kent:dash.git$ bash t.sh </dev/null >.X 2>&1 &
  [3] 28766
  #?0|kent:dash.git$
^RETURN
  [3]   Done                    bash t.sh < /dev/null > .X 2>&1

A nice Sunday everybody!
Ciao from Germany,

--steffen
|
|Der Kragenbaer,                The moon bear,
|der holt sich munter           he cheerfully and one by one
|einen nach dem anderen runter  wa.ks himself off
|(By Robert Gernhardt)
Herbert Xu April 7, 2024, 5:21 a.m. UTC | #3
On Sun, Apr 07, 2024 at 01:07:04AM +0200, Steffen Nurpmeso wrote:
> Herbert Xu wrote in
>  <ZhD4JcqNct+rmxca@gondor.apana.org.au>:
>  |On Mon, Jan 30, 2023 at 10:15:40PM +0100, Steffen Nurpmeso wrote:
>  |> This is a take-over of the FreeBSD bin/sh
>  |> 
>  |>   commit cd60e2c67d52e1f957841af19128c7227880743a
>  |>   Author:     Jilles Tjoelker <jilles@FreeBSD.org>
>  |>   AuthorDate: 2014-09-04 21:48:33 +0000
>  |>   Commit:     Jilles Tjoelker <jilles@FreeBSD.org>
>  |>   CommitDate: 2014-09-04 21:48:33 +0000
>  |> 
>  |>       sh: Allow enabling job control without a tty in non-interactive \
>  |>       mode.
>  |> 
>  |>       If no tty is available, 'set -m' is still useful to put jobs \
>  |>       in their own
>  |>       process groups.
>  |
>  |I don't see why dash needs this.  Please keep in mind that one
>  |of the primary goals of dash it to be minimal.
> 
> Isn't that standardized answer a bit misplaced for that patch?
> It was about Ganael Laplanche having problems with creating
> process groups in backgrounded jobs (2023-01-12):

Sorry, I'd lost the context to the original discussion.  I'll
look into your patch again.

Thanks,
diff mbox series

Patch

diff --git a/src/jobs.c b/src/jobs.c
index f3b9ffc285..db1f204045 100644
--- a/src/jobs.c
+++ b/src/jobs.c
@@ -125,7 +125,7 @@  STATIC int getstatus(struct job *);
 
 #if JOBS
 static int restartjob(struct job *, int);
-static void xtcsetpgrp(int, pid_t);
+static void xtcsetpgrp(pid_t);
 #endif
 
 STATIC void
@@ -174,70 +174,84 @@  set_curjob(struct job *jp, unsigned mode)
 	}
 }
 
-#if JOBS
 /*
  * Turn job control on and off.
- *
- * Note:  This code assumes that the third arg to ioctl is a character
- * pointer, which is true on Berkeley systems but not System V.  Since
- * System V doesn't have job control yet, this isn't a problem now.
- *
- * Called with interrupts off.
  */
 
 int jobctl;
 
+#if JOBS
+static void
+jobctl_notty(void)
+{
+	if (ttyfd >= 0) {
+		close(ttyfd);
+		ttyfd = -1;
+	}
+	if (!iflag) {
+		setsignal(SIGTSTP);
+		setsignal(SIGTTOU);
+		setsignal(SIGTTIN);
+		jobctl = 1;
+		return;
+	}
+	sh_warnx("can't access tty; job control turned off");
+	mflag = 0;
+}
+
 void
 setjobctl(int on)
 {
-	int fd;
-	int pgrp;
+	int i;
 
 	if (on == jobctl || rootshell == 0)
 		return;
 	if (on) {
-		int ofd;
-		ofd = fd = sh_open(_PATH_TTY, O_RDWR, 1);
-		if (fd < 0) {
-			fd += 3;
-			while (!isatty(fd))
-				if (--fd < 0)
-					goto out;
+		if (ttyfd != -1)
+			close(ttyfd);
+		if ((ttyfd = sh_open(_PATH_TTY, O_RDWR, 1)) < 0) {
+			i = 0;
+			while (i <= 2 && !isatty(i))
+				i++;
+			if (i > 2 ||
+			    (ttyfd = fcntl(i, F_DUPFD_CLOEXEC, 10)) < 0) {
+				jobctl_notty();
+				return;
+			}
 		}
-		fd = savefd(fd, ofd);
+		ttyfd = savefd(ttyfd, ttyfd);
 		do { /* while we are in the background */
-			if ((pgrp = tcgetpgrp(fd)) < 0) {
-out:
-				sh_warnx("can't access tty; job control turned off");
-				mflag = on = 0;
-				goto close;
+			initialpgrp = tcgetpgrp(ttyfd);
+			if (initialpgrp < 0) {
+				jobctl_notty();
+				return;
 			}
-			if (pgrp == getpgrp())
-				break;
-			killpg(0, SIGTTIN);
-		} while (1);
-		initialpgrp = pgrp;
-
+			if (initialpgrp != getpgrp()) {
+				if (!iflag) {
+					initialpgrp = -1;
+					jobctl_notty();
+					return;
+				}
+				kill(0, SIGTTIN);
+				continue;
+			}
+		} while (0);
 		setsignal(SIGTSTP);
 		setsignal(SIGTTOU);
 		setsignal(SIGTTIN);
-		pgrp = rootpid;
-		setpgid(0, pgrp);
-		xtcsetpgrp(fd, pgrp);
-	} else {
-		/* turning job control off */
-		fd = ttyfd;
-		pgrp = initialpgrp;
-		xtcsetpgrp(fd, pgrp);
-		setpgid(0, pgrp);
+		setpgid(0, rootpid);
+		tcsetpgrp(ttyfd, rootpid);
+	} else { /* turning job control off */
+		setpgid(0, initialpgrp);
+		if (ttyfd >= 0) {
+			tcsetpgrp(ttyfd, initialpgrp);
+			close(ttyfd);
+			ttyfd = -1;
+		}
 		setsignal(SIGTSTP);
 		setsignal(SIGTTOU);
 		setsignal(SIGTTIN);
-close:
-		close(fd);
-		fd = -1;
 	}
-	ttyfd = fd;
 	jobctl = on;
 }
 #endif
@@ -394,7 +408,7 @@  restartjob(struct job *jp, int mode)
 	jp->state = JOBRUNNING;
 	pgid = jp->ps->pid;
 	if (mode == FORK_FG)
-		xtcsetpgrp(ttyfd, pgid);
+		xtcsetpgrp(pgid);
 	killpg(pgid, SIGCONT);
 	ps = jp->ps;
 	i = jp->nprocs;
@@ -457,6 +471,9 @@  showjob(struct output *out, struct job *jp, int mode)
 	int indent;
 	char s[80];
 
+	if (!iflag)
+		return;
+
 	ps = jp->ps;
 
 	if (mode & SHOW_PGID) {
@@ -878,7 +895,7 @@  static void forkchild(struct job *jp, union node *n, int mode)
 		/* This can fail because we are doing it in the parent also */
 		(void)setpgid(0, pgrp);
 		if (mode == FORK_FG)
-			xtcsetpgrp(ttyfd, pgrp);
+			xtcsetpgrp(pgrp);
 		setsignal(SIGTSTP);
 		setsignal(SIGTTOU);
 	} else
@@ -1018,7 +1035,7 @@  waitforjob(struct job *jp)
 	st = getstatus(jp);
 #if JOBS
 	if (jp->jobctl) {
-		xtcsetpgrp(ttyfd, rootpid);
+		xtcsetpgrp(rootpid);
 		/*
 		 * This is truly gross.
 		 * If we're doing job control, then we did a TIOCSPGRP which
@@ -1508,12 +1525,15 @@  showpipe(struct job *jp, struct output *out)
 
 #if JOBS
 STATIC void
-xtcsetpgrp(int fd, pid_t pgrp)
+xtcsetpgrp(pid_t pgrp)
 {
 	int err;
 
+	if (ttyfd < 0)
+		return;
+
 	sigblockall(NULL);
-	err = tcsetpgrp(fd, pgrp);
+	err = tcsetpgrp(ttyfd, pgrp);
 	sigclearmask();
 
 	if (err)