libsubprocess: invert SETPGRP flag logic

Problem: Remote subprocesses default to always set the SETPGRP
flag.  This leads to inconsistent behavior where local subprocesses
don't have the flag set but remote ones do.

We could remove this default and require remote subprocess execution
to set this flag as needed.  However, it ends up that the vast majority
of code in flux-core sets the SETPGRP flag anyways, leading to an excess
amount of code change to make local vs remote subprocesses consistent.

Solution: Invert the SETPGRP flag logic into NO_SETPGRP.  Now, by default,
have both local and remote subprocesses call setpgrp(2).  The NO_SETPGRP
flag will then disable the call to setpgrp(2).  Update all callers
to libsubprocess accordingly.

Fixes #5968

src/broker/runat.c

@@ -351,10 +351,10 @@ static struct runat_command *runat_command_create (char **env, int flags)
     if (flags & RUNAT_FLAG_FORK_EXEC)
         cmd->flags |= FLUX_SUBPROCESS_FLAGS_FORK_EXEC;
     /*
-     * Always run runat command in separate process group so that any
-     * processes spawned by command are signaled by flux_subprocess_signal()
+     * N.B. By default subprocesses call setpgrp() before exec(2).  So
+     * any processes spawned by command are also signaled by
+     * flux_subprocess_signal()
      */
-    cmd->flags |= FLUX_SUBPROCESS_FLAGS_SETPGRP;
     if (!(cmd->cmd = flux_cmd_create (0, NULL, env)))
         goto error;
     return cmd;

src/cmd/builtin/proxy.c

@@ -114,6 +114,7 @@ static int child_create (struct proxy_command *ctx,
         .on_stderr = NULL,
     };
     flux_cmd_t *cmd = NULL;
+    int flags = 0;
     int i;
 
     if (!shell)
@@ -154,10 +155,12 @@ static int child_create (struct proxy_command *ctx,
         goto error;
 
     /* We want stdio fallthrough so subprocess can capture tty if
-     * necessary (i.e. an interactive shell)
+     * necessary (i.e. an interactive shell).
      */
+    flags |= FLUX_SUBPROCESS_FLAGS_STDIO_FALLTHROUGH;
+    flags |= FLUX_SUBPROCESS_FLAGS_NO_SETPGRP;
     if (!(p = flux_local_exec (flux_get_reactor (ctx->h),
-                               FLUX_SUBPROCESS_FLAGS_STDIO_FALLTHROUGH,
+                               flags,
                                cmd,
                                &ops)))
         goto error;

src/cmd/flux-start.c

@@ -735,15 +735,18 @@ int client_run (struct client *cli)
         .on_stdout = NULL,
         .on_stderr = NULL,
     };
+    int flags = 0;
     if (cli->p) {
         errno = EEXIST;
         return -1;
     }
     /* We want stdio fallthrough so subprocess can capture tty if
      * necessary (i.e. an interactive shell)
      */
+    flags |= FLUX_SUBPROCESS_FLAGS_STDIO_FALLTHROUGH;
+    flags |= FLUX_SUBPROCESS_FLAGS_NO_SETPGRP;
     if (!(cli->p = flux_local_exec (ctx.reactor,
-                                    FLUX_SUBPROCESS_FLAGS_STDIO_FALLTHROUGH,
+                                    flags,
                                     cli->cmd,
                                     &ops)))
         log_err_exit ("flux_exec");

src/common/libsubprocess/fork.c

@@ -166,7 +166,7 @@ static int local_child (flux_subprocess_t *p)
         p->in_hook = false;
     }
 
-    if (p->flags & FLUX_SUBPROCESS_FLAGS_SETPGRP
+    if (!(p->flags & FLUX_SUBPROCESS_FLAGS_NO_SETPGRP)
         && getpgrp () != getpid ()) {
         if (setpgrp () < 0) {
             fprintf (stderr, "setpgrp: %s\n", strerror (errno));

src/common/libsubprocess/posix_spawn.c

@@ -136,7 +136,7 @@ int create_process_spawn (flux_subprocess_t *p)
     /*  If setpgrp(2) is desired for the child process, then add this
      *  flag to the spawnattr flags.
      */
-    if (p->flags & FLUX_SUBPROCESS_FLAGS_SETPGRP)
+    if (!(p->flags & FLUX_SUBPROCESS_FLAGS_NO_SETPGRP))
         flags |= POSIX_SPAWN_SETPGROUP;
     posix_spawnattr_setflags (&attr, flags);
 

src/common/libsubprocess/server.c

@@ -365,7 +365,7 @@ static void server_exec_cb (flux_t *h,
     flux_cmd_unsetenv (cmd, "FLUX_PROXY_REMOTE");
 
     if (!(p = flux_local_exec_ex (flux_get_reactor (s->h),
-                                  FLUX_SUBPROCESS_FLAGS_SETPGRP,
+                                  0,
                                   cmd,
                                   &ops,
                                   NULL,

src/common/libsubprocess/subprocess.c

@@ -463,7 +463,7 @@ flux_subprocess_t *flux_local_exec_ex (flux_reactor_t *r,
 {
     flux_subprocess_t *p = NULL;
     int valid_flags = (FLUX_SUBPROCESS_FLAGS_STDIO_FALLTHROUGH
-                       | FLUX_SUBPROCESS_FLAGS_SETPGRP
+                       | FLUX_SUBPROCESS_FLAGS_NO_SETPGRP
                        | FLUX_SUBPROCESS_FLAGS_FORK_EXEC);
 
     if (!r || !cmd) {
@@ -946,7 +946,7 @@ flux_future_t *flux_subprocess_kill (flux_subprocess_t *p, int signum)
             errno = ESRCH;
             return NULL;
         }
-        if (p->flags & FLUX_SUBPROCESS_FLAGS_SETPGRP)
+        if (!(p->flags & FLUX_SUBPROCESS_FLAGS_NO_SETPGRP))
             ret = killpg (p->pid, signum);
         else
             ret = kill (p->pid, signum);

src/common/libsubprocess/subprocess.h

@@ -57,8 +57,8 @@ enum {
      * will fail on streams of "stdin", "stdout", or "stderr".
      */
     FLUX_SUBPROCESS_FLAGS_STDIO_FALLTHROUGH = 1,
-    /* flux_local_exec(): call setpgrp() before exec(2) */
-    FLUX_SUBPROCESS_FLAGS_SETPGRP = 2,
+    /* flux_local_exec(): do not call setpgrp() before exec(2) */
+    FLUX_SUBPROCESS_FLAGS_NO_SETPGRP = 2,
     /* flux_local_exec(): use fork(2)/exec(2) even if posix_spawn(3)
      * available */
     FLUX_SUBPROCESS_FLAGS_FORK_EXEC = 4,

src/common/libsubprocess/test/subprocess.c

@@ -323,7 +323,7 @@ void test_basic_fail (flux_reactor_t *r)
     flux_cmd_destroy (cmd);
 }
 
-void test_flag_setpgrp (flux_reactor_t *r)
+void test_flag_no_setpgrp (flux_reactor_t *r)
 {
     char *av[] = { "/bin/true", NULL };
     flux_cmd_t *cmd;
@@ -335,7 +335,7 @@ void test_flag_setpgrp (flux_reactor_t *r)
         .on_completion = completion_cb
     };
     completion_cb_count = 0;
-    p = flux_local_exec (r, FLUX_SUBPROCESS_FLAGS_SETPGRP, cmd, &ops);
+    p = flux_local_exec (r, FLUX_SUBPROCESS_FLAGS_NO_SETPGRP, cmd, &ops);
     ok (p != NULL, "flux_local_exec");
 
     ok (flux_subprocess_state (p) == FLUX_SUBPROCESS_RUNNING,
@@ -551,7 +551,7 @@ void test_kill_setpgrp (flux_reactor_t *r)
     output_processes_cb_count = 0;
     parent_pid = -1;
     child_pid = -1;
-    p = flux_local_exec (r, FLUX_SUBPROCESS_FLAGS_SETPGRP, cmd, &ops);
+    p = flux_local_exec (r, 0, cmd, &ops);
     ok (p != NULL, "flux_local_exec");
 
     ok (flux_subprocess_state (p) == FLUX_SUBPROCESS_RUNNING,
@@ -1097,8 +1097,8 @@ int main (int argc, char *argv[])
     test_basic_fail (r);
     diag ("env_passed");
     test_env_passed (r);
-    diag ("flag_setpgrp");
-    test_flag_setpgrp (r);
+    diag ("flag_no_setpgrp");
+    test_flag_no_setpgrp (r);
     diag ("flag_fork_exec");
     test_flag_fork_exec (r);
     diag ("kill");

src/shell/task.c

@@ -222,15 +222,15 @@ int shell_task_start (struct flux_shell *shell,
                       shell_task_completion_f cb,
                       void *arg)
 {
-    int flags = FLUX_SUBPROCESS_FLAGS_SETPGRP;
+    int flags = 0;
     flux_reactor_t *r = shell->r;
     flux_subprocess_hooks_t hooks = {
         .pre_exec = subproc_preexec_hook,
         .pre_exec_arg = task,
     };
 
     if (shell->nosetpgrp)
-        flags &= ~FLUX_SUBPROCESS_FLAGS_SETPGRP;
+        flags |= FLUX_SUBPROCESS_FLAGS_NO_SETPGRP;
 
     task->proc = flux_local_exec_ex (r,
                                      flags,