Merge pull request #5294 from chu11/issue5233_libsubprocess_human_readable_errors

libsubprocess: support user friendly error string

Author: mergify[bot]

src/cmd/flux-exec.c

@@ -157,12 +157,17 @@ void state_cb (flux_subprocess_t *p, flux_subprocess_state_t state)
     if (state == FLUX_SUBPROCESS_FAILED) {
         flux_cmd_t *cmd = flux_subprocess_get_cmd (p);
         int errnum = flux_subprocess_fail_errno (p);
+        const char *errmsg = flux_subprocess_fail_error (p);
         int ec = 1;
 
+        /* N.B. if no error message available from
+         * flux_subprocess_fail_error(), errmsg is set to strerror of
+         * subprocess errno.
+         */
         log_msg ("Error: rank %d: %s: %s",
                  flux_subprocess_rank (p),
                  flux_cmd_arg (cmd, 0),
-                 strerror (errnum));
+                 errmsg);
 
         /* bash standard, 126 for permission/access denied, 127 for
          * command not found.  68 (EX_NOHOST) for No route to host.

src/common/libsubprocess/fork.c

@@ -248,7 +248,12 @@ static int local_release_child (flux_subprocess_t *p)
 
 static int local_exec (flux_subprocess_t *p)
 {
-    if ((p->exec_failed_errno = local_release_child (p)) != 0) {
+    int ret;
+    /* N.B. We don't set p->failed_errno here, if locally launched via
+     * flux_local_exec(), will return -1 and errno to caller.  If
+     * called via server, p->failed_errno will be set by remote
+     * handler. */
+    if ((ret = local_release_child (p)) != 0) {
         /*
          *  Reap child immediately. Expectation from caller is that
          *   failure to exec will not require subsequent reaping of
@@ -260,7 +265,7 @@ static int local_exec (flux_subprocess_t *p)
             return -1;
         p->status = status;
 
-        errno = p->exec_failed_errno;
+        errno = ret;
         return -1;
     }
     return 0;

src/common/libsubprocess/remote.c

@@ -15,13 +15,15 @@
 #include <sys/types.h>
 #include <wait.h>
 #include <unistd.h>
+#include <stdarg.h>
 #include <errno.h>
 #include <assert.h>
 
 #include <flux/core.h>
 
 #include "ccan/str/str.h"
 #include "src/common/libczmqcontainers/czmq_containers.h"
+#include "src/common/libutil/errprintf.h"
 #include "src/common/libutil/log.h"
 #include "src/common/libutil/fdwalk.h"
 #include "src/common/libutil/macros.h"
@@ -37,6 +39,15 @@
 
 static void remote_kill_nowait (flux_subprocess_t *p, int signum);
 
+static void set_failed (flux_subprocess_t *p, const char *fmt, ...)
+{
+    va_list ap;
+    va_start (ap, fmt);
+    verrprintf (&p->failed_error, fmt, ap);
+    p->failed_errno = errno;
+    va_end (ap);
+}
+
 static void start_channel_watchers (flux_subprocess_t *p)
 {
     struct subprocess_channel *c;
@@ -169,6 +180,7 @@ static int remote_write (struct subprocess_channel *c)
 
     if (!(ptr = flux_buffer_read (c->write_buffer, -1, &lenp))) {
         llog_debug (c->p, "flux_buffer_read: %s", strerror (errno));
+        set_failed (c->p, "internal buffer read error");
         goto error;
     }
 
@@ -192,6 +204,7 @@ static int remote_write (struct subprocess_channel *c)
         llog_debug (c->p,
                     "error sending rexec.write request: %s",
                     strerror (errno));
+        set_failed (c->p, "internal write error");
         goto error;
     }
 
@@ -215,6 +228,7 @@ static int remote_close (struct subprocess_channel *c)
         llog_debug (c->p,
                     "error sending rexec.write request: %s",
                     strerror (errno));
+        set_failed (c->p, "internal close error");
         return -1;
     }
     /* No need to do a "channel_flush", normal io reactor will handle
@@ -254,7 +268,9 @@ static void remote_in_check_cb (flux_reactor_t *r,
     return;
 
 error:
-    c->p->failed_errno = errno;
+    /* c->p->failed_errno and c->p->failed_error expected to be
+     * set before this point (typically via set_failed())
+     */
     process_new_state (c->p, FLUX_SUBPROCESS_FAILED);
     remote_kill_nowait (c->p, SIGKILL);
     flux_future_destroy (c->p->f);
@@ -520,6 +536,7 @@ static int remote_output (flux_subprocess_t *p,
                     (int)flux_subprocess_pid (p),
                     stream);
         errno = EPROTO;
+        set_failed (p, "error buffering unknown channel %s", stream);
         return -1;
     }
 
@@ -539,6 +556,7 @@ static int remote_output (flux_subprocess_t *p,
                         (int)flux_subprocess_pid (p),
                         stream,
                         strerror (errno));
+            set_failed (p, "error buffering %d bytes of data", len);
             return -1;
         }
     }
@@ -572,6 +590,7 @@ static void rexec_continuation (flux_future_t *f, void *arg)
             remote_completion (p);
             return;
         }
+        set_failed (p, "%s", future_strerror (f, errno));
         goto error;
     }
     if (subprocess_rexec_is_started (f, &p->pid)) {
@@ -592,7 +611,9 @@ static void rexec_continuation (flux_future_t *f, void *arg)
     return;
 
 error:
-    p->failed_errno = errno;
+    /* c->p->failed_errno and c->p->failed_error expected to be
+     * set before this point (typically via set_failed())
+     */
     process_new_state (p, FLUX_SUBPROCESS_FAILED);
     remote_kill_nowait (p, SIGKILL);
 }

src/common/libsubprocess/server.c

@@ -19,6 +19,7 @@
 
 #include "src/common/libczmqcontainers/czmq_containers.h"
 #include "src/common/libutil/errno_safe.h"
+#include "src/common/libutil/errprintf.h"
 #include "src/common/libutil/llog.h"
 #include "src/common/libioencode/ioencode.h"
 #include "ccan/str/str.h"
@@ -136,6 +137,7 @@ static void proc_internal_fatal (flux_subprocess_t *p)
      */
     p->state = FLUX_SUBPROCESS_FAILED;
     p->failed_errno = errno;
+    errprintf (&p->failed_error, "internal fatal error: %s", strerror (errno));
     state_change_start (p);
 
     /* if we fail here, probably not much can be done */
@@ -177,7 +179,13 @@ static void proc_state_change_cb (flux_subprocess_t *p,
                                 "type", "stopped");
     }
     else if (state == FLUX_SUBPROCESS_FAILED) {
-        rc = flux_respond_error (s->h, request, p->failed_errno, NULL);
+        const char *errmsg = NULL;
+        if (p->failed_error.text[0] != '\0')
+            errmsg = p->failed_error.text;
+        rc = flux_respond_error (s->h,
+                                 request,
+                                 p->failed_errno,
+                                 errmsg);
         proc_delete (s, p); // N.B. proc_delete preserves errno
     } else {
         errno = EPROTO;
@@ -333,8 +341,11 @@ static void server_exec_cb (flux_t *h, flux_msg_handler_t *mh,
                                   &ops,
                                   NULL,
                                   s->llog,
-                                  s->llog_data)))
+                                  s->llog_data))) {
+        errprintf (&error, "error launching process: %s", strerror (errno));
+        errmsg = error.text;
         goto error;
+    }
 
     if (flux_subprocess_aux_set (p,
                                 msgkey,

src/common/libsubprocess/subprocess.c

@@ -1121,6 +1121,17 @@ int flux_subprocess_fail_errno (flux_subprocess_t *p)
     return p->failed_errno;
 }
 
+const char *flux_subprocess_fail_error (flux_subprocess_t *p)
+{
+    if (!p)
+        return "internal error: subprocess is NULL";
+    if (p->state != FLUX_SUBPROCESS_FAILED)
+        return "internal error: subprocess is not in FAILED state";
+    if (p->failed_error.text[0] == '\0')
+        return strerror (p->failed_errno);
+    return p->failed_error.text;
+}
+
 int flux_subprocess_status (flux_subprocess_t *p)
 {
     if (!p) {

src/common/libsubprocess/subprocess.h

@@ -453,10 +453,16 @@ const char *flux_subprocess_state_string (flux_subprocess_state_t state);
 
 int flux_subprocess_rank (flux_subprocess_t *p);
 
-/* Returns the errno causing the FLUX_SUBPROCESS_FAILED states to be reached.
+/* Returns the errno causing the FLUX_SUBPROCESS_FAILED state to be reached.
  */
 int flux_subprocess_fail_errno (flux_subprocess_t *p);
 
+/* Returns error message describing why FLUX_SUBPROCESS_FAILED state was
+ * reached.  If error message was not set, will return strerror() of
+ * errno returned from flux_subprocess_fail_errno().
+ */
+const char *flux_subprocess_fail_error (flux_subprocess_t *p);
+
 /* Returns exit status as returned from wait(2).  Works only for
  * FLUX_SUBPROCESS_EXITED state. */
 int flux_subprocess_status (flux_subprocess_t *p);

src/common/libsubprocess/subprocess_private.h

@@ -84,7 +84,6 @@ struct flux_subprocess {
     int channels_eof_sent;          /* counter to avoid loop checks */
 
     int status;      /* Raw status from waitpid(2), valid if exited       */
-    int exec_failed_errno;  /* Holds errno from exec(2) if exec() failed  */
 
     flux_subprocess_state_t state;
     flux_subprocess_state_t state_reported; /* for on_state_change */
@@ -111,6 +110,7 @@ struct flux_subprocess {
     flux_future_t *f;           /* primary future reactor */
     bool remote_completed;      /* if remote has completed */
     int failed_errno;           /* Holds errno if FAILED state reached */
+    flux_error_t failed_error;  /* Holds detailed message for failed_errno */
     int signal_pending;         /* signal sent while starting */
 };