Libguestfs - Mar 2023 - [libnbd PATCH v3 16/19] socket activation: generalize environment construction

V3 was here:
<http://mid.mail-archive.com/20230215141158.2426855-1-lersek at
redhat.com>.

See the Notes section on each patch for the v4 updates.

The series is nearly ready for merging: every patch has at least one R-b
tag, except "socket activation: avoid manipulating the sign bit".

The series builds, and passes "make check" and "make
check-valgrind", at
every stage.

Thanks for reviewing!
Laszlo

Laszlo Ersek (17):
  socket activation: fix error message upon asprintf() failure
  socket activation: clean up responsibilities of prep.sock.act.env.()
  socket activation: avoid manipulating the sign bit
  socket activation: check syscalls for errors in the child process
  socket activation: centralize resource release
  socket activation: plug AF_UNIX socket address (filesystem) leak on
    error
  socket activation: replace execvp() call with fork-safe variant
  CONNECT_COMMAND.START: fix small comment thinko about socket pair
    usage
  CONNECT_COMMAND.START: set the NBD error when fcntl() fails
  CONNECT_COMMAND.START: use symbolic constants for fd#0 and fd#1
  CONNECT_COMMAND.START: sanitize close() calls in the child process
  CONNECT_COMMAND.START: check syscalls for errors in the child process
  CONNECT_COMMAND.START: centralize resource release
  CONNECT_COMMAND.START: plug child process leak on error
  CONNECT_COMMAND.START: replace execvp() call with fork-safe variant
  socket activation: generalize environment construction
  socket activation: set LISTEN_FDNAMES

Richard W.M. Jones (2):
  common/include: Copy ascii-ctype functions from nbdkit
  generator: Add APIs to get/set the socket activation socket name

 lib/internal.h                               |   1 +
 common/include/ascii-ctype.h                 |  75 +++++
 generator/API.ml                             |  49 ++++
 generator/states-connect-socket-activation.c | 287 ++++++++++++++------
 generator/states-connect.c                   | 123 ++++++---
 lib/handle.c                                 |  56 ++++
 common/include/Makefile.am                   |   6 +
 common/include/test-ascii-ctype.c            |  88 ++++++
 .gitignore                                   |   1 +
 9 files changed, 570 insertions(+), 116 deletions(-)
 create mode 100644 common/include/ascii-ctype.h
 create mode 100644 common/include/test-ascii-ctype.c


base-commit: 9075f68ffc8bed320d0d1d46f1f0456d10626878

Correctly report "asprintf" in the error message when asprintf()
fails.

Signed-off-by: Laszlo Ersek <lersek at redhat.com>
Reviewed-by: Richard W.M. Jones <rjones at redhat.com>
Reviewed-by: Eric Blake <eblake at redhat.com>
---

Notes:
    v4:
    
    - pick up R-b's from Rich and Eric

 generator/states-connect-socket-activation.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/generator/states-connect-socket-activation.c
b/generator/states-connect-socket-activation.c
index e4b3b565ae2e..ddbccff8240d 100644
--- a/generator/states-connect-socket-activation.c
+++ b/generator/states-connect-socket-activation.c
@@ -127,7 +127,7 @@  CONNECT_SA.START:
 
   if (asprintf (&h->sact_sockpath, "%s/sock",
h->sact_tmpdir) == -1) {
     SET_NEXT_STATE (%.DEAD);
-    set_error (errno, "strdup");
+    set_error (errno, "asprintf");
     return 0;
   }

prepare_socket_activation_environment() is a construction function that is
supposed to fill in a string_vector object from the ground up. Right now
it has its responsibilities mixed up in two ways:

- it expects the caller to pass in a previously re-set string_vector,

- if it fails, it calls set_error() internally (with a blanket reference
  to "malloc").

Fix the first wart:

- pass in an *uninitialized* (only allocated) string vector from the
  caller, and initialize it in prepare_socket_activation_environment().

Document the second wart:

- in the caller, add a comment about the set_error() call that's
  internal to the callee.

Signed-off-by: Laszlo Ersek <lersek at redhat.com>
Reviewed-by: Eric Blake <eblake at redhat.com>
Acked-by: Richard W.M. Jones <rjones at redhat.com>
---

Notes:
    v4:
    
    - Pick up Eric's R-b.
    
    - Pick up Rich's A-b for initializing the string vector (via struct
      assignment) internally to prepare_socket_activation_environment().
    
    - Keep the set_error() call internal to
      prepare_socket_activation_environment(), just add a comment about it
      near the prepare_socket_activation_environment() call site [Rich].
      (Unfortunately, this creates a long series of rebase conflicts.)
    
    - Update the commit message.

 generator/states-connect-socket-activation.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/generator/states-connect-socket-activation.c
b/generator/states-connect-socket-activation.c
index ddbccff8240d..61d3d1900f45 100644
--- a/generator/states-connect-socket-activation.c
+++ b/generator/states-connect-socket-activation.c
@@ -51,7 +51,7 @@ prepare_socket_activation_environment (string_vector *env)
   char *p;
   size_t i;
 
-  assert (env->len == 0);
+  *env = (string_vector)empty_vector;
 
   /* Reserve slots env[0] and env[1]. */
   p = strdup ("LISTEN_PID=xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx");
@@ -99,7 +99,7 @@ STATE_MACHINE {
  CONNECT_SA.START:
   int s;
   struct sockaddr_un addr;
-  string_vector env = empty_vector;
+  string_vector env;
   pid_t pid;
 
   assert (!h->sock);
@@ -156,6 +156,7 @@  CONNECT_SA.START:
 
   if (prepare_socket_activation_environment (&env) == -1) {
     SET_NEXT_STATE (%.DEAD);
+    /* prepare_socket_activation_environment() calls set_error() internally */
     close (s);
     return 0;
   }

F_SETFD takes an "int", so it stands to reason that FD_CLOEXEC expands
to
an "int". In turn, it's bad hygiene to manipulate the sign bit of
(signed)
integers with bit operations:

  ~FD_CLOEXEC

Convert FD_CLOEXEC to "unsigned int" for the bitwise complement
operator:

  ~(unsigned)FD_CLOEXEC

The bitwise complement then results in an "unsigned int".
"Flags" (of type
"int", and having, per POSIX, a non-negative value returned by
fcntl(F_GETFD)) will be automatically converted to "unsigned int" by
the
usual arithmetic conversions for the bitwise AND operator:

  flags & ~(unsigned)FD_CLOEXEC

We could pass the result of *that* to fcntl(), due to (a) the value being
in range for "signed int" ("flags" is a non-negative
"int", and we're only
clearing a value bit), and (b) non-negative "int" values being
represented
identically by "unsigned int" (C99 6.2.5 p9). But, for clarity,
let's cast
the result to "int" explicitly:

  (int)(flags & ~(unsigned)FD_CLOEXEC)

Signed-off-by: Laszlo Ersek <lersek at redhat.com>
---

Notes:
    context:-U5

 generator/states-connect-socket-activation.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/generator/states-connect-socket-activation.c
b/generator/states-connect-socket-activation.c
index 61d3d1900f45..b9845c11a221 100644
--- a/generator/states-connect-socket-activation.c
+++ b/generator/states-connect-socket-activation.c
@@ -182,11 +182,11 @@  CONNECT_SA.START:
       int flags = fcntl (s, F_GETFD, 0);
       if (flags == -1) {
         nbd_internal_fork_safe_perror ("fcntl: F_GETFD");
         _exit (126);
       }
-      if (fcntl (s, F_SETFD, flags & ~FD_CLOEXEC) == -1) {
+      if (fcntl (s, F_SETFD, (int)(flags & ~(unsigned)FD_CLOEXEC)) == -1) {
         nbd_internal_fork_safe_perror ("fcntl: F_SETFD");
         _exit (126);
       }
     }

It's bad hygiene to just assume dup2(), close() and signal() will succeed,
especially considering the inconsistency that we do check fcntl(). Check
all return values.

Signed-off-by: Laszlo Ersek <lersek at redhat.com>
Reviewed-by: Richard W.M. Jones <rjones at redhat.com>
---

Notes:
    v4:
    
    - pick up Rich's R-b
    
    context:-U12

 generator/states-connect-socket-activation.c | 15 ++++++++++++---
 1 file changed, 12 insertions(+), 3 deletions(-)

diff --git a/generator/states-connect-socket-activation.c
b/generator/states-connect-socket-activation.c
index b9845c11a221..e3f7d05670b0 100644
--- a/generator/states-connect-socket-activation.c
+++ b/generator/states-connect-socket-activation.c
@@ -162,50 +162,59 @@  CONNECT_SA.START:
   }
 
   pid = fork ();
   if (pid == -1) {
     SET_NEXT_STATE (%.DEAD);
     set_error (errno, "fork");
     close (s);
     string_vector_empty (&env);
     return 0;
   }
   if (pid == 0) {         /* child - run command */
     if (s != FIRST_SOCKET_ACTIVATION_FD) {
-      dup2 (s, FIRST_SOCKET_ACTIVATION_FD);
-      close (s);
+      if (dup2 (s, FIRST_SOCKET_ACTIVATION_FD) == -1) {
+        nbd_internal_fork_safe_perror ("dup2");
+        _exit (126);
+      }
+      if (close (s) == -1) {
+        nbd_internal_fork_safe_perror ("close");
+        _exit (126);
+      }
     }
     else {
       /* We must unset CLOEXEC on the fd.  (dup2 above does this
        * implicitly because CLOEXEC is set on the fd, not on the
        * socket).
        */
       int flags = fcntl (s, F_GETFD, 0);
       if (flags == -1) {
         nbd_internal_fork_safe_perror ("fcntl: F_GETFD");
         _exit (126);
       }
       if (fcntl (s, F_SETFD, (int)(flags & ~(unsigned)FD_CLOEXEC)) == -1) {
         nbd_internal_fork_safe_perror ("fcntl: F_SETFD");
         _exit (126);
       }
     }
 
     char buf[32];
     const char *v        nbd_internal_fork_safe_itoa ((long) getpid (), buf,
sizeof buf);
     strcpy (&env.ptr[0][PREFIX_LENGTH], v);
 
     /* Restore SIGPIPE back to SIG_DFL. */
-    signal (SIGPIPE, SIG_DFL);
+    if (signal (SIGPIPE, SIG_DFL) == SIG_ERR) {
+      nbd_internal_fork_safe_perror ("signal");
+      _exit (126);
+    }
 
     environ = env.ptr;
     execvp (h->argv.ptr[0], h->argv.ptr);
     nbd_internal_fork_safe_perror (h->argv.ptr[0]);
     if (errno == ENOENT)
       _exit (127);
     else
       _exit (126);
   }
 
   /* Parent. */
   close (s);

The CONNECT_SA.START handler constructs several resources; some of those
are needed permanently if the entire handler succeeds, while others are
needed only temporarily, for constructing the permanent objects.
Currently, the various error branches in the handler deal with resource
release individually, leading to code duplication and dispersed
responsibilities; if we want to construct a new resource, then, under the
existent pattern, we'll have to modify multiple error branches to release
it as well.

In my opinion, the best pattern for dealing with this scenario is the
following structure:

- Place cascading error handling labels at the end of the function -- an
  error handling slide in effect. The order of destruction on this slide
  is the inverse of construction.

- Whenever an operation fails mid-construction, enter the slide such that
  precisely the thus far constructed objects be freed. Note that the goto
  statements and the destination labels nest properly.

- The very last construction step needs no rollback, because the last
  successful step completes the entire function / handler. When this
  happens, *commit* by (a) outputting the permanent resources, (b) setting
  a top-level "final result" variable to "success" (aka
"ownership of
  permanent resources transferred to the caller"), and (c) falling through
  to the slide. On the slide, use the "final result" variable to skip
the
  release of those resources that have been output as permanent ones.

This way, each resource is freed in exactly one location, and whenever an
error occurs, no resource is even *checked* for rollback if its
construction could not be tried, or completed, in the first place. The
introduction of a new resource needs one properly placed construction step
and one matchingly placed error handling label.

Rich notes that "state machine labels match the basic regexp pattern:

^ \\([A-Z0-9][A-Z0-9_\\.]*\\):$

(see generator/state_machine_generator.ml), and because of the
all-capitalization of state names, they won't match the lower case
ordinary C labels we're using here".

The restructuring highlights the following leak: whenever any construction
step after bind() fails, the UNIX domain socket address (i.e., the
pathname in the filesystem) is leaked. (The filesystem object may well be
removed once the application decides to call nbd_close(), but until then,
after the CONNECT_SA.START handler returns with failure, unusable
(half-constructed) resources linger around indefinitely. A library API
should either fully succeed or fully fail.) We'll plug the leak later in
this series.

Signed-off-by: Laszlo Ersek <lersek at redhat.com>
Reviewed-by: Richard W.M. Jones <rjones at redhat.com>
---

Notes:
    v4:
    
    - pick up Rich's R-b
    
    - remark in the commit message that the generated state machine labels,
      and the manual labels introduced by the patch, belong to distinct
      "namespaces" [Rich]
    
    - cumbersome rebase conflict resolution near the
      prepare_socket_activation_environment() call site, due to keeping
      set_error() internal to that callee, in patch "socket activation:
      clean up responsibilities of prep.sock.act.env.()"
    
    context:-W

 generator/states-connect-socket-activation.c | 85 +++++++++++---------
 1 file changed, 49 insertions(+), 36 deletions(-)

diff --git a/generator/states-connect-socket-activation.c
b/generator/states-connect-socket-activation.c
index e3f7d05670b0..b20c3f1f10bb 100644
--- a/generator/states-connect-socket-activation.c
+++ b/generator/states-connect-socket-activation.c
@@ -97,132 +97,145 @@ prepare_socket_activation_environment (string_vector *env)
 
 STATE_MACHINE {
  CONNECT_SA.START:
+  enum state next;
+  char *tmpdir;
+  char *sockpath;
   int s;
   struct sockaddr_un addr;
   string_vector env;
   pid_t pid;
 
   assert (!h->sock);
   assert (h->argv.ptr);
   assert (h->argv.ptr[0]);
 
+  next = %.DEAD;
+
   /* Use /tmp instead of TMPDIR because we must ensure the path is
    * short enough to store in the sockaddr_un.  On some platforms this
    * may cause problems so we may need to revisit it.  XXX
    */
-  h->sact_tmpdir = strdup ("/tmp/libnbdXXXXXX");
-  if (h->sact_tmpdir == NULL) {
-    SET_NEXT_STATE (%.DEAD);
+  tmpdir = strdup ("/tmp/libnbdXXXXXX");
+  if (tmpdir == NULL) {
     set_error (errno, "strdup");
-    return 0;
+    goto done;
   }
-  if (mkdtemp (h->sact_tmpdir) == NULL) {
-    SET_NEXT_STATE (%.DEAD);
+
+  if (mkdtemp (tmpdir) == NULL) {
     set_error (errno, "mkdtemp");
-    /* Avoid cleanup in nbd_close. */
-    free (h->sact_tmpdir);
-    h->sact_tmpdir = NULL;
-    return 0;
+    goto free_tmpdir;
   }
 
-  if (asprintf (&h->sact_sockpath, "%s/sock",
h->sact_tmpdir) == -1) {
-    SET_NEXT_STATE (%.DEAD);
+  if (asprintf (&sockpath, "%s/sock", tmpdir) == -1) {
     set_error (errno, "asprintf");
-    return 0;
+    goto rmdir_tmpdir;
   }
 
   s = nbd_internal_socket (AF_UNIX, SOCK_STREAM, 0, false);
   if (s == -1) {
-    SET_NEXT_STATE (%.DEAD);
     set_error (errno, "socket");
-    return 0;
+    goto free_sockpath;
   }
 
   addr.sun_family = AF_UNIX;
-  memcpy (addr.sun_path, h->sact_sockpath, strlen (h->sact_sockpath) +
1);
+  memcpy (addr.sun_path, sockpath, strlen (sockpath) + 1);
   if (bind (s, (struct sockaddr *) &addr, sizeof addr) == -1) {
-    SET_NEXT_STATE (%.DEAD);
-    set_error (errno, "bind: %s", h->sact_sockpath);
-    close (s);
-    return 0;
+    set_error (errno, "bind: %s", sockpath);
+    goto close_socket;
   }
 
   if (listen (s, SOMAXCONN) == -1) {
-    SET_NEXT_STATE (%.DEAD);
     set_error (errno, "listen");
-    close (s);
-    return 0;
+    goto close_socket;
   }
 
-  if (prepare_socket_activation_environment (&env) == -1) {
-    SET_NEXT_STATE (%.DEAD);
+  if (prepare_socket_activation_environment (&env) == -1)
     /* prepare_socket_activation_environment() calls set_error() internally */
-    close (s);
-    return 0;
-  }
+    goto close_socket;
 
   pid = fork ();
   if (pid == -1) {
-    SET_NEXT_STATE (%.DEAD);
     set_error (errno, "fork");
-    close (s);
-    string_vector_empty (&env);
-    return 0;
+    goto empty_env;
   }
+
   if (pid == 0) {         /* child - run command */
     if (s != FIRST_SOCKET_ACTIVATION_FD) {
       if (dup2 (s, FIRST_SOCKET_ACTIVATION_FD) == -1) {
         nbd_internal_fork_safe_perror ("dup2");
         _exit (126);
       }
       if (close (s) == -1) {
         nbd_internal_fork_safe_perror ("close");
         _exit (126);
       }
     }
     else {
       /* We must unset CLOEXEC on the fd.  (dup2 above does this
        * implicitly because CLOEXEC is set on the fd, not on the
        * socket).
        */
       int flags = fcntl (s, F_GETFD, 0);
       if (flags == -1) {
         nbd_internal_fork_safe_perror ("fcntl: F_GETFD");
         _exit (126);
       }
       if (fcntl (s, F_SETFD, (int)(flags & ~(unsigned)FD_CLOEXEC)) == -1) {
         nbd_internal_fork_safe_perror ("fcntl: F_SETFD");
         _exit (126);
       }
     }
 
     char buf[32];
     const char *v        nbd_internal_fork_safe_itoa ((long) getpid (), buf,
sizeof buf);
     strcpy (&env.ptr[0][PREFIX_LENGTH], v);
 
     /* Restore SIGPIPE back to SIG_DFL. */
     if (signal (SIGPIPE, SIG_DFL) == SIG_ERR) {
       nbd_internal_fork_safe_perror ("signal");
       _exit (126);
     }
 
     environ = env.ptr;
     execvp (h->argv.ptr[0], h->argv.ptr);
     nbd_internal_fork_safe_perror (h->argv.ptr[0]);
     if (errno == ENOENT)
       _exit (127);
     else
       _exit (126);
   }
 
-  /* Parent. */
-  close (s);
-  string_vector_empty (&env);
+  /* Parent -- we're done; commit. */
+  h->sact_tmpdir = tmpdir;
+  h->sact_sockpath = sockpath;
   h->pid = pid;
 
   h->connaddrlen = sizeof addr;
   memcpy (&h->connaddr, &addr, h->connaddrlen);
-  SET_NEXT_STATE (%^CONNECT.START);
+  next = %^CONNECT.START;
+
+  /* fall through, for releasing the temporaries */
+
+empty_env:
+  string_vector_empty (&env);
+
+close_socket:
+  close (s);
+
+free_sockpath:
+  if (next == %.DEAD)
+    free (sockpath);
+
+rmdir_tmpdir:
+  if (next == %.DEAD)
+    rmdir (tmpdir);
+
+free_tmpdir:
+  if (next == %.DEAD)
+    free (tmpdir);
+
+done:
+  SET_NEXT_STATE (next);
   return 0;
 } /* END STATE MACHINE */

A prior patch highlighted the following leak: whenever any construction
step after bind() fails, the UNIX domain socket address (i.e., the
pathname in the filesystem) is leaked. Plug the leak by inserting a new
error handling section that unlinks the pathname.

Signed-off-by: Laszlo Ersek <lersek at redhat.com>
Reviewed-by: Richard W.M. Jones <rjones at redhat.com>
---

Notes:
    v4:
    
    - pick up Rich's R-b
    
    - resolve rebase conflict near the
      prepare_socket_activation_environment() call site, due to keeping
      set_error() internal to that callee, in patch "socket activation:
      clean up responsibilities of prep.sock.act.env.()"
    
    context:-U12

 generator/states-connect-socket-activation.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/generator/states-connect-socket-activation.c
b/generator/states-connect-socket-activation.c
index b20c3f1f10bb..752ee73bb62f 100644
--- a/generator/states-connect-socket-activation.c
+++ b/generator/states-connect-socket-activation.c
@@ -137,30 +137,30 @@  CONNECT_SA.START:
     goto free_sockpath;
   }
 
   addr.sun_family = AF_UNIX;
   memcpy (addr.sun_path, sockpath, strlen (sockpath) + 1);
   if (bind (s, (struct sockaddr *) &addr, sizeof addr) == -1) {
     set_error (errno, "bind: %s", sockpath);
     goto close_socket;
   }
 
   if (listen (s, SOMAXCONN) == -1) {
     set_error (errno, "listen");
-    goto close_socket;
+    goto unlink_sockpath;
   }
 
   if (prepare_socket_activation_environment (&env) == -1)
     /* prepare_socket_activation_environment() calls set_error() internally */
-    goto close_socket;
+    goto unlink_sockpath;
 
   pid = fork ();
   if (pid == -1) {
     set_error (errno, "fork");
     goto empty_env;
   }
 
   if (pid == 0) {         /* child - run command */
     if (s != FIRST_SOCKET_ACTIVATION_FD) {
       if (dup2 (s, FIRST_SOCKET_ACTIVATION_FD) == -1) {
         nbd_internal_fork_safe_perror ("dup2");
         _exit (126);
@@ -211,24 +211,28 @@  CONNECT_SA.START:
   h->sact_sockpath = sockpath;
   h->pid = pid;
 
   h->connaddrlen = sizeof addr;
   memcpy (&h->connaddr, &addr, h->connaddrlen);
   next = %^CONNECT.START;
 
   /* fall through, for releasing the temporaries */
 
 empty_env:
   string_vector_empty (&env);
 
+unlink_sockpath:
+  if (next == %.DEAD)
+    unlink (sockpath);
+
 close_socket:
   close (s);
 
 free_sockpath:
   if (next == %.DEAD)
     free (sockpath);
 
 rmdir_tmpdir:
   if (next == %.DEAD)
     rmdir (tmpdir);
 
 free_tmpdir:

Per POSIX, execvp() is not safe to call in a child process forked from a
multi-threaded process. We can now replace the execvp() call in the child
process with a call to our fork-safe (async-signal-safe) variant.

Prepare our internal execvpe context on the parent's construction path,
use the context in the child, and release the context in the parent on the
way out, regardless of whether the handler as a whole succeeded or not.
(The context is only a temporary resource.)

Signed-off-by: Laszlo Ersek <lersek at redhat.com>
Reviewed-by: Richard W.M. Jones <rjones at redhat.com>
---

Notes:
    v4:
    
    - pick up Rich's R-b
    
    - resolve rebase conflict near the
      prepare_socket_activation_environment() call site, due to keeping
      set_error() internal to that callee, in patch "socket activation:
      clean up responsibilities of prep.sock.act.env.()"
    
    context:-U11

 generator/states-connect-socket-activation.c | 15 ++++++++++++---
 1 file changed, 12 insertions(+), 3 deletions(-)

diff --git a/generator/states-connect-socket-activation.c
b/generator/states-connect-socket-activation.c
index 752ee73bb62f..a214ffd5a6e4 100644
--- a/generator/states-connect-socket-activation.c
+++ b/generator/states-connect-socket-activation.c
@@ -94,22 +94,23 @@ prepare_socket_activation_environment (string_vector *env)
   string_vector_empty (env);
   return -1;
 }
 
 STATE_MACHINE {
  CONNECT_SA.START:
   enum state next;
   char *tmpdir;
   char *sockpath;
   int s;
   struct sockaddr_un addr;
+  struct execvpe execvpe_ctx;
   string_vector env;
   pid_t pid;
 
   assert (!h->sock);
   assert (h->argv.ptr);
   assert (h->argv.ptr[0]);
 
   next = %.DEAD;
 
   /* Use /tmp instead of TMPDIR because we must ensure the path is
    * short enough to store in the sockaddr_un.  On some platforms this
@@ -141,25 +142,31 @@  CONNECT_SA.START:
   memcpy (addr.sun_path, sockpath, strlen (sockpath) + 1);
   if (bind (s, (struct sockaddr *) &addr, sizeof addr) == -1) {
     set_error (errno, "bind: %s", sockpath);
     goto close_socket;
   }
 
   if (listen (s, SOMAXCONN) == -1) {
     set_error (errno, "listen");
     goto unlink_sockpath;
   }
 
+  if (nbd_internal_execvpe_init (&execvpe_ctx, h->argv.ptr[0],
h->argv.len) =+      -1) {
+    set_error (errno, "nbd_internal_execvpe_init");
+    goto unlink_sockpath;
+  }
+
   if (prepare_socket_activation_environment (&env) == -1)
     /* prepare_socket_activation_environment() calls set_error() internally */
-    goto unlink_sockpath;
+    goto uninit_execvpe;
 
   pid = fork ();
   if (pid == -1) {
     set_error (errno, "fork");
     goto empty_env;
   }
 
   if (pid == 0) {         /* child - run command */
     if (s != FIRST_SOCKET_ACTIVATION_FD) {
       if (dup2 (s, FIRST_SOCKET_ACTIVATION_FD) == -1) {
         nbd_internal_fork_safe_perror ("dup2");
@@ -189,45 +196,47 @@  CONNECT_SA.START:
     char buf[32];
     const char *v        nbd_internal_fork_safe_itoa ((long) getpid (), buf,
sizeof buf);
     strcpy (&env.ptr[0][PREFIX_LENGTH], v);
 
     /* Restore SIGPIPE back to SIG_DFL. */
     if (signal (SIGPIPE, SIG_DFL) == SIG_ERR) {
       nbd_internal_fork_safe_perror ("signal");
       _exit (126);
     }
 
-    environ = env.ptr;
-    execvp (h->argv.ptr[0], h->argv.ptr);
+    (void)nbd_internal_fork_safe_execvpe (&execvpe_ctx, &h->argv,
env.ptr);
     nbd_internal_fork_safe_perror (h->argv.ptr[0]);
     if (errno == ENOENT)
       _exit (127);
     else
       _exit (126);
   }
 
   /* Parent -- we're done; commit. */
   h->sact_tmpdir = tmpdir;
   h->sact_sockpath = sockpath;
   h->pid = pid;
 
   h->connaddrlen = sizeof addr;
   memcpy (&h->connaddr, &addr, h->connaddrlen);
   next = %^CONNECT.START;
 
   /* fall through, for releasing the temporaries */
 
 empty_env:
   string_vector_empty (&env);
 
+uninit_execvpe:
+  nbd_internal_execvpe_uninit (&execvpe_ctx);
+
 unlink_sockpath:
   if (next == %.DEAD)
     unlink (sockpath);
 
 close_socket:
   close (s);
 
 free_sockpath:
   if (next == %.DEAD)
     free (sockpath);

A comment currently states that "*the* socket must be set to non-blocking
only in the parent" (emphasis mine). This is a thinko: it implies that we
have just one file descriptor, and that the underlying file description
(which is manipulated by fcntl()'s F_GETFL and F_SETFL) is shared by the
parent and the child processes. That's not the case: we have *two* sockets
here (a socket pair), one of which is used by the parent exclusively, and
the other is used by the child exclusively.

Clarify the comment: say that we set the parent-side end of the socket
pair to non-blocking.

This clarification will play a role in a subsequent patch.

Signed-off-by: Laszlo Ersek <lersek at redhat.com>
Reviewed-by: Richard W.M. Jones <rjones at redhat.com>
---

Notes:
    v4:
    
    - pick up Rich's R-b
    
    context:-U5

 generator/states-connect.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/generator/states-connect.c b/generator/states-connect.c
index 503f3ebe8819..137350cc77e6 100644
--- a/generator/states-connect.c
+++ b/generator/states-connect.c
@@ -281,11 +281,11 @@  CONNECT_COMMAND.START:
   /* Parent. */
   close (sv[1]);
 
   h->pid = pid;
 
-  /* The socket must be set to non-blocking only in the parent,
+  /* Only the parent-side end of the socket pair must be set to non-blocking,
    * because the child may not be expecting a non-blocking socket.
    */
   flags = fcntl (sv[0], F_GETFL, 0);
   if (flags == -1 ||
       fcntl (sv[0], F_SETFL, flags|O_NONBLOCK) == -1) {

Furthermore, explain in a comment that *not* calling set_error() upon
nbd_internal_socket_create() failing is intentional, and not an omission.

Signed-off-by: Laszlo Ersek <lersek at redhat.com>
Reviewed-by: Richard W.M. Jones <rjones at redhat.com>
---

Notes:
    v4:
    
    - pick up Rich's R-b
    
    context:-W

 generator/states-connect.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/generator/states-connect.c b/generator/states-connect.c
index 137350cc77e6..2fd7429ae056 100644
--- a/generator/states-connect.c
+++ b/generator/states-connect.c
@@ -240,71 +240,73 @@  CONNECT_TCP.NEXT_ADDRESS:
  CONNECT_COMMAND.START:
   int sv[2];
   pid_t pid;
   int flags;
 
   assert (!h->sock);
   assert (h->argv.ptr);
   assert (h->argv.ptr[0]);
   if (nbd_internal_socketpair (AF_UNIX, SOCK_STREAM, 0, sv) == -1) {
     SET_NEXT_STATE (%.DEAD);
     set_error (errno, "socketpair");
     return 0;
   }
 
   pid = fork ();
   if (pid == -1) {
     SET_NEXT_STATE (%.DEAD);
     set_error (errno, "fork");
     close (sv[0]);
     close (sv[1]);
     return 0;
   }
   if (pid == 0) {         /* child - run command */
     close (0);
     close (1);
     close (sv[0]);
     dup2 (sv[1], 0);
     dup2 (sv[1], 1);
     close (sv[1]);
 
     /* Restore SIGPIPE back to SIG_DFL. */
     signal (SIGPIPE, SIG_DFL);
 
     execvp (h->argv.ptr[0], h->argv.ptr);
     nbd_internal_fork_safe_perror (h->argv.ptr[0]);
     if (errno == ENOENT)
       _exit (127);
     else
       _exit (126);
   }
 
   /* Parent. */
   close (sv[1]);
 
   h->pid = pid;
 
   /* Only the parent-side end of the socket pair must be set to non-blocking,
    * because the child may not be expecting a non-blocking socket.
    */
   flags = fcntl (sv[0], F_GETFL, 0);
   if (flags == -1 ||
       fcntl (sv[0], F_SETFL, flags|O_NONBLOCK) == -1) {
     SET_NEXT_STATE (%.DEAD);
+    set_error (errno, "fcntl");
     close (sv[0]);
     return 0;
   }
 
   h->sock = nbd_internal_socket_create (sv[0]);
   if (!h->sock) {
     SET_NEXT_STATE (%.DEAD);
+    /* nbd_internal_socket_create() calls set_error() internally */
     close (sv[0]);
     return 0;
   }
 
   /* The sockets are connected already, we can jump directly to
    * receiving the server magic.
    */
   SET_NEXT_STATE (%^MAGIC.START);
   return 0;
 
 } /* END STATE MACHINE */

Refer to fd#0 and fd#1 with the symbolic constants STDIN_FILENO and
STDOUT_FILENO from <unistd.h>, for better readability.

Signed-off-by: Laszlo Ersek <lersek at redhat.com>
Reviewed-by: Richard W.M. Jones <rjones at redhat.com>
---

Notes:
    v4:
    
    - pick up Rich's R-b

 generator/states-connect.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/generator/states-connect.c b/generator/states-connect.c
index 2fd7429ae056..5612da53f59f 100644
--- a/generator/states-connect.c
+++ b/generator/states-connect.c
@@ -260,11 +260,11 @@  CONNECT_COMMAND.START:
     return 0;
   }
   if (pid == 0) {         /* child - run command */
-    close (0);
-    close (1);
+    close (STDIN_FILENO);
+    close (STDOUT_FILENO);
     close (sv[0]);
-    dup2 (sv[1], 0);
-    dup2 (sv[1], 1);
+    dup2 (sv[1], STDIN_FILENO);
+    dup2 (sv[1], STDOUT_FILENO);
     close (sv[1]);
 
     /* Restore SIGPIPE back to SIG_DFL. */

In the child process:

- we have no need for the parent-side end of the socket pair (sv[0]),

- we duplicate the child-side end of the socket pair (sv[1]) to both of
  the child's fd#0 and fd#1, and then close sv[1].

  close (STDIN_FILENO);
  close (STDOUT_FILENO);
  close (sv[0]);
  dup2 (sv[1], STDIN_FILENO);
  dup2 (sv[1], STDOUT_FILENO);
  close (sv[1]);

This code silently assumes that sv[1] falls outside of the the fd set
{0,1} -- put differently, the code assumes that each dup2() call will
duplicate sv[1] to a file descriptor that is *different* from sv[1].

Let's investigate (a) if the logic is correct under said assumption, and
(b) what happened if the assumption were wrong, (c) whether the assumption
is valid to make.

(a) Under the assumption, the initial two close() calls turn out to be
    superfluous. That's because dup2 (oldfd, newfd) closes "newfd"
first,
    if "newfd" is open and *differs* from "oldfd".

(b) If the assumption were wrong -- that is, if sv[1] equaled 0 or 1 --,
    then the logic would misbehave, at several levels.

    First, one of the initial close() calls would cause sv[1] to be
    closed, thereby breaking both dup2() calls.

    Second, even if we removed the first two close() calls, the final
    close() would still be wrong. In this case, one of the dup2() calls
    would be a no-op (oldfd == newfd), and the other dup2() would work
    correctly. However, the final close would retroactively invalidate the
    one dup2() call that had behaved as a no-op.

(c) Now let's see whether we need to fix issue (b); that is, whether the
    assumption that sv[1] differs from both 0 and 1 is valid to make.

    This assumption is actually valid. ISO C guarantees that the stdin,
    stdout and stderr I/O streams are open at program startup, and POSIX
    translates the same requirement to file descriptors. In particular,
    the *informative* rationale in POSIX tallies its *normative*
    requirements as follows:

   
https://pubs.opengroup.org/onlinepubs/9699919799/xrat/V4_xsh_chap02.html#tag_22_02_05

    | B.2.5 Standard I/O Streams
    |
    | Although the ISO C standard guarantees that, at program start-up,
    | /stdin/ is open for reading and /stdout/ and /stderr/ are open for
    | writing, this guarantee is contingent (as are all guarantees made by
    | the ISO C and POSIX standards) on the program being executed in a
    | conforming environment. Programs executed with file descriptor 0 not
    | open for reading or with file descriptor 1 or 2 not open for writing
    | are executed in a non-conforming environment. Application writers
    | are warned (in [exec], [posix_spawn], and [Redirection]) not to
    | execute a standard utility or a conforming application with file
    | descriptor 0 not open for reading or with file descriptor 1 or 2 not
    | open for writing.

    The proper way for "disabling" these file descriptors (as
described by
    XRAT as well) is to redirect them from/to "/dev/null".

    Now, if the libnbd client application closed file descriptors 0 and/or
    1 after its startup, it would effectively invalidate itself. Consider
    the (informative) APPLICATION USAGE section of the fclose() spec:

   
https://pubs.opengroup.org/onlinepubs/9699919799/functions/fclose.html#tag_16_121_07

    | Since after the call to /fclose()/ any use of stream results in
    | undefined behavior, /fclose()/ should not be used on /stdin/,
    | /stdout/, or /stderr/ except immediately before process termination
    | (see XBD [Process Termination]), so as to avoid triggering undefined
    | behavior in other standard interfaces that rely on these streams. If
    | there are any [atexit()] handlers registered by the application,
    | such a call to /fclose()/ should not occur until the last handler is
    | finishing. Once /fclose()/ has been used to close /stdin/, /stdout/,
    | or /stderr/, there is no standard way to reopen any of these
    | streams.
    |
    | Use of [freopen()] to change /stdin/, /stdout/, or /stderr/ instead
    | of closing them avoids the danger of a file unexpectedly being
    | opened as one of the special file descriptors STDIN_FILENO,
    | STDOUT_FILENO, or STDERR_FILENO at a later time in the application.

    Thus, the assumption is deemed valid.

Therefore:

- While valid, the assumption is not trivial. So, assert it in the child
  process. Furthermore, because regular assert()'s in the parent process
  may be easier to read for the user, assert a slightly more comprehensive
  predicate about socketpair()'s output there, too.

- Remove the first two close() calls, which are superfluous.

Signed-off-by: Laszlo Ersek <lersek at redhat.com>
Reviewed-by: Richard W.M. Jones <rjones at redhat.com>
---

Notes:
    v4:
    
    - pick up Rich's R-b
    
    context:-U6

 generator/states-connect.c | 13 +++++++++++--
 1 file changed, 11 insertions(+), 2 deletions(-)

diff --git a/generator/states-connect.c b/generator/states-connect.c
index 5612da53f59f..19c33f367519 100644
--- a/generator/states-connect.c
+++ b/generator/states-connect.c
@@ -248,26 +248,35 @@  CONNECT_COMMAND.START:
   if (nbd_internal_socketpair (AF_UNIX, SOCK_STREAM, 0, sv) == -1) {
     SET_NEXT_STATE (%.DEAD);
     set_error (errno, "socketpair");
     return 0;
   }
 
+  /* A process is effectively in an unusable state if any of STDIN_FILENO
+   * (fd#0), STDOUT_FILENO (fd#1) and STDERR_FILENO (fd#2) don't exist. If
they
+   * exist however, then the socket pair created above will not intersect with
+   * the fd set { 0, 1, 2 }. This is relevant for the child-side dup2() logic
+   * below.
+   */
+  assert (sv[0] > STDERR_FILENO);
+  assert (sv[1] > STDERR_FILENO);
+
   pid = fork ();
   if (pid == -1) {
     SET_NEXT_STATE (%.DEAD);
     set_error (errno, "fork");
     close (sv[0]);
     close (sv[1]);
     return 0;
   }
   if (pid == 0) {         /* child - run command */
-    close (STDIN_FILENO);
-    close (STDOUT_FILENO);
     close (sv[0]);
     dup2 (sv[1], STDIN_FILENO);
     dup2 (sv[1], STDOUT_FILENO);
+    NBD_INTERNAL_FORK_SAFE_ASSERT (sv[1] != STDIN_FILENO);
+    NBD_INTERNAL_FORK_SAFE_ASSERT (sv[1] != STDOUT_FILENO);
     close (sv[1]);
 
     /* Restore SIGPIPE back to SIG_DFL. */
     signal (SIGPIPE, SIG_DFL);
 
     execvp (h->argv.ptr[0], h->argv.ptr);

It's bad hygiene to just assume dup2(), close() and signal() will succeed.
Check all return values.

(Exit with status 126 on the new error branches for consistency with the
child process fcntl() -- and other syscall -- error modes in
"generator/states-connect-socket-activation.c".

See also these links, provided by Rich:
<https://listman.redhat.com/archives/libguestfs/2019-September/022737.html>,
<https://listman.redhat.com/archives/libguestfs/2019-September/022739.html>,
<https://www.gnu.org/software/bash/manual/html_node/Exit-Status.html>.)

Signed-off-by: Laszlo Ersek <lersek at redhat.com>
Reviewed-by: Richard W.M. Jones <rjones at redhat.com>
---

Notes:
    v4:
    
    - pick up Rich's R-b
    
    - explain the use of exit status 126 in the commit message [Rich]
    
    context:-U10

 generator/states-connect.c | 22 +++++++++++++++-----
 1 file changed, 17 insertions(+), 5 deletions(-)

diff --git a/generator/states-connect.c b/generator/states-connect.c
index 19c33f367519..750f8b765578 100644
--- a/generator/states-connect.c
+++ b/generator/states-connect.c
@@ -262,29 +262,41 @@  CONNECT_COMMAND.START:
 
   pid = fork ();
   if (pid == -1) {
     SET_NEXT_STATE (%.DEAD);
     set_error (errno, "fork");
     close (sv[0]);
     close (sv[1]);
     return 0;
   }
   if (pid == 0) {         /* child - run command */
-    close (sv[0]);
-    dup2 (sv[1], STDIN_FILENO);
-    dup2 (sv[1], STDOUT_FILENO);
+    if (close (sv[0]) == -1) {
+      nbd_internal_fork_safe_perror ("close");
+      _exit (126);
+    }
+    if (dup2 (sv[1], STDIN_FILENO) == -1 ||
+        dup2 (sv[1], STDOUT_FILENO) == -1) {
+      nbd_internal_fork_safe_perror ("dup2");
+      _exit (126);
+    }
     NBD_INTERNAL_FORK_SAFE_ASSERT (sv[1] != STDIN_FILENO);
     NBD_INTERNAL_FORK_SAFE_ASSERT (sv[1] != STDOUT_FILENO);
-    close (sv[1]);
+    if (close (sv[1]) == -1) {
+      nbd_internal_fork_safe_perror ("close");
+      _exit (126);
+    }
 
     /* Restore SIGPIPE back to SIG_DFL. */
-    signal (SIGPIPE, SIG_DFL);
+    if (signal (SIGPIPE, SIG_DFL) == SIG_ERR) {
+      nbd_internal_fork_safe_perror ("signal");
+      _exit (126);
+    }
 
     execvp (h->argv.ptr[0], h->argv.ptr);
     nbd_internal_fork_safe_perror (h->argv.ptr[0]);
     if (errno == ENOENT)
       _exit (127);
     else
       _exit (126);
   }
 
   /* Parent. */

This patch implements the same ideas as those presented in the prior patch

  socket activation: centralize resource release

of this same series, only for CONNECT_COMMAND.START.

The one temporary resource we have here is sv[1].

Similarly to the above-cited patch, the present patch also exposes a
resource leak on the error path. Namely, when any construction step fails
in the parent after fork(), the child process is leaked. (The child
process may well be killed and reaped once the application decides to call
nbd_close(), but until then, after the CONNECT_COMMAND.START handler
returns with failure, unusable (half-constructed) resources linger around
indefinitely. A library API should either fully succeed or fully fail.) We
are going to plug this leak in a subsequent patch.

Signed-off-by: Laszlo Ersek <lersek at redhat.com>
Reviewed-by: Richard W.M. Jones <rjones at redhat.com>
---

Notes:
    v4:
    
    - pick up Rich's R-b
    
    context:-W

 generator/states-connect.c | 52 +++++++++++---------
 1 file changed, 30 insertions(+), 22 deletions(-)

diff --git a/generator/states-connect.c b/generator/states-connect.c
index 750f8b765578..0fe44f6d44d5 100644
--- a/generator/states-connect.c
+++ b/generator/states-connect.c
@@ -238,96 +238,104 @@  CONNECT_TCP.NEXT_ADDRESS:
   return 0;
 
  CONNECT_COMMAND.START:
+  enum state next;
   int sv[2];
   pid_t pid;
   int flags;
+  struct socket *sock;
 
   assert (!h->sock);
   assert (h->argv.ptr);
   assert (h->argv.ptr[0]);
+
+  next = %.DEAD;
+
   if (nbd_internal_socketpair (AF_UNIX, SOCK_STREAM, 0, sv) == -1) {
-    SET_NEXT_STATE (%.DEAD);
     set_error (errno, "socketpair");
-    return 0;
+    goto done;
   }
 
   /* A process is effectively in an unusable state if any of STDIN_FILENO
    * (fd#0), STDOUT_FILENO (fd#1) and STDERR_FILENO (fd#2) don't exist. If
they
    * exist however, then the socket pair created above will not intersect with
    * the fd set { 0, 1, 2 }. This is relevant for the child-side dup2() logic
    * below.
    */
   assert (sv[0] > STDERR_FILENO);
   assert (sv[1] > STDERR_FILENO);
 
   pid = fork ();
   if (pid == -1) {
-    SET_NEXT_STATE (%.DEAD);
     set_error (errno, "fork");
-    close (sv[0]);
-    close (sv[1]);
-    return 0;
+    goto close_socket_pair;
   }
+
   if (pid == 0) {         /* child - run command */
     if (close (sv[0]) == -1) {
       nbd_internal_fork_safe_perror ("close");
       _exit (126);
     }
     if (dup2 (sv[1], STDIN_FILENO) == -1 ||
         dup2 (sv[1], STDOUT_FILENO) == -1) {
       nbd_internal_fork_safe_perror ("dup2");
       _exit (126);
     }
     NBD_INTERNAL_FORK_SAFE_ASSERT (sv[1] != STDIN_FILENO);
     NBD_INTERNAL_FORK_SAFE_ASSERT (sv[1] != STDOUT_FILENO);
     if (close (sv[1]) == -1) {
       nbd_internal_fork_safe_perror ("close");
       _exit (126);
     }
 
     /* Restore SIGPIPE back to SIG_DFL. */
     if (signal (SIGPIPE, SIG_DFL) == SIG_ERR) {
       nbd_internal_fork_safe_perror ("signal");
       _exit (126);
     }
 
     execvp (h->argv.ptr[0], h->argv.ptr);
     nbd_internal_fork_safe_perror (h->argv.ptr[0]);
     if (errno == ENOENT)
       _exit (127);
     else
       _exit (126);
   }
 
-  /* Parent. */
-  close (sv[1]);
-
-  h->pid = pid;
-
-  /* Only the parent-side end of the socket pair must be set to non-blocking,
+  /* Parent.
+   *
+   * Only the parent-side end of the socket pair must be set to non-blocking,
    * because the child may not be expecting a non-blocking socket.
    */
   flags = fcntl (sv[0], F_GETFL, 0);
   if (flags == -1 ||
       fcntl (sv[0], F_SETFL, flags|O_NONBLOCK) == -1) {
-    SET_NEXT_STATE (%.DEAD);
     set_error (errno, "fcntl");
-    close (sv[0]);
-    return 0;
+    goto close_socket_pair;
   }
 
-  h->sock = nbd_internal_socket_create (sv[0]);
-  if (!h->sock) {
-    SET_NEXT_STATE (%.DEAD);
+  sock = nbd_internal_socket_create (sv[0]);
+  if (!sock)
     /* nbd_internal_socket_create() calls set_error() internally */
-    close (sv[0]);
-    return 0;
-  }
+    goto close_socket_pair;
+
+  /* Commit. */
+  h->pid = pid;
+  h->sock = sock;
 
   /* The sockets are connected already, we can jump directly to
    * receiving the server magic.
    */
-  SET_NEXT_STATE (%^MAGIC.START);
+  next = %^MAGIC.START;
+
+  /* fall through, for releasing the temporaries */
+
+close_socket_pair:
+  if (next == %.DEAD)
+    close (sv[0]);
+  close (sv[1]);
+
+done:
+  SET_NEXT_STATE (next);
   return 0;
 
 } /* END STATE MACHINE */

A prior patch highlighted the following leak: when any construction step
fails in the parent after fork(), the child process is leaked.

We could plug the leak by inserting a new error handling section for
killing the child process and reaping it with waitpid(). However, it would
raise some new questions (what signal to send, ensure the child not ignore
that signal, hope that whatever process image we execute in the child
handle that signal properly, etc).

Instead, rearrange the steps in the CONNECT_COMMAND.START handler so that
fork() be the last operation in the parent process, on the construction
path. If fork() succeeds, let the entire handler succeed. For this, move
the fcntl() and nbd_internal_socket_create() calls above fork().

(The hoisting of the fcntl() calls is where we rely on the earlier
observation that O_NONBLOCK only applies to the parent-side end of the
socket pair, so it is safe to do before forking.)

The trouble in turn is that nbd_internal_socket_create() takes ownership
of the parent-side end of the socket pair (sv[0]). So once that function
returns successfully, we can't close sv[0] even on the error path -- we
close the "higher level" socket, and that invalidates sv[0] at once.
Track
this ownership transfer with a new boolean flag therefore.

Signed-off-by: Laszlo Ersek <lersek at redhat.com>
Reviewed-by: Richard W.M. Jones <rjones at redhat.com>
---

Notes:
    v4:
    
    - pick up Rich's R-b
    
    context:-W

 generator/states-connect.c | 48 +++++++++++---------
 1 file changed, 27 insertions(+), 21 deletions(-)

diff --git a/generator/states-connect.c b/generator/states-connect.c
index 0fe44f6d44d5..4dce7e01f457 100644
--- a/generator/states-connect.c
+++ b/generator/states-connect.c
@@ -239,103 +239,109 @@  CONNECT_TCP.NEXT_ADDRESS:
 
  CONNECT_COMMAND.START:
   enum state next;
+  bool parentfd_transferred;
   int sv[2];
-  pid_t pid;
   int flags;
   struct socket *sock;
+  pid_t pid;
 
   assert (!h->sock);
   assert (h->argv.ptr);
   assert (h->argv.ptr[0]);
 
   next = %.DEAD;
+  parentfd_transferred = false;
 
   if (nbd_internal_socketpair (AF_UNIX, SOCK_STREAM, 0, sv) == -1) {
     set_error (errno, "socketpair");
     goto done;
   }
 
   /* A process is effectively in an unusable state if any of STDIN_FILENO
    * (fd#0), STDOUT_FILENO (fd#1) and STDERR_FILENO (fd#2) don't exist. If
they
    * exist however, then the socket pair created above will not intersect with
    * the fd set { 0, 1, 2 }. This is relevant for the child-side dup2() logic
    * below.
    */
   assert (sv[0] > STDERR_FILENO);
   assert (sv[1] > STDERR_FILENO);
 
+  /* Only the parent-side end of the socket pair must be set to non-blocking,
+   * because the child may not be expecting a non-blocking socket.
+   */
+  flags = fcntl (sv[0], F_GETFL, 0);
+  if (flags == -1 ||
+      fcntl (sv[0], F_SETFL, flags|O_NONBLOCK) == -1) {
+    set_error (errno, "fcntl");
+    goto close_socket_pair;
+  }
+
+  sock = nbd_internal_socket_create (sv[0]);
+  if (!sock)
+    /* nbd_internal_socket_create() calls set_error() internally */
+    goto close_socket_pair;
+  parentfd_transferred = true;
+
   pid = fork ();
   if (pid == -1) {
     set_error (errno, "fork");
-    goto close_socket_pair;
+    goto close_high_level_socket;
   }
 
   if (pid == 0) {         /* child - run command */
     if (close (sv[0]) == -1) {
       nbd_internal_fork_safe_perror ("close");
       _exit (126);
     }
     if (dup2 (sv[1], STDIN_FILENO) == -1 ||
         dup2 (sv[1], STDOUT_FILENO) == -1) {
       nbd_internal_fork_safe_perror ("dup2");
       _exit (126);
     }
     NBD_INTERNAL_FORK_SAFE_ASSERT (sv[1] != STDIN_FILENO);
     NBD_INTERNAL_FORK_SAFE_ASSERT (sv[1] != STDOUT_FILENO);
     if (close (sv[1]) == -1) {
       nbd_internal_fork_safe_perror ("close");
       _exit (126);
     }
 
     /* Restore SIGPIPE back to SIG_DFL. */
     if (signal (SIGPIPE, SIG_DFL) == SIG_ERR) {
       nbd_internal_fork_safe_perror ("signal");
       _exit (126);
     }
 
     execvp (h->argv.ptr[0], h->argv.ptr);
     nbd_internal_fork_safe_perror (h->argv.ptr[0]);
     if (errno == ENOENT)
       _exit (127);
     else
       _exit (126);
   }
 
-  /* Parent.
-   *
-   * Only the parent-side end of the socket pair must be set to non-blocking,
-   * because the child may not be expecting a non-blocking socket.
-   */
-  flags = fcntl (sv[0], F_GETFL, 0);
-  if (flags == -1 ||
-      fcntl (sv[0], F_SETFL, flags|O_NONBLOCK) == -1) {
-    set_error (errno, "fcntl");
-    goto close_socket_pair;
-  }
-
-  sock = nbd_internal_socket_create (sv[0]);
-  if (!sock)
-    /* nbd_internal_socket_create() calls set_error() internally */
-    goto close_socket_pair;
-
-  /* Commit. */
+  /* Parent -- we're done; commit. */
   h->pid = pid;
   h->sock = sock;
 
   /* The sockets are connected already, we can jump directly to
    * receiving the server magic.
    */
   next = %^MAGIC.START;
 
   /* fall through, for releasing the temporaries */
 
-close_socket_pair:
+close_high_level_socket:
   if (next == %.DEAD)
+    sock->ops->close (sock);
+
+close_socket_pair:
+  assert (next == %.DEAD || parentfd_transferred);
+  if (!parentfd_transferred)
     close (sv[0]);
   close (sv[1]);
 
 done:
   SET_NEXT_STATE (next);
   return 0;
 
 } /* END STATE MACHINE */

Per POSIX, execvp() is not safe to call in a child process forked from a
multi-threaded process. We can now replace the execvp() call in the child
process with a call to our fork-safe (async-signal-safe) variant.

Prepare our internal execvpe context on the parent's construction path,
use the context in the child, and release the context in the parent on the
way out, regardless of whether the handler as a whole succeeded or not.
(The context is only a temporary resource.)

Signed-off-by: Laszlo Ersek <lersek at redhat.com>
Reviewed-by: Richard W.M. Jones <rjones at redhat.com>
---

Notes:
    v4:
    
    - pick up Rich's R-b
    
    context:-U7

 generator/states-connect.c | 16 ++++++++++++++--
 1 file changed, 14 insertions(+), 2 deletions(-)

diff --git a/generator/states-connect.c b/generator/states-connect.c
index 4dce7e01f457..65a68003583e 100644
--- a/generator/states-connect.c
+++ b/generator/states-connect.c
@@ -32,14 +32,16 @@
 #include <signal.h>
 #include <netdb.h>
 #include <netinet/in.h>
 #include <netinet/tcp.h>
 #include <sys/types.h>
 #include <sys/socket.h>
 
+extern char **environ;
+
 /* Disable Nagle's algorithm on the socket, but don't fail. */
 static void
 disable_nagle (int sock)
 {
   const int flag = 1;
 
   setsockopt (sock, IPPROTO_TCP, TCP_NODELAY, &flag, sizeof flag);
@@ -239,14 +241,15 @@  CONNECT_TCP.NEXT_ADDRESS:
 
  CONNECT_COMMAND.START:
   enum state next;
   bool parentfd_transferred;
   int sv[2];
   int flags;
   struct socket *sock;
+  struct execvpe execvpe_ctx;
   pid_t pid;
 
   assert (!h->sock);
   assert (h->argv.ptr);
   assert (h->argv.ptr[0]);
 
   next = %.DEAD;
@@ -278,18 +281,24 @@  CONNECT_COMMAND.START:
 
   sock = nbd_internal_socket_create (sv[0]);
   if (!sock)
     /* nbd_internal_socket_create() calls set_error() internally */
     goto close_socket_pair;
   parentfd_transferred = true;
 
+  if (nbd_internal_execvpe_init (&execvpe_ctx, h->argv.ptr[0],
h->argv.len) =+      -1) {
+    set_error (errno, "nbd_internal_execvpe_init");
+    goto close_high_level_socket;
+  }
+
   pid = fork ();
   if (pid == -1) {
     set_error (errno, "fork");
-    goto close_high_level_socket;
+    goto uninit_execvpe;
   }
 
   if (pid == 0) {         /* child - run command */
     if (close (sv[0]) == -1) {
       nbd_internal_fork_safe_perror ("close");
       _exit (126);
     }
@@ -307,15 +316,15 @@  CONNECT_COMMAND.START:
 
     /* Restore SIGPIPE back to SIG_DFL. */
     if (signal (SIGPIPE, SIG_DFL) == SIG_ERR) {
       nbd_internal_fork_safe_perror ("signal");
       _exit (126);
     }
 
-    execvp (h->argv.ptr[0], h->argv.ptr);
+    (void)nbd_internal_fork_safe_execvpe (&execvpe_ctx, &h->argv,
environ);
     nbd_internal_fork_safe_perror (h->argv.ptr[0]);
     if (errno == ENOENT)
       _exit (127);
     else
       _exit (126);
   }
 
@@ -326,14 +335,17 @@  CONNECT_COMMAND.START:
   /* The sockets are connected already, we can jump directly to
    * receiving the server magic.
    */
   next = %^MAGIC.START;
 
   /* fall through, for releasing the temporaries */
 
+uninit_execvpe:
+  nbd_internal_execvpe_uninit (&execvpe_ctx);
+
 close_high_level_socket:
   if (next == %.DEAD)
     sock->ops->close (sock);
 
 close_socket_pair:
   assert (next == %.DEAD || parentfd_transferred);
   if (!parentfd_transferred)

In the future, we'd like to add systemd socket activation environment
variables such that:

- their values may not be constants (not even for pre-allocating space),

- they may be optional,

- regardless of some optional variables being added or not, the positions
  of those that we do add can be captured, for later reference.

Generalize prepare_socket_activation_environment() accordingly. Write the
child PID to "LISTEN_PID=xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx" with the
new
"capturing" interface.

Signed-off-by: Laszlo Ersek <lersek at redhat.com>
Reviewed-by: Richard W.M. Jones <rjones at redhat.com>
---

Notes:
    v4:
    
    - pick up Rich's R-b
    
    - resolve rebase conflicts (a) inside
      prepare_socket_activation_environment(), (b) near the
      prepare_socket_activation_environment() call site -- due to keeping
      set_error() internal to prepare_socket_activation_environment(), in
      patch "socket activation: clean up responsibilities of
      prep.sock.act.env.()"
    
    context:-U6

 generator/states-connect-socket-activation.c | 160 +++++++++++++++-----
 1 file changed, 125 insertions(+), 35 deletions(-)

diff --git a/generator/states-connect-socket-activation.c
b/generator/states-connect-socket-activation.c
index a214ffd5a6e4..10ccc3119299 100644
--- a/generator/states-connect-socket-activation.c
+++ b/generator/states-connect-socket-activation.c
@@ -27,85 +27,169 @@
 #include <errno.h>
 #include <assert.h>
 #include <sys/socket.h>
 #include <sys/un.h>
 
 #include "internal.h"
+#include "compiler-macros.h"
+#include "unique-name.h"
+#include "array-size.h"
+#include "checked-overflow.h"
 
 /* This is baked into the systemd socket activation API. */
 #define FIRST_SOCKET_ACTIVATION_FD 3
 
-/* == strlen ("LISTEN_PID=") | strlen ("LISTEN_FDS=") */
-#define PREFIX_LENGTH 11
+/* Describes a systemd socket activation environment variable. */
+struct sact_var {
+  const char *prefix; /* variable name and equal sign */
+  size_t prefix_len;
+  const char *value;
+  size_t value_len;
+};
+
+/* Determine the length of a string, using "sizeof" whenever
possible.
+ *
+ * Do not use this macro on an argument that has side effects, as no guarantees
+ * are given regarding the number of times the argument may be evaluated.
+ * TYPE_IS_ARRAY(s) itself may contribute a different number of evaluations
+ * dependent on whether "s" has variably modified type, and then the
conditional
+ * operator either evaluates "sizeof s" (which contributes 0 or 1
evaluations,
+ * dependent on whether "s" has variably modified type) or strlen(s)
(which
+ * contributes 1 evaluation). Also note that the argument of the
"sizeof"
+ * operator is *only* parenthesized because "s" is a macro parameter
here.
+*/
+#define STRLEN1(s) ((TYPE_IS_ARRAY (s) ? sizeof (s) - 1 : strlen (s)))
+
+/* Push a new element to an array of "sact_var" structures.
+ *
+ * "vars" is the array to extend. "num_vars" (of type
(size_t *)) points to the
+ * number of elements that the array, on input, contains; (*num_vars) is
+ * increased by one on output. "prefix" and "value" serve
as the values for
+ * setting the fields in the new element. "ofs" (of type (size_t *))
may be
+ * NULL; if it isn't, then on output, (*ofs) is set to the input value of
+ * (*num_vars): the offset of the just-pushed element.
+ *
+ * Avoid arguments with side-effects here as well.
+ */
+#define SACT_VAR_PUSH(vars, num_vars, prefix, value, ofs)       \
+  SACT_VAR_PUSH1 ((vars), (num_vars), (prefix), (value), (ofs), \
+                  NBDKIT_UNIQUE_NAME (_ofs))
+#define SACT_VAR_PUSH1(vars, num_vars, prefix, value, ofs, ofs1)             \
+  do {                                                                       \
+    size_t *ofs1;                                                            \
+                                                                             \
+    assert (*(num_vars) < ARRAY_SIZE (vars));                               
\
+    ofs1 = (ofs);                                                            \
+    if (ofs1 != NULL)                                                        \
+      *ofs1 = *(num_vars);                                                   \
+    (vars)[(*(num_vars))++] = (struct sact_var){ (prefix), STRLEN1 (prefix), \
+                                                 (value), STRLEN1 (value) }; \
+  } while (0)
 
 extern char **environ;
 
-/* Prepare environment for calling execvp when doing systemd socket
- * activation.  Takes the current environment and copies it.  Removes
- * any existing LISTEN_PID or LISTEN_FDS and replaces them with new
- * variables.  env[0] is "LISTEN_PID=..." which is filled in by
- * CONNECT_SA.START, and env[1] is "LISTEN_FDS=1".
+/* Prepare environment for calling execvp when doing systemd socket activation.
+ * Takes the current environment and copies it. Removes any existing socket
+ * activation variables and replaces them with new ones. Variables in
"sact_var"
+ * will be placed at the front of "env", preserving the order from
"sact_var".
  */
 static int
-prepare_socket_activation_environment (string_vector *env)
+prepare_socket_activation_environment (string_vector *env,
+                                       const struct sact_var *sact_var,
+                                       size_t num_vars)
 {
-  char *p;
+  const struct sact_var *var_end;
+  char *new_var;
+  const struct sact_var *var;
   size_t i;
 
   *env = (string_vector)empty_vector;
 
-  /* Reserve slots env[0] and env[1]. */
-  p = strdup ("LISTEN_PID=xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx");
-  if (p == NULL)
-    goto err;
-  if (string_vector_append (env, p) == -1) {
-    free (p);
-    goto err;
-  }
-  p = strdup ("LISTEN_FDS=1");
-  if (p == NULL)
-    goto err;
-  if (string_vector_append (env, p) == -1) {
-    free (p);
-    goto err;
+  /* Set the exclusive limit for loops over "sact_var". */
+  var_end = sact_var + num_vars;
+
+  /* New environment variable being constructed for "env". */
+  new_var = NULL;
+
+  /* Copy "sact_var" to the front of "env". */
+  for (var = sact_var; var < var_end; ++var) {
+    size_t new_var_size;
+    char *p;
+
+    /* Calculate the size of the "NAME=value" string. */
+    if (ADD_OVERFLOW (var->prefix_len, var->value_len, &new_var_size)
||
+        ADD_OVERFLOW (new_var_size, 1u, &new_var_size)) {
+      errno = EOVERFLOW;
+      goto err;
+    }
+
+    /* Allocate and format "NAME=value". */
+    new_var = malloc (new_var_size);
+    if (new_var == NULL)
+      goto err;
+    p = new_var;
+
+    memcpy (p, var->prefix, var->prefix_len);
+    p += var->prefix_len;
+
+    memcpy (p, var->value, var->value_len);
+    p += var->value_len;
+
+    *p++ = '\0';
+
+    /* Push "NAME=value" to the vector. */
+    if (string_vector_append (env, new_var) == -1)
+      goto err;
+    /* Ownership transferred. */
+    new_var = NULL;
   }
 
-  /* Append the current environment, but remove LISTEN_PID, LISTEN_FDS. */
+  /* Append the current environment to "env", but remove
"sact_var". */
   for (i = 0; environ[i] != NULL; ++i) {
-    if (strncmp (environ[i], "LISTEN_PID=", PREFIX_LENGTH) != 0
&&
-        strncmp (environ[i], "LISTEN_FDS=", PREFIX_LENGTH) != 0) {
-      char *copy = strdup (environ[i]);
-      if (copy == NULL)
-        goto err;
-      if (string_vector_append (env, copy) == -1) {
-        free (copy);
-        goto err;
-      }
+    for (var = sact_var; var < var_end; ++var) {
+      if (strncmp (environ[i], var->prefix, var->prefix_len) == 0)
+        break;
     }
+    /* Drop known socket activation variable from the current environment. */
+    if (var < var_end)
+      continue;
+
+    new_var = strdup (environ[i]);
+    if (new_var == NULL)
+      goto err;
+
+    if (string_vector_append (env, new_var) == -1)
+      goto err;
+    /* Ownership transferred. */
+    new_var = NULL;
   }
 
   /* The environ must be NULL-terminated. */
   if (string_vector_append (env, NULL) == -1)
     goto err;
 
   return 0;
 
  err:
   set_error (errno, "malloc");
+  free (new_var);
   string_vector_empty (env);
   return -1;
 }
 
 STATE_MACHINE {
  CONNECT_SA.START:
   enum state next;
   char *tmpdir;
   char *sockpath;
   int s;
   struct sockaddr_un addr;
   struct execvpe execvpe_ctx;
+  size_t num_vars;
+  struct sact_var sact_var[2];
+  size_t pid_ofs;
   string_vector env;
   pid_t pid;
 
   assert (!h->sock);
   assert (h->argv.ptr);
   assert (h->argv.ptr[0]);
@@ -153,13 +237,18 @@  CONNECT_SA.START:
   if (nbd_internal_execvpe_init (&execvpe_ctx, h->argv.ptr[0],
h->argv.len) =       -1) {
     set_error (errno, "nbd_internal_execvpe_init");
     goto unlink_sockpath;
   }
 
-  if (prepare_socket_activation_environment (&env) == -1)
+  num_vars = 0;
+  SACT_VAR_PUSH (sact_var, &num_vars,
+                 "LISTEN_PID=",
"xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx", &pid_ofs);
+  SACT_VAR_PUSH (sact_var, &num_vars,
+                 "LISTEN_FDS=", "1", NULL);
+  if (prepare_socket_activation_environment (&env, sact_var, num_vars) ==
-1)
     /* prepare_socket_activation_environment() calls set_error() internally */
     goto uninit_execvpe;
 
   pid = fork ();
   if (pid == -1) {
     set_error (errno, "fork");
@@ -193,13 +282,14 @@  CONNECT_SA.START:
       }
     }
 
     char buf[32];
     const char *v        nbd_internal_fork_safe_itoa ((long) getpid (), buf,
sizeof buf);
-    strcpy (&env.ptr[0][PREFIX_LENGTH], v);
+    NBD_INTERNAL_FORK_SAFE_ASSERT (strlen (v) <=
sact_var[pid_ofs].value_len);
+    strcpy (env.ptr[pid_ofs] + sact_var[pid_ofs].prefix_len, v);
 
     /* Restore SIGPIPE back to SIG_DFL. */
     if (signal (SIGPIPE, SIG_DFL) == SIG_ERR) {
       nbd_internal_fork_safe_perror ("signal");
       _exit (126);
     }

From: "Richard W.M. Jones" <rjones at redhat.com>

[Laszlo's note: "ascii-ctype.h" and "test-ascii-ctype.c"
match those of
nbdkit at commit 45b72f5bd8fc. Originally posted by Rich at
<https://listman.redhat.com/archives/libguestfs/2023-January/030559.html>.]

Message-Id: <20230130225521.1771496-3-rjones at redhat.com>
Signed-off-by: Laszlo Ersek <lersek at redhat.com>
Reviewed-by: Richard W.M. Jones <rjones at redhat.com>
---

Notes:
    v4:
    
    - pick up Rich's R-b
    
    - refresh patch and commit message against nbdkit 45b72f5bd8fc

 common/include/ascii-ctype.h      | 75 +++++++++++++++++
 common/include/Makefile.am        |  6 ++
 common/include/test-ascii-ctype.c | 88 ++++++++++++++++++++
 .gitignore                        |  1 +
 4 files changed, 170 insertions(+)

diff --git a/common/include/ascii-ctype.h b/common/include/ascii-ctype.h
new file mode 100644
index 000000000000..191a0f27e19d
--- /dev/null
+++ b/common/include/ascii-ctype.h
@@ -0,0 +1,75 @@
+/* nbdkit
+ * Copyright Red Hat
+ *
+ * Redistribution and use in source and binary forms, with or without
+ * modification, are permitted provided that the following conditions are
+ * met:
+ *
+ * * Redistributions of source code must retain the above copyright
+ * notice, this list of conditions and the following disclaimer.
+ *
+ * * Redistributions in binary form must reproduce the above copyright
+ * notice, this list of conditions and the following disclaimer in the
+ * documentation and/or other materials provided with the distribution.
+ *
+ * * Neither the name of Red Hat nor the names of its contributors may be
+ * used to endorse or promote products derived from this software without
+ * specific prior written permission.
+ *
+ * THIS SOFTWARE IS PROVIDED BY RED HAT AND CONTRIBUTORS ''AS
IS'' AND
+ * ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO,
+ * THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A
+ * PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL RED HAT OR
+ * CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL,
+ * SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT
+ * LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF
+ * USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND
+ * ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY,
+ * OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT
+ * OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF
+ * SUCH DAMAGE.
+ */
+
+/* Normal ctype functions are affected by the current locale.  For
+ * example isupper() might recognize ? in some but not all locales.
+ * These functions match only 7 bit ASCII characters.
+ */
+
+#ifndef NBDKIT_ASCII_CTYPE_H
+#define NBDKIT_ASCII_CTYPE_H
+
+#define ascii_isalnum(c) (ascii_isalpha (c) || ascii_isdigit (c))
+
+#define ascii_isalpha(c)                                        \
+  (((c) >= 'a' && (c) <= 'z') || ((c) >=
'A' && (c) <= 'Z'))
+
+#define ascii_isdigit(c)                        \
+  ((c) >= '0' && (c) <= '9')
+
+#define ascii_isspace(c)                                                \
+  ((c) == '\t' || (c) == '\n' || (c) == '\f' || (c) ==
'\r' || (c) == ' ')
+
+#define ascii_isupper(c)                        \
+  ((c) >= 'A' && (c) <= 'Z')
+
+#define ascii_islower(c)                        \
+  ((c) >= 'a' && (c) <= 'z')
+
+/* See also hexdigit.h */
+#define ascii_isxdigit(c)                                               \
+  ((c) == '0' || (c) == '1' || (c) == '2' || (c) ==
'3' || (c) == '4' || \
+   (c) == '5' || (c) == '6' || (c) == '7' || (c) ==
'8' || (c) == '9' || \
+   (c) == 'a' || (c) == 'b' || (c) == 'c' ||           
\
+   (c) == 'd' || (c) == 'e' || (c) == 'f' ||           
\
+   (c) == 'A' || (c) == 'B' || (c) == 'C' ||           
\
+   (c) == 'D' || (c) == 'E' || (c) == 'F')
+
+#define ascii_tolower(c)                        \
+  (ascii_isupper ((c)) ? (c) + 32 : (c))
+
+#define ascii_toupper(c)                        \
+  (ascii_islower ((c)) ? (c) - 32 : (c))
+
+#define ascii_isprint(c) ((c) >= 32 && (c) <= 126)
+
+#endif /* NBDKIT_ASCII_CTYPE_H */
diff --git a/common/include/Makefile.am b/common/include/Makefile.am
index 8c99f76b8e73..1acc9fa41311 100644
--- a/common/include/Makefile.am
+++ b/common/include/Makefile.am
@@ -20,6 +20,7 @@ include $(top_srcdir)/subdir-rules.mk
 EXTRA_DIST = \
 	ansi-colours.h \
 	array-size.h \
+	ascii-ctype.h \
 	byte-swapping.h \
 	checked-overflow.h \
 	compiler-macros.h \
@@ -36,6 +37,7 @@ EXTRA_DIST = \
 
 TESTS = \
 	test-array-size \
+	test-ascii-ctype \
 	test-byte-swapping \
 	test-checked-overflow \
 	test-isaligned \
@@ -49,6 +51,10 @@ test_array_size_SOURCES = test-array-size.c array-size.h
 test_array_size_CPPFLAGS = -I$(srcdir)
 test_array_size_CFLAGS = $(WARNINGS_CFLAGS)
 
+test_ascii_ctype_SOURCES = test-ascii-ctype.c ascii-ctype.h
+test_ascii_ctype_CPPFLAGS = -I$(srcdir)
+test_ascii_ctype_CFLAGS = $(WARNINGS_CFLAGS)
+
 test_byte_swapping_SOURCES = test-byte-swapping.c byte-swapping.h
 test_byte_swapping_CPPFLAGS = -I$(srcdir)
 test_byte_swapping_CFLAGS = $(WARNINGS_CFLAGS)
diff --git a/common/include/test-ascii-ctype.c
b/common/include/test-ascii-ctype.c
new file mode 100644
index 000000000000..aefe1cc63640
--- /dev/null
+++ b/common/include/test-ascii-ctype.c
@@ -0,0 +1,88 @@
+/* nbdkit
+ * Copyright Red Hat
+ *
+ * Redistribution and use in source and binary forms, with or without
+ * modification, are permitted provided that the following conditions are
+ * met:
+ *
+ * * Redistributions of source code must retain the above copyright
+ * notice, this list of conditions and the following disclaimer.
+ *
+ * * Redistributions in binary form must reproduce the above copyright
+ * notice, this list of conditions and the following disclaimer in the
+ * documentation and/or other materials provided with the distribution.
+ *
+ * * Neither the name of Red Hat nor the names of its contributors may be
+ * used to endorse or promote products derived from this software without
+ * specific prior written permission.
+ *
+ * THIS SOFTWARE IS PROVIDED BY RED HAT AND CONTRIBUTORS ''AS
IS'' AND
+ * ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO,
+ * THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A
+ * PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL RED HAT OR
+ * CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL,
+ * SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT
+ * LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF
+ * USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND
+ * ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY,
+ * OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT
+ * OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF
+ * SUCH DAMAGE.
+ */
+
+#include <config.h>
+
+#include <stdio.h>
+#include <stdlib.h>
+#undef NDEBUG /* Keep test strong even for nbdkit built without assertions */
+#include <assert.h>
+
+#include "ascii-ctype.h"
+
+int
+main (void)
+{
+  assert (ascii_isspace (' '));
+  assert (ascii_isspace ('\t'));
+  assert (ascii_isspace ('\n'));
+  assert (! ascii_isspace ('a'));
+
+  assert (ascii_isalpha ('a'));
+  assert (ascii_isalpha ('Z'));
+  assert (ascii_isalpha ('z'));
+  assert (! ascii_isalpha (' '));
+  assert (! ascii_isalpha ('0'));
+  { const char *s = "?"; assert (! ascii_isalpha (s[0])); }
+  { const char *s = "?"; assert (! ascii_isalpha (s[0])); }
+
+  assert (ascii_isdigit ('0'));
+  assert (ascii_isdigit ('9'));
+  { const char *s = "?"; assert (! ascii_isdigit (s[0])); } /* U+00D8
*/
+  { const char *s = "?"; assert (! ascii_isdigit (s[0])); } /* U+FF19
*/
+
+  assert (ascii_islower ('a'));
+  assert (ascii_islower ('z'));
+  assert (! ascii_islower ('Z'));
+  { const char *s = "?"; assert (! ascii_islower (s[0])); }
+
+  assert (ascii_isupper ('A'));
+  assert (ascii_isupper ('Z'));
+  assert (! ascii_isupper ('z'));
+  { const char *s = "?"; assert (! ascii_isupper (s[0])); }
+
+  assert (ascii_tolower ('A') == 'a');
+  assert (ascii_tolower ('Z') == 'z');
+  assert (ascii_tolower ('a') == 'a');
+  assert (ascii_tolower ('z') == 'z');
+  assert (ascii_tolower ('0') == '0');
+  { const char *s = "?"; assert (ascii_tolower (s[0]) == s[0]); }
+
+  assert (ascii_toupper ('a') == 'A');
+  assert (ascii_toupper ('z') == 'Z');
+  assert (ascii_toupper ('A') == 'A');
+  assert (ascii_toupper ('Z') == 'Z');
+  assert (ascii_toupper ('0') == '0');
+  { const char *s = "?"; assert (ascii_toupper (s[0]) == s[0]); }
+
+  exit (EXIT_SUCCESS);
+}
diff --git a/.gitignore b/.gitignore
index 9df5d9f9db4c..df4b6ca249c6 100644
--- a/.gitignore
+++ b/.gitignore
@@ -34,6 +34,7 @@ Makefile.in
 /bash-completion/nbdinfo
 /bash-completion/nbdublk
 /common/include/test-array-size
+/common/include/test-ascii-ctype
 /common/include/test-byte-swapping
 /common/include/test-checked-overflow
 /common/include/test-isaligned

From: "Richard W.M. Jones" <rjones at redhat.com>

To allow us to name the socket passed down to the NBD server when
calling nbd_connect_systemd_socket_activation(3), we need to add the
field to the handle and add access functions.

[Laszlo's note: originally posted by Rich at
<https://listman.redhat.com/archives/libguestfs/2023-January/030557.html>.
I've renamed "sa_name" to "sact_name", due to
<signal.h> reserving symbols
with the "sa_" prefix. This corresponds to earlier patches in this
series,
such as 'socket activation: rename sa_(tmpdir|sockpath) to
sact_(tmpdir|sockpath)' and 'ocaml: rename "sa_u" to
"saddr_u"'.]

Message-Id: <20230130225521.1771496-4-rjones at redhat.com>
Signed-off-by: Laszlo Ersek <lersek at redhat.com>
Reviewed-by: Richard W.M. Jones <rjones at redhat.com>
---

Notes:
    v4:
    
    - pick up Rich's R-b

 lib/internal.h   |  1 +
 generator/API.ml | 49 +++++++++++++++++
 lib/handle.c     | 56 ++++++++++++++++++++
 3 files changed, 106 insertions(+)

diff --git a/lib/internal.h b/lib/internal.h
index 35cb5e8994ee..2de8e4e5e043 100644
--- a/lib/internal.h
+++ b/lib/internal.h
@@ -101,6 +101,7 @@ struct nbd_handle {
   _Atomic uintptr_t private_data;
 
   char *export_name;            /* Export name, never NULL. */
+  char *sact_name;              /* Socket activation name, can be NULL. */
 
   /* TLS settings. */
   int tls;                      /* 0 = disable, 1 = enable, 2 = require */
diff --git a/generator/API.ml b/generator/API.ml
index 91e57a4c7c4f..24f97e647d2c 100644
--- a/generator/API.ml
+++ b/generator/API.ml
@@ -2036,15 +2036,62 @@   "connect_systemd_socket_activation", {
 
 When the NBD handle is closed the server subprocess
 is killed.
+
+=head3 Socket name
+
+The socket activation protocol lets you optionally give
+the socket a name.  If used, the name is passed to the
+NBD server using the C<LISTEN_FDNAMES> environment
+variable.  To provide a socket name, call
+L<nbd_set_socket_activation_name(3)> before calling
+the connect function.
 " ^ blocking_connect_call_description;
     see_also = [Link "aio_connect_systemd_socket_activation";
                 Link "connect_command"; Link
"kill_subprocess";
                 Link "set_opt_mode";
+                Link "set_socket_activation_name";
+                Link "get_socket_activation_name";
                 ExternalLink ("qemu-nbd", 1);
                 URLLink
"http://0pointer.de/blog/projects/socket-activation.html"];
     example = Some "examples/open-qcow2.c";
   };
 
+  "set_socket_activation_name", {
+    default_call with
+    args = [ String "socket_name" ]; ret = RErr;
+    shortdesc = "set the socket activation name";
+    longdesc = "\
+When running an NBD server using
+L<nbd_connect_systemd_socket_activation(3)> you can optionally
+name the socket.  Call this function before connecting to the
+server.
+
+Some servers such as L<qemu-storage-daemon(1)>
+can use this information to associate the socket with a name
+used on the command line, but most servers will ignore it.
+The name is passed through the C<LISTEN_FDNAMES> environment
+variable.
+
+The parameter C<socket_name> can be a short alphanumeric string.
+If it is set to the empty string (also the default when the handle
+is created) then no name is passed to the server.";
+    see_also = [Link "connect_systemd_socket_activation";
+                Link "get_socket_activation_name"];
+  };
+
+  "get_socket_activation_name", {
+    default_call with
+    args = []; ret = RString;
+    shortdesc = "get the socket activation name";
+    longdesc = "\
+Return the socket name used when you call
+L<nbd_connect_systemd_socket_activation(3)> on the same
+handle.  By default this will return the empty string
+meaning that no name is passed to the server.";
+    see_also = [Link "connect_systemd_socket_activation";
+                Link "set_socket_activation_name"];
+  };
+
   "is_read_only", {
     default_call with
     args = []; ret = RBool;
@@ -3844,6 +3891,8 @@   "get_uri", {
   "aio_opt_structured_reply", (1, 16);
   "opt_starttls", (1, 16);
   "aio_opt_starttls", (1, 16);
+  "set_socket_activation_name", (1, 16);
+  "get_socket_activation_name", (1, 16);
 
   (* These calls are proposed for a future version of libnbd, but
    * have not been added to any released version so far.
diff --git a/lib/handle.c b/lib/handle.c
index 8468b964240b..0f11bee56221 100644
--- a/lib/handle.c
+++ b/lib/handle.c
@@ -28,6 +28,7 @@
 #include <sys/types.h>
 #include <sys/wait.h>
 
+#include "ascii-ctype.h"
 #include "internal.h"
 
 static void
@@ -159,6 +160,7 @@ nbd_close (struct nbd_handle *h)
     waitpid (h->pid, NULL, 0);
 
   free (h->export_name);
+  free (h->sact_name);
   free (h->tls_certificates);
   free (h->tls_username);
   free (h->tls_psk_file);
@@ -197,6 +199,60 @@ nbd_unlocked_get_handle_name (struct nbd_handle *h)
   return copy;
 }
 
+int
+nbd_unlocked_set_socket_activation_name (struct nbd_handle *h,
+                                         const char *name)
+{
+  size_t i, len;
+  char *new_name;
+
+  len = strlen (name);
+
+  /* Setting it to empty string stores NULL in the handle. */
+  if (len == 0) {
+    free (h->sact_name);
+    h->sact_name = NULL;
+    return 0;
+  }
+
+  /* Check the proposed name is short and alphanumeric. */
+  if (len > 32) {
+    set_error (ENAMETOOLONG, "socket activation name should be "
+               "<= 32 characters");
+    return -1;
+  }
+  for (i = 0; i < len; ++i) {
+    if (! ascii_isalnum (name[i])) {
+      set_error (EINVAL, "socket activation name should contain "
+                 "only alphanumeric ASCII characters");
+      return -1;
+    }
+  }
+
+  new_name = strdup (name);
+  if (!new_name) {
+    set_error (errno, "strdup");
+    return -1;
+  }
+
+  free (h->sact_name);
+  h->sact_name = new_name;
+  return 0;
+}
+
+char *
+nbd_unlocked_get_socket_activation_name (struct nbd_handle *h)
+{
+  char *copy = strdup (h->sact_name ? h->sact_name : "");
+
+  if (!copy) {
+    set_error (errno, "strdup");
+    return NULL;
+  }
+
+  return copy;
+}
+
 uintptr_t
 nbd_unlocked_set_private_data (struct nbd_handle *h, uintptr_t data)
 {

When the user calls nbd_set_socket_activation_name before calling
nbd_connect_system_socket_activation, pass the name down to the server
through LISTEN_FDNAMES.  This has no effect unless the new API has
been called to set the socket name to a non-empty string.

https://lists.nongnu.org/archive/html/qemu-devel/2023-01/msg06114.html

[Original commit message and upstream discussion reference by Rich Jones;
at
<https://listman.redhat.com/archives/libguestfs/2023-January/030558.html>
/ msgid <20230130225521.1771496-5-rjones at redhat.com>.]

Signed-off-by: Laszlo Ersek <lersek at redhat.com>
Reviewed-by: Richard W.M. Jones <rjones at redhat.com>
---

Notes:
    v4:
    
    - pick up Rich's R-b
    
    - resolve rebase conflict near the
      prepare_socket_activation_environment() call site, due to keeping
      set_error() internal to that callee, in patch "socket activation:
      clean up responsibilities of prep.sock.act.env.()"

 generator/states-connect-socket-activation.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/generator/states-connect-socket-activation.c
b/generator/states-connect-socket-activation.c
index 10ccc3119299..f9dbd433ce00 100644
--- a/generator/states-connect-socket-activation.c
+++ b/generator/states-connect-socket-activation.c
@@ -185,7 +185,7 @@  CONNECT_SA.START:
   struct sockaddr_un addr;
   struct execvpe execvpe_ctx;
   size_t num_vars;
-  struct sact_var sact_var[2];
+  struct sact_var sact_var[3];
   size_t pid_ofs;
   string_vector env;
   pid_t pid;
@@ -245,6 +245,9 @@  CONNECT_SA.START:
                  "LISTEN_PID=",
"xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx", &pid_ofs);
   SACT_VAR_PUSH (sact_var, &num_vars,
                  "LISTEN_FDS=", "1", NULL);
+  if (h->sact_name != NULL)
+    SACT_VAR_PUSH (sact_var, &num_vars,
+                   "LISTEN_FDNAMES=", h->sact_name, NULL);
   if (prepare_socket_activation_environment (&env, sact_var, num_vars) ==
-1)
     /* prepare_socket_activation_environment() calls set_error() internally */
     goto uninit_execvpe;

On 3/23/23 13:09, Laszlo Ersek wrote:
> V3 was here:
> <http://mid.mail-archive.com/20230215141158.2426855-1-lersek at
redhat.com>.
Meh, *this* version is of course v4.

I updated everything in my git-format-series command line, from my bash
history, from the time of posting v3, *except* of course the "-v3"
option...

Laszlo