Merge pull request #6082 from chu11/issue5968_libsubprocess_remote_su…

…bprocess_flags

libsubprocess: invert SETPGRP flag logic

src/broker/runat.c

@@ -350,10 +350,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

@@ -111,6 +111,7 @@ static int child_create (struct proxy_command *ctx,
         .on_stderr = NULL,
     };
     flux_cmd_t *cmd = NULL;
+    int flags = 0;
     int i;
 
     if (!shell)
@@ -151,10 +152,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

@@ -799,15 +799,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,
@@ -377,9 +377,9 @@ static void server_exec_cb (flux_t *h,
     }
 
     if (flux_subprocess_aux_set (p,
-                                msgkey,
-                                (void *)flux_msg_incref (msg),
-                                (flux_free_f)flux_msg_decref) < 0) {
+                                 msgkey,
+                                 (void *)flux_msg_incref (msg),
+                                 (flux_free_f)flux_msg_decref) < 0) {
         flux_msg_decref (msg);
         goto error;
     }

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) {
@@ -953,7 +953,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,9 +57,10 @@ 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,
-    /* use fork(2)/exec(2) even if posix_spawn(3) available */
+    /* 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,
     /* flux_rexec(): In order to improve performance, do not locally
      * copy and buffer output from the remote subprocess.  Immediately

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,