Libguestfs - Jan 2023 - [PATCH libnbd v2 0/4] Pass LISTEN_FDNAMES with systemd socket activation

This is an alternative approach to
https://listman.redhat.com/archives/libguestfs/2023-January/030535.html

After discussing this with Dan Berrange we came to the conclusion that
you really might want to set LISTEN_FDNAMES to arbitrary short strings
(or not set it).  Especially when talking to qemu-storage-daemon which
would allow you to use these names on the command line.

Rich.

Some small refactorings which should not affect the code:

 - Use string_vector_reserve instead of checking each time we append.

 - Get rid of the hard-coded length, and use strncmp (..., s, strlen (s)).
   The compiler should compile this to the same code.
---
 generator/states-connect-socket-activation.c | 23 +++++++-------------
 1 file changed, 8 insertions(+), 15 deletions(-)

diff --git a/generator/states-connect-socket-activation.c
b/generator/states-connect-socket-activation.c
index 9a83834915..24544018fb 100644
--- a/generator/states-connect-socket-activation.c
+++ b/generator/states-connect-socket-activation.c
@@ -34,9 +34,6 @@
 /* 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
-
 extern char **environ;
 
 /* Prepare environment for calling execvp when doing systemd socket
@@ -53,26 +50,22 @@ prepare_socket_activation_environment (string_vector *env)
 
   assert (env->len == 0);
 
-  /* Reserve slots env[0] and env[1]. */
+  /* Reserve slots env[0]..env[1] */
+  if (string_vector_reserve (env, 2) == -1)
+    goto err;
   p = strdup ("LISTEN_PID=xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx");
   if (p == NULL)
     goto err;
-  if (string_vector_append (env, p) == -1) {
-    free (p);
-    goto err;
-  }
+  string_vector_append (env, p);
   p = strdup ("LISTEN_FDS=1");
   if (p == NULL)
     goto err;
-  if (string_vector_append (env, p) == -1) {
-    free (p);
-    goto err;
-  }
+  string_vector_append (env, p);
 
   /* Append the current environment, but remove LISTEN_PID, LISTEN_FDS. */
   for (i = 0; environ[i] != NULL; ++i) {
-    if (strncmp (environ[i], "LISTEN_PID=", PREFIX_LENGTH) != 0
&&
-        strncmp (environ[i], "LISTEN_FDS=", PREFIX_LENGTH) != 0) {
+    if (strncmp (environ[i], "LISTEN_PID=", strlen
("LISTEN_PID=")) != 0 &&
+        strncmp (environ[i], "LISTEN_FDS=", strlen
("LISTEN_FDS=")) != 0) {
       char *copy = strdup (environ[i]);
       if (copy == NULL)
         goto err;
@@ -194,7 +187,7 @@  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);
+    strcpy (&env.ptr[0][strlen ("LISTEN_FDS=")], v);
 
     /* Restore SIGPIPE back to SIG_DFL. */
     signal (SIGPIPE, SIG_DFL);
-- 
2.39.0

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

diff --git a/.gitignore b/.gitignore
index f42737131d..90eeadb793 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-checked-overflow
 /common/include/test-ispowerof2
 /common/utils/test-human-size
diff --git a/common/include/Makefile.am b/common/include/Makefile.am
index fa2633c25a..04553a367d 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 \
@@ -35,6 +36,7 @@ EXTRA_DIST = \
 
 TESTS = \
 	test-array-size \
+	test-ascii-ctype \
 	test-checked-overflow \
 	test-ispowerof2 \
 	$(NULL)
@@ -44,6 +46,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_checked_overflow_SOURCES = test-checked-overflow.c checked-overflow.h
 test_checked_overflow_CPPFLAGS = -I$(srcdir)
 test_checked_overflow_CFLAGS = $(WARNINGS_CFLAGS)
diff --git a/common/include/ascii-ctype.h b/common/include/ascii-ctype.h
new file mode 100644
index 0000000000..a55dee11b8
--- /dev/null
+++ b/common/include/ascii-ctype.h
@@ -0,0 +1,75 @@
+/* nbdkit
+ * Copyright (C) 2013-2020 Red Hat Inc.
+ *
+ * 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/test-ascii-ctype.c
b/common/include/test-ascii-ctype.c
new file mode 100644
index 0000000000..4fbb0259f8
--- /dev/null
+++ b/common/include/test-ascii-ctype.c
@@ -0,0 +1,88 @@
+/* nbdkit
+ * Copyright (C) 2020 Red Hat Inc.
+ *
+ * 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);
+}
-- 
2.39.0

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.
---
 generator/API.ml | 49 ++++++++++++++++++++++++++++++++++++++++++
 lib/handle.c     | 56 ++++++++++++++++++++++++++++++++++++++++++++++++
 lib/internal.h   |  1 +
 3 files changed, 106 insertions(+)

diff --git a/generator/API.ml b/generator/API.ml
index 25a612a2e8..08fc80960b 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 @@ let first_version    "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 4a186f8fa9..96c8b1f2b1 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
@@ -161,6 +162,7 @@ nbd_close (struct nbd_handle *h)
     waitpid (h->pid, NULL, 0);
 
   free (h->export_name);
+  free (h->sa_name);
   free (h->tls_certificates);
   free (h->tls_username);
   free (h->tls_psk_file);
@@ -200,6 +202,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->sa_name);
+    h->sa_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->sa_name);
+  h->sa_name = new_name;
+  return 0;
+}
+
+char *
+nbd_unlocked_get_socket_activation_name (struct nbd_handle *h)
+{
+  char *copy = strdup (h->sa_name ? h->sa_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)
 {
diff --git a/lib/internal.h b/lib/internal.h
index bbbd26393f..19d7f0af60 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 *sa_name;                /* Socket activation name, can be NULL. */
 
   /* TLS settings. */
   int tls;                      /* 0 = disable, 1 = enable, 2 = require */
-- 
2.39.0

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.
---
 generator/states-connect-socket-activation.c | 35 +++++++++++++++-----
 1 file changed, 26 insertions(+), 9 deletions(-)

diff --git a/generator/states-connect-socket-activation.c
b/generator/states-connect-socket-activation.c
index 24544018fb..2ff191bb9f 100644
--- a/generator/states-connect-socket-activation.c
+++ b/generator/states-connect-socket-activation.c
@@ -22,6 +22,7 @@
 
 #include <stdio.h>
 #include <stdlib.h>
+#include <stdbool.h>
 #include <string.h>
 #include <unistd.h>
 #include <errno.h>
@@ -38,20 +39,27 @@ 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".
+ * any existing LISTEN_PID, LISTEN_FDS or LISTEN_FDNAAMES, and
+ * replaces them with new variables.
+ *
+ * env[0] is "LISTEN_PID=..." which is filled in by CONNECT_SA.START
+ *
+ * env[1] is "LISTEN_FDS=1"
+ *
+ * env[2] (if used) is "LISTEN_FDNAMES=" + h->sa_name
  */
 static int
-prepare_socket_activation_environment (string_vector *env)
+prepare_socket_activation_environment (struct nbd_handle *h,
+                                       string_vector *env)
 {
   char *p;
   size_t i;
+  const bool using_name = h->sa_name != NULL;
 
   assert (env->len == 0);
 
-  /* Reserve slots env[0]..env[1] */
-  if (string_vector_reserve (env, 2) == -1)
+  /* Reserve slots in env. */
+  if (string_vector_reserve (env, using_name ? 3 : 2) == -1)
     goto err;
   p = strdup ("LISTEN_PID=xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx");
   if (p == NULL)
@@ -61,11 +69,20 @@ prepare_socket_activation_environment (string_vector *env)
   if (p == NULL)
     goto err;
   string_vector_append (env, p);
+  if (using_name) {
+    if (asprintf (&p, "LISTEN_FDNAMES=%s", h->sa_name) == -1)
+      goto err;
+    string_vector_append (env, p);
+  }
 
-  /* Append the current environment, but remove LISTEN_PID, LISTEN_FDS. */
+  /* Append the current environment, but remove the special
+   * environment variables.
+   */
   for (i = 0; environ[i] != NULL; ++i) {
     if (strncmp (environ[i], "LISTEN_PID=", strlen
("LISTEN_PID=")) != 0 &&
-        strncmp (environ[i], "LISTEN_FDS=", strlen
("LISTEN_FDS=")) != 0) {
+        strncmp (environ[i], "LISTEN_FDS=", strlen
("LISTEN_FDS=")) != 0 &&
+        strncmp (environ[i], "LISTEN_FDNAMES=",
+                 strlen ("LISTEN_FDNAMES=")) != 0) {
       char *copy = strdup (environ[i]);
       if (copy == NULL)
         goto err;
@@ -148,7 +165,7 @@  CONNECT_SA.START:
     return 0;
   }
 
-  if (prepare_socket_activation_environment (&env) == -1) {
+  if (prepare_socket_activation_environment (h, &env) == -1) {
     SET_NEXT_STATE (%.DEAD);
     close (s);
     return 0;
-- 
2.39.0