Libguestfs - Mar 2023 - [libnbd PATCH v5 0/4] pass LISTEN_FDNAMES with systemd socket activation

V4 was here (incorrectly versioned on the mailing list as v3):
<http://mid.mail-archive.com/20230323121016.1442655-1-lersek at
redhat.com>.

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

Laszlo Ersek (2):
  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

 .gitignore                                   |   1 +
 common/include/Makefile.am                   |   6 +
 common/include/ascii-ctype.h                 |  75 +++++++++
 common/include/test-ascii-ctype.c            |  88 ++++++++++
 generator/API.ml                             |  50 ++++++
 generator/states-connect-socket-activation.c | 170 ++++++++++++++++----
 lib/handle.c                                 |  56 +++++++
 lib/internal.h                               |   1 +
 8 files changed, 412 insertions(+), 35 deletions(-)
 create mode 100644 common/include/ascii-ctype.h
 create mode 100644 common/include/test-ascii-ctype.c


base-commit: a48a1142bc54b09dbd9ed45cc9f9f4945f8174ef

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>
Reviewed-by: Eric Blake <eblake at redhat.com>
---

Notes:
    v5:
    
    - pick up Eric's R-b
    
    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:
    v5:
    
    - no change
    
    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 notes:

- Originally posted by Rich at
 
<https://listman.redhat.com/archives/libguestfs/2023-January/030557.html>
  (Message-Id: <20230130225521.1771496-4-rjones at redhat.com>).

- 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"'.

- Restricted "set_socket_activation_name" to the "Created"
state, per
  Eric's recommendation
 
<http://mid.mail-archive.com/n6lzl7ii5m4hu77n7rrzm3uih3tnx457e4y67rghki34b2ghh7
at 7vyini26mqko>.

- Documented that we are going to pass "unknown" if
"socket_name" is set
  to the empty string
 
<http://mid.mail-archive.com/oqwjnjvq4phqr76yum6zo5erfrm3tvmyewr5nxru3oxklobpgp
at 4plkku7opujw>.]

Signed-off-by: Laszlo Ersek <lersek at redhat.com>
Reviewed-by: Eric Blake <eblake at redhat.com>
---

Notes:
    v5:
    
    - restrict the set_socket_activation_name() API to the Created state
      [Eric]
    
    - Explain that not setting (or unsetting) the socket activation name
      results (per update to the next patch) in the child NBD server seeing
      "unknown" [Eric, Laszlo]
    
    - update the commit message accordingly (as the commit message is still
      based on Rich's original); also move the Message-Id tag (identifying
      Rich's original patch) next to the mailing list archive URL
    
    - drop Rich's R-b due to the above changes
    
    - pick up Eric's R-b
    
    v4:
    
    - pick up Rich's R-b

 lib/internal.h   |  1 +
 generator/API.ml | 50 +++++++++++++++++
 lib/handle.c     | 56 ++++++++++++++++++++
 3 files changed, 107 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..bbb0b8a6e1ee 100644
--- a/generator/API.ml
+++ b/generator/API.ml
@@ -2036,15 +2036,63 @@   "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;
+    permitted_states = [ Created ];
+    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 the name C<unknown> will be seen by 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 +3892,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)
 {

Add LISTEN_FDNAMES to the child process's environment unconditionally.

- If the user calls nbd_set_socket_activation_name() with a non-empty
  string before calling nbd_connect_systemd_socket_activation(), pass the
  name down to the server through LISTEN_FDNAMES.

  This is a feature addition.

- If the user doesn't call nbd_set_socket_activation_name(), or calls it
  with the empty string, add "LISTEN_FDNAMES=unknown" to the
environment,
  overwriting any potentially inherited LISTEN_FDNAMES variable.

  This is a bugfix: otherwise we could leak a confusing LISTEN_FDNAMES to
  the child process, in case the libnbd client app were itself
  socket-activated. Given "LISTEN_FDS=1", the setting
  "LISTEN_FDNAMES=unknown" has the same effect on the child as
removing
  LISTEN_FDNAMES from the environment:
  <https://www.freedesktop.org/software/systemd/man/sd_listen_fds.html>.

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

Notes:
    v5:
    
    - push LISTEN_FDNAMES unconditionally, with value "unknown" if the
      socket activation name is not set by the libnbd client application --
      rewrite patch and commit message [Eric, Laszlo]
    
    - drop Rich's R-b due to the above changes
    
    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 | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

diff --git a/generator/states-connect-socket-activation.c
b/generator/states-connect-socket-activation.c
index 10ccc3119299..b57d5d0075b0 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,16 @@  CONNECT_SA.START:
                  "LISTEN_PID=",
"xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx", &pid_ofs);
   SACT_VAR_PUSH (sact_var, &num_vars,
                  "LISTEN_FDS=", "1", NULL);
+  /* Push LISTEN_FDNAMES unconditionally. This ensures we overwrite any
+   * inherited LISTEN_FDNAMES. If "h->sact_name" is NULL, then
push
+   * "LISTEN_FDNAMES=unknown"; it will have the same effect on the
child process
+   * as unsetting LISTEN_FDNAMES would (with LISTEN_FDS being set to 1):
+   *
<https://www.freedesktop.org/software/systemd/man/sd_listen_fds.html>.
+   */
+  SACT_VAR_PUSH (sact_var, &num_vars,
+                 "LISTEN_FDNAMES=",
+                 h->sact_name == NULL ? "unknown" :
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;