libsubprocess: support user friendly error string

Problem: When a subprocess fails the "failed_errno" value is
set and can be returned in flux_subprocess_fail_errno().  It would
be convenient if there was an error message to describe the
error in more detail.

Solution: Internally support a new "failed_error" field which can store
a text message describing the error in more detail.  Generally speaking,
always fill this with a text message when "failed_errno" is also set.

Set user friendly error string in many cases throughout libsubprocess.

Support a new flux_subprocess_fail_error() to retrieve this error message.

Add corner case unit tests.

Fixes #5233

Author: chu11

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

@@ -457,6 +457,12 @@ int flux_subprocess_rank (flux_subprocess_t *p);
  */
 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

@@ -110,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 */
 };
 

src/common/libsubprocess/test/subprocess.c

@@ -173,6 +173,8 @@ void test_corner_cases (flux_reactor_t *r)
     ok (flux_subprocess_fail_errno (NULL) < 0
         && errno == EINVAL,
         "flux_subprocess_fail_errno fails with NULL pointer input");
+    ok (flux_subprocess_fail_error (NULL) != NULL,
+        "flux_subprocess_fail_error works with NULL pointer input");
     ok (flux_subprocess_status (NULL) < 0
         && errno == EINVAL,
         "flux_subprocess_status fails with NULL pointer input");
@@ -258,6 +260,8 @@ void test_post_exec_errors (flux_reactor_t *r)
         "flux_subprocess_rank fails b/c subprocess is local");
     ok (flux_subprocess_fail_errno (p) < 0,
         "subprocess fail errno fails b/c subprocess not failed");
+    ok (flux_subprocess_fail_error (p) != NULL,
+        "subprocess fail error works when subprocess not failed");
     ok (flux_subprocess_status (p) < 0,
         "subprocess status fails b/c subprocess not yet exited");
     ok (flux_subprocess_exit_code (p) < 0,