flux-{start,broker}: propagate argument quoting

src/broker/broker.c

@@ -157,6 +157,7 @@ typedef struct {
     struct subprocess_manager *sm;
 
     char *init_shell_cmd;
+    size_t init_shell_cmd_len;
     struct subprocess *init_shell;
 } ctx_t;
 
@@ -358,10 +359,8 @@ int main (int argc, char *argv[])
         }
     }
     if (optind < argc) {
-        size_t len = 0;
-        if ((e = argz_create (argv + optind, &ctx.init_shell_cmd, &len)) != 0)
+        if ((e = argz_create (argv + optind, &ctx.init_shell_cmd, &ctx.init_shell_cmd_len)) != 0)
             log_errn_exit (e, "argz_create");
-        argz_stringify (ctx.init_shell_cmd, len, ' ');
     }
 
 
@@ -579,6 +578,7 @@ int main (int argc, char *argv[])
     if (ctx.rank == 0) {
         const char *rc1, *rc3, *pmi, *uri;
         const char *rc2 = ctx.init_shell_cmd;
+        size_t rc2_len = ctx.init_shell_cmd_len;
 
         if (attr_get (ctx.attrs, "local-uri", &uri, NULL) < 0)
             log_err_exit ("local-uri is not set");
@@ -594,11 +594,11 @@ int main (int argc, char *argv[])
         runlevel_set_callback (ctx.runlevel, runlevel_cb, &ctx);
         runlevel_set_io_callback (ctx.runlevel, runlevel_io_cb, &ctx);
 
-        if (runlevel_set_rc (ctx.runlevel, 1, rc1, uri) < 0)
+        if (runlevel_set_rc (ctx.runlevel, 1, rc1, rc1 ? strlen (rc1) + 1 : 0, uri) < 0)
             log_err_exit ("runlevel_set_rc 1");
-        if (runlevel_set_rc (ctx.runlevel, 2, rc2, uri) < 0)
+        if (runlevel_set_rc (ctx.runlevel, 2, rc2, rc2_len, uri) < 0)
             log_err_exit ("runlevel_set_rc 2");
-        if (runlevel_set_rc (ctx.runlevel, 3, rc3, uri) < 0)
+        if (runlevel_set_rc (ctx.runlevel, 3, rc3, rc1 ? strlen (rc3) + 1 : 0, uri) < 0)
             log_err_exit ("runlevel_set_rc 3");
     }
 

src/broker/runlevel.c

@@ -22,6 +22,7 @@
  *  See also:  http://www.gnu.org/licenses/
 \*****************************************************************************/
 
+
 #if HAVE_CONFIG_H
 #include "config.h"
 #endif
@@ -30,14 +31,12 @@
 #include <argz.h>
 #include <flux/core.h>
 
-#include "src/common/libsubprocess/subprocess.h"
 #include "src/common/libsubprocess/zio.h"
 #include "src/common/libutil/log.h"
 #include "src/common/libutil/xzmalloc.h"
 #include "src/common/libutil/shortjson.h"
 #include "src/common/libutil/monotime.h"
 
-#include "attr.h"
 #include "runlevel.h"
 
 struct level {
@@ -284,8 +283,8 @@ static int subprocess_io_cb (struct subprocess *p, const char *json_str)
     return 0;
 }
 
-int runlevel_set_rc (runlevel_t *r, int level, const char *command,
-                     const char *local_uri)
+int runlevel_set_rc (runlevel_t *r, int level, const char *cmd_argz,
+                     size_t cmd_argz_len, const char *local_uri)
 {
     struct subprocess *p = NULL;
     const char *shell = getenv ("SHELL");
@@ -297,19 +296,35 @@ int runlevel_set_rc (runlevel_t *r, int level, const char *command,
         goto error;
     }
 
-    if (!(p = subprocess_create (r->sm))
-            || subprocess_set_context (p, "runlevel", r) < 0
-            || subprocess_add_hook (p, SUBPROCESS_COMPLETE, subprocess_cb) < 0
-            || subprocess_argv_append (p, shell) < 0
-            || (command && subprocess_argv_append (p, "-c") < 0)
-            || (command && subprocess_argv_append (p, command) < 0)
-            || subprocess_set_environ (p, environ) < 0
-            || subprocess_unsetenv (p, "PMI_FD") < 0
-            || subprocess_unsetenv (p, "PMI_RANK") < 0
-            || subprocess_unsetenv (p, "PMI_SIZE") < 0
-            || (local_uri && subprocess_setenv (p, "FLUX_URI",
-                                                local_uri, 1) < 0))
+    // Only wrap in a shell if there is only one argument
+    bool shell_wrap = argz_count (cmd_argz, cmd_argz_len) < 2;
+    if ((p = subprocess_create (r->sm)) == NULL)
+        goto error;
+    if ((subprocess_set_context (p, "runlevel", r)) < 0)
+        goto error;
+    if ((subprocess_add_hook (p, SUBPROCESS_COMPLETE, subprocess_cb)) < 0)
+        goto error;
+    if (shell_wrap || !cmd_argz) {
+        if ((subprocess_argv_append (p, shell)) < 0)
+            goto error;
+    }
+    if (shell_wrap) {
+        if (cmd_argz && subprocess_argv_append (p, "-c") < 0)
+            goto error;
+    }
+    if (cmd_argz && subprocess_argv_append_argz (p, cmd_argz, cmd_argz_len) < 0)
+        goto error;
+    if (subprocess_set_environ (p, environ) < 0)
+        goto error;
+    if (subprocess_unsetenv (p, "PMI_FD") < 0)
+        goto error;
+    if (subprocess_unsetenv (p, "PMI_RANK") < 0)
         goto error;
+    if (subprocess_unsetenv (p, "PMI_SIZE") < 0)
+        goto error;
+    if (local_uri && subprocess_setenv (p, "FLUX_URI", local_uri, 1) < 0)
+        goto error;
+
     if (level == 1 || level == 3) {
         if (subprocess_setenv (p, "FLUX_NODESET_MASK", r->nodeset, 1) < 0)
             goto error;

src/broker/runlevel.h

@@ -1,6 +1,12 @@
 #ifndef _BROKER_RUNLEVEL_H
 #define _BROKER_RUNLEVEL_H
 
+#include "attr.h"
+#include "src/common/libsubprocess/subprocess.h"
+
+#include <stdint.h>
+#include <stddef.h> // for size_t
+
 typedef struct runlevel runlevel_t;
 
 typedef void (*runlevel_cb_f)(runlevel_t *r, int level, int rc, double elapsed,
@@ -28,8 +34,8 @@ void runlevel_set_io_callback (runlevel_t *r, runlevel_io_cb_f cb, void *arg);
 /* Associate 'command' with 'level'.  'local_uri' and 'library_path' are
  * used to set FLUX_URI and LD_LIBRARY_PATH in the subprocess environment.
  */
-int runlevel_set_rc (runlevel_t *r, int level, const char *command,
-                     const char *local_uri);
+int runlevel_set_rc (runlevel_t *r, int level, const char *cmd_argz,
+                     size_t cmd_argz_len, const char *local_uri);
 
 /* Change the runlevel.  It is assumed that the previous run level (if any)
  * has completed and this is being called from the runlevel callback.

src/cmd/flux-start.c

@@ -73,10 +73,10 @@ struct client {
 };
 
 void killer (flux_reactor_t *r, flux_watcher_t *w, int revents, void *arg);
-int start_session (struct context *ctx, const char *cmd);
-int exec_broker (struct context *ctx, const char *cmd);
+int start_session (struct context *ctx, const char *cmd_argz, size_t cmd_argz_len);
+int exec_broker (struct context *ctx, const char *cmd_argz, size_t cmd_argz_len);
 char *create_scratch_dir (struct context *ctx);
-struct client *client_create (struct context *ctx, int rank, const char *cmd);
+struct client *client_create (struct context *ctx, int rank, const char *cmd_argz, size_t cmd_argz_len);
 void client_destroy (struct client *cli);
 char *find_broker (const char *searchpath);
 static void setup_profiling_env (struct context *ctx);
@@ -147,7 +147,6 @@ int main (int argc, char *argv[])
     if (optindex < argc) {
         if ((e = argz_create (argv + optindex, &command, &len)) != 0)
             log_errn_exit (e, "argz_creawte");
-        argz_stringify (command, len, ' ');
     }
 
     if (!(searchpath = getenv ("FLUX_EXEC_PATH")))
@@ -162,9 +161,9 @@ int main (int argc, char *argv[])
         log_msg_exit ("--size argument must be >= 0");
 
     if (ctx->size == 1) {
-        status = exec_broker (ctx, command);
+        status = exec_broker (ctx, command, len);
     } else {
-        status = start_session (ctx, command);
+        status = start_session (ctx, command, len);
     }
 
     optparse_destroy (ctx->opts);
@@ -277,25 +276,12 @@ static int child_exit (struct subprocess *p)
     return 0;
 }
 
-void add_arg (struct subprocess *p, const char *fmt, ...)
-{
-    va_list ap;
-    char *arg;
-
-    va_start (ap, fmt);
-    arg = xvasprintf (fmt, ap);
-    va_end (ap);
-    if (subprocess_argv_append (p, arg) < 0)
-        log_err_exit ("subprocess_argv_append");
-    free (arg);
-}
-
-void add_args_list (struct subprocess *p, optparse_t *opt, const char *name)
+void add_args_list (char **argz, size_t *argz_len, optparse_t *opt, const char *name)
 {
     const char *arg;
     optparse_getopt_iterator_reset (opt, name);
     while ((arg = optparse_getopt_next (opt, name)))
-        if (subprocess_argv_append  (p, arg) < 0)
+        if (argz_add  (argz, argz_len, arg) != 0)
             log_err_exit ("subprocess_argv_append");
 }
 
@@ -361,20 +347,20 @@ int pmi_kvs_get (void *arg, const char *kvsname,
     return 0;
 }
 
-int execvp_argz (const char *cmd, char *argz, size_t argz_len)
+int execvp_argz (const char *cmd_argz, char *argz, size_t argz_len)
 {
     char **av = malloc (sizeof (char *) * (argz_count (argz, argz_len) + 1));
     if (!av) {
         errno = ENOMEM;
         return -1;
     }
     argz_extract (argz, argz_len, av);
-    execvp (cmd, av);
+    execvp (cmd_argz, av);
     free (av);
     return -1;
 }
 
-int exec_broker (struct context *ctx, const char *cmd)
+int exec_broker (struct context *ctx, const char *cmd_argz, size_t cmd_argz_len)
 {
     char *argz = NULL;
     size_t argz_len = 0;
@@ -388,8 +374,8 @@ int exec_broker (struct context *ctx, const char *cmd)
         if (argz_add (&argz, &argz_len, arg) != 0)
             goto nomem;
     }
-    if (cmd) {
-        if (argz_add (&argz, &argz_len, cmd) != 0)
+    if (cmd_argz) {
+        if (argz_append (&argz, &argz_len, cmd_argz, cmd_argz_len) != 0)
             goto nomem;
     }
     if (optparse_hasopt (ctx->opts, "verbose")) {
@@ -414,10 +400,12 @@ int exec_broker (struct context *ctx, const char *cmd)
     return -1;
 }
 
-struct client *client_create (struct context *ctx, int rank, const char *cmd)
+struct client *client_create (struct context *ctx, int rank, const char *cmd_argz, size_t cmd_argz_len)
 {
     struct client *cli = xzmalloc (sizeof (*cli));
     int client_fd;
+    char * argz = NULL;
+    size_t argz_len = 0;
 
     cli->rank = rank;
     cli->fd = -1;
@@ -427,12 +415,17 @@ struct client *client_create (struct context *ctx, int rank, const char *cmd)
     subprocess_set_context (cli->p, "cli", cli);
     subprocess_add_hook (cli->p, SUBPROCESS_COMPLETE, child_exit);
     subprocess_add_hook (cli->p, SUBPROCESS_STATUS, child_report);
-    add_arg (cli->p, "%s", ctx->broker_path);
-    add_arg (cli->p, "--shared-ipc-namespace");
-    add_arg (cli->p, "--setattr=scratch-directory=%s", ctx->scratch_dir);
-    add_args_list (cli->p, ctx->opts, "broker-opts");
-    if (rank == 0 && cmd)
-        add_arg (cli->p, "%s", cmd); /* must be last arg */
+    argz_add (&argz, &argz_len, ctx->broker_path);
+    argz_add (&argz, &argz_len, "--shared-ipc-namespace");
+    char * scratch_dir = xasprintf ("--setattr=scratch-directory=%s", ctx->scratch_dir);
+    argz_add (&argz, &argz_len, scratch_dir);
+    free(scratch_dir);
+    add_args_list (&argz, &argz_len, ctx->opts, "broker-opts");
+    if (rank == 0 && cmd_argz)
+        argz_append (&argz, &argz_len, cmd_argz, cmd_argz_len); /* must be last arg */
+
+    subprocess_set_args_from_argz (cli->p, argz, argz_len);
+    free (argz);
 
     subprocess_set_environ (cli->p, environ);
 
@@ -509,7 +502,7 @@ int client_run (struct client *cli)
     return subprocess_run (cli->p);
 }
 
-int start_session (struct context *ctx, const char *cmd)
+int start_session (struct context *ctx, const char *cmd_argz, size_t cmd_argz_len)
 {
     struct client *cli;
     int rank;
@@ -534,7 +527,7 @@ int start_session (struct context *ctx, const char *cmd)
     pmi_server_initialize (ctx, flags);
 
     for (rank = 0; rank < ctx->size; rank++) {
-        if (!(cli = client_create (ctx, rank, cmd)))
+        if (!(cli = client_create (ctx, rank, cmd_argz, cmd_argz_len)))
             log_err_exit ("client_create");
         if (optparse_hasopt (ctx->opts, "verbose"))
             client_dumpargs (cli);

src/common/libsubprocess/subprocess.c

@@ -431,6 +431,18 @@ int subprocess_set_args (struct subprocess *p, int argc, char **argv)
     return (init_argz (&p->argz, &p->argz_len, argv));
 }
 
+int subprocess_set_args_from_argz (struct subprocess *p, const char * argz, size_t argz_len)
+{
+    if (p->started || argz == NULL) {
+        errno = EINVAL;
+        return (-1);
+    }
+    free (p->argz);
+    p->argz_len = 0;
+    int e = argz_append (&p->argz, &p->argz_len, argz, argz_len);
+    return e ? (errno = e, -1) : 0;
+}
+
 const char * subprocess_get_arg (struct subprocess *p, int n)
 {
     int i;
@@ -488,6 +500,22 @@ int subprocess_argv_append (struct subprocess *p, const char *s)
     return (0);
 }
 
+int subprocess_argv_append_argz (struct subprocess *p, const char *argz, size_t argz_len)
+{
+    int e;
+
+    if (p->started) {
+        errno = EINVAL;
+        return (-1);
+    }
+
+    if ((e = argz_append (&p->argz, &p->argz_len, argz, argz_len)) != 0) {
+        errno = e;
+        return -1;
+    }
+    return (0);
+}
+
 int subprocess_set_command (struct subprocess *p, const char *cmd)
 {
     int e;

src/common/libsubprocess/subprocess.h

@@ -22,6 +22,9 @@
  *  See also:  http://www.gnu.org/licenses/
 \*****************************************************************************/
 
+#ifndef _SUBPROCESS_H
+#define _SUBPROCESS_H
+
 #include <stdbool.h>
 
 struct subprocess_manager;
@@ -141,6 +144,14 @@ void *subprocess_get_context (struct subprocess *p, const char *name);
  */
 int subprocess_set_args (struct subprocess *p, int argc, char *argv[]);
 
+/*
+ *  Set argument vector for subprocess [p] from argz vector (argz, argz_len).
+ *   This function is only valid before subprocess_run() is called. Any existing
+ *   args associated with subprocess are discarded.
+ *   Returns -1 with errno set to EINVAL if subprocess has already started.
+ */
+int subprocess_set_args_from_argz (struct subprocess *p, const char * argz, size_t argz_len);
+
 /*
  *  Identical subprocess_set_args(), subprocess_set_command() is a
  *   convenience function similar to system(3). That is, it will set
@@ -159,6 +170,13 @@ int subprocess_set_command (struct subprocess *p, const char *command);
  */
 int subprocess_argv_append (struct subprocess *p, const char *arg);
 
+/*
+ *  Append a single argument to the subprocess [p] argument vector.
+ *   Returns -1 with errno set to EINVAL if subprocess has already started.
+ */
+int subprocess_argv_append_argz (struct subprocess *p, const char *argz, size_t argz_len);
+
+
 /*
  *  Get argument at index [n] from current argv array for process [p]
  *  Returns NULL if n > argc - 1.
@@ -327,3 +345,5 @@ int subprocess_io_complete (struct subprocess *p);
  *   be scheduled for stdin one all buffered data is written.
  */
 int subprocess_write (struct subprocess *p, void *buf, size_t count, bool eof);
+
+#endif /* !_SUBPROCESS_H */