[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
On 1/30/23 23:55, Richard W.M. Jones wrote:
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);
I'd propose a more comprehensive refactoring here, see under my v1 comments. Laszlo
--- .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
On 1/30/23 23:55, Richard W.M. Jones wrote:
--- .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(+)
I suggest noting in the commit message the commit hash of nbdkit at which the files are taken. Reviewed-by: Laszlo Ersek <lersek@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. --- 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
On Mon, Jan 30, 2023 at 10:55:20PM +0000, Richard W.M. Jones wrote:
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.
This creates an implicit client-side stateful API: to pass socket names, you must call two APIs in the correct sequence: nbd_set_socket_activation_name (h, "foo"); nbd_connect_systemd_socket_activation (h, argv); I can live with that design, but I recall Laszlo questioning in the past if we always need to force this stateful design onto clients, or if it would be easier to instead add a alternative API that takes the socket name as an additional parameter, in one shot: nbd_connect_systemd_named_socket_activation (h, "foo", argv);
+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");
I don't mind keeping us strict to start with and loosening it later if someone demonstrates why it is needed. But systemd documents a larger set of possible names: https://www.freedesktop.org/software/systemd/man/sd_pid_notify_with_fds.html FDNAME=… When used in combination with FDSTORE=1, specifies a name for the submitted file descriptors. When used with FDSTOREREMOVE=1, specifies the name for the file descriptors to remove. This name is passed to the service during activation, and may be queried using sd_listen_fds_with_names(3). File descriptors submitted without this field set, will implicitly get the name "stored" assigned. Note that, if multiple file descriptors are submitted at once, the specified name will be assigned to all of them. In order to assign different names to submitted file descriptors, submit them in separate invocations of sd_pid_notify_with_fds(). The name may consist of arbitrary ASCII characters except control characters or ":". It may not be longer than 255 characters. If a submitted name does not follow these restrictions, it is ignored. -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org
On Tue, Jan 31, 2023 at 10:29:00AM -0600, Eric Blake wrote:
On Mon, Jan 30, 2023 at 10:55:20PM +0000, Richard W.M. Jones wrote:
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.
This creates an implicit client-side stateful API: to pass socket names, you must call two APIs in the correct sequence:
nbd_set_socket_activation_name (h, "foo"); nbd_connect_systemd_socket_activation (h, argv);
I can live with that design, but I recall Laszlo questioning in the past if we always need to force this stateful design onto clients, or if it would be easier to instead add a alternative API that takes the socket name as an additional parameter, in one shot:
nbd_connect_systemd_named_socket_activation (h, "foo", argv);
While I'm not totally opposed to this, I would say a few things: - the API is already stateful, even used in the most basic way, eg you must connect before you can do other things - having the state modified by various nbd_set_* calls allows us to easily add more variants, instead of having to create a combinatorial future set of nbd_connect_systemd_*_socket_activation calls So I would lean towards my design. (Also it's the same thing we already do, eg. for opt mode).
+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");
I don't mind keeping us strict to start with and loosening it later if someone demonstrates why it is needed. But systemd documents a larger set of possible names:
https://www.freedesktop.org/software/systemd/man/sd_pid_notify_with_fds.html
FDNAME=…
When used in combination with FDSTORE=1, specifies a name for the submitted file descriptors. When used with FDSTOREREMOVE=1, specifies the name for the file descriptors to remove. This name is passed to the service during activation, and may be queried using sd_listen_fds_with_names(3). File descriptors submitted without this field set, will implicitly get the name "stored" assigned. Note that, if multiple file descriptors are submitted at once, the specified name will be assigned to all of them. In order to assign different names to submitted file descriptors, submit them in separate invocations of sd_pid_notify_with_fds(). The name may consist of arbitrary ASCII characters except control characters or ":". It may not be longer than 255 characters. If a submitted name does not follow these restrictions, it is ignored.
I didn't know about this documentation before. Arbitrary ASCII characters doesn't sound that great though. I'm sure that q-s-d will want its own much more strict limitations, eg. I assume you can't possibly support any characters which are meaningful to JSON / QMP. Any thoughts on that? Rich. -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com libguestfs lets you edit virtual machines. Supports shell scripting, bindings from many languages. http://libguestfs.org
On Tue, Jan 31, 2023 at 05:28:18PM +0000, Richard W.M. Jones wrote:
+ /* Check the proposed name is short and alphanumeric. */ + if (len > 32) { + set_error (ENAMETOOLONG, "socket activation name should be " + "<= 32 characters");
I don't mind keeping us strict to start with and loosening it later if someone demonstrates why it is needed. But systemd documents a larger set of possible names:
https://www.freedesktop.org/software/systemd/man/sd_pid_notify_with_fds.html
FDNAME=…
When used in combination with FDSTORE=1, specifies a name for the submitted file descriptors. When used with FDSTOREREMOVE=1, specifies the name for the file descriptors to remove. This name is passed to the service during activation, and may be queried using sd_listen_fds_with_names(3). File descriptors submitted without this field set, will implicitly get the name "stored" assigned. Note that, if multiple file descriptors are submitted at once, the specified name will be assigned to all of them. In order to assign different names to submitted file descriptors, submit them in separate invocations of sd_pid_notify_with_fds(). The name may consist of arbitrary ASCII characters except control characters or ":". It may not be longer than 255 characters. If a submitted name does not follow these restrictions, it is ignored.
I didn't know about this documentation before.
I only found it this morning.
Arbitrary ASCII characters doesn't sound that great though. I'm sure that q-s-d will want its own much more strict limitations, eg. I assume you can't possibly support any characters which are meaningful to JSON / QMP. Any thoughts on that?
You have a strong point there. Just because systemd allows it doesn't make it wise; I'm a big fan of "it's easier to resrict now and loosen later when we see the need", as being easier than "let's try and be as loose as we can now, then regret it later when we find it was a security hole". Limiting to alphanumeric and 32 bytes until someone proves they have a use case for needing more than that works for me. -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org
On 1/31/23 19:03, Eric Blake wrote:
On Tue, Jan 31, 2023 at 05:28:18PM +0000, Richard W.M. Jones wrote:
+ /* Check the proposed name is short and alphanumeric. */ + if (len > 32) { + set_error (ENAMETOOLONG, "socket activation name should be " + "<= 32 characters");
I don't mind keeping us strict to start with and loosening it later if someone demonstrates why it is needed. But systemd documents a larger set of possible names:
https://www.freedesktop.org/software/systemd/man/sd_pid_notify_with_fds.html
FDNAME=…
When used in combination with FDSTORE=1, specifies a name for the submitted file descriptors. When used with FDSTOREREMOVE=1, specifies the name for the file descriptors to remove. This name is passed to the service during activation, and may be queried using sd_listen_fds_with_names(3). File descriptors submitted without this field set, will implicitly get the name "stored" assigned. Note that, if multiple file descriptors are submitted at once, the specified name will be assigned to all of them. In order to assign different names to submitted file descriptors, submit them in separate invocations of sd_pid_notify_with_fds(). The name may consist of arbitrary ASCII characters except control characters or ":". It may not be longer than 255 characters. If a submitted name does not follow these restrictions, it is ignored.
I didn't know about this documentation before.
I only found it this morning.
Arbitrary ASCII characters doesn't sound that great though. I'm sure that q-s-d will want its own much more strict limitations, eg. I assume you can't possibly support any characters which are meaningful to JSON / QMP. Any thoughts on that?
You have a strong point there. Just because systemd allows it doesn't make it wise; I'm a big fan of "it's easier to resrict now and loosen later when we see the need", as being easier than "let's try and be as loose as we can now, then regret it later when we find it was a security hole". Limiting to alphanumeric and 32 bytes until someone proves they have a use case for needing more than that works for me.
I agree! Laszlo
On 1/31/23 18:28, Richard W.M. Jones wrote:
On Tue, Jan 31, 2023 at 10:29:00AM -0600, Eric Blake wrote:
On Mon, Jan 30, 2023 at 10:55:20PM +0000, Richard W.M. Jones wrote:
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.
This creates an implicit client-side stateful API: to pass socket names, you must call two APIs in the correct sequence:
nbd_set_socket_activation_name (h, "foo"); nbd_connect_systemd_socket_activation (h, argv);
I can live with that design, but I recall Laszlo questioning in the past if we always need to force this stateful design onto clients, or if it would be easier to instead add a alternative API that takes the socket name as an additional parameter, in one shot:
nbd_connect_systemd_named_socket_activation (h, "foo", argv);
While I'm not totally opposed to this, I would say a few things:
- the API is already stateful, even used in the most basic way, eg you must connect before you can do other things
This subthread is quite funny to me because I have mostly accepted the reasoning behind the API being such as it is :) Thank you, Eric, for remembering my qualms.
- having the state modified by various nbd_set_* calls allows us to easily add more variants, instead of having to create a combinatorial future set of nbd_connect_systemd_*_socket_activation calls
So I would lean towards my design. (Also it's the same thing we already do, eg. for opt mode).
"Combinatorial explosion" was certainly my thought here, too! I guess the ship has sailed; upon seeing this patch, I only said, "hrmpf, another knob, okay". It's consistent with the status quo. Laszlo
+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");
I don't mind keeping us strict to start with and loosening it later if someone demonstrates why it is needed. But systemd documents a larger set of possible names:
https://www.freedesktop.org/software/systemd/man/sd_pid_notify_with_fds.html
FDNAME=…
When used in combination with FDSTORE=1, specifies a name for the submitted file descriptors. When used with FDSTOREREMOVE=1, specifies the name for the file descriptors to remove. This name is passed to the service during activation, and may be queried using sd_listen_fds_with_names(3). File descriptors submitted without this field set, will implicitly get the name "stored" assigned. Note that, if multiple file descriptors are submitted at once, the specified name will be assigned to all of them. In order to assign different names to submitted file descriptors, submit them in separate invocations of sd_pid_notify_with_fds(). The name may consist of arbitrary ASCII characters except control characters or ":". It may not be longer than 255 characters. If a submitted name does not follow these restrictions, it is ignored.
I didn't know about this documentation before.
Arbitrary ASCII characters doesn't sound that great though. I'm sure that q-s-d will want its own much more strict limitations, eg. I assume you can't possibly support any characters which are meaningful to JSON / QMP. Any thoughts on that?
Rich.
On 1/30/23 23:55, Richard W.M. Jones wrote:
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 */
"lib/handle.c" includes <signal.h>, and <signal.h> reserves symbols starting with the prefix "sa_": https://pubs.opengroup.org/onlinepubs/9699919799/functions/V2_chap02.html#ta... While the fields of unions and structures live in a separate namespace, that's no protection from macros. Otherwise the patch looks OK to me. (I'm noticing we already have fields called "sa_tmpdir" and "sa_sockpath"; that's not ideal, although it may not matter in practice.) Reviewed-by: Laszlo Ersek <lersek@redhat.com> Laszlo
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
On Mon, Jan 30, 2023 at 10:55:21PM +0000, Richard W.M. Jones wrote:
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(-)
+ * + * env[2] (if used) is "LISTEN_FDNAMES=" + h->sa_name
@@ -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) {
Hmm. Even if we _don't_ expose the ability to set LISTEN_FDNAMES to users, we should probably be stripping it from the environment (without replacement), rather than just stripping the other two LISTEN_* variables. Which might be worth doing in a separate patch, in case it has to be backported across different versions of libnbd. But overall, I agree with exposing the ability for the client to programatically set the name they want, whether by this series or by my idea of an alternative API that takes the socket name alongside the argv; and whether we keep to our 32-byte [[:alnum:]] limit or instead allow for a larger name up to 255 bytes in the regex range notation by ASCII byte value [\x20-\x39\x41-\x7e] (aka [ -9;-~], or [^\x00-\x1f\x7f-\xff]). -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org
On 1/31/23 17:38, Eric Blake wrote:
On Mon, Jan 30, 2023 at 10:55:21PM +0000, Richard W.M. Jones wrote:
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(-)
+ * + * env[2] (if used) is "LISTEN_FDNAMES=" + h->sa_name
@@ -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) {
Hmm. Even if we _don't_ expose the ability to set LISTEN_FDNAMES to users, we should probably be stripping it from the environment (without replacement), rather than just stripping the other two LISTEN_* variables.
I'm unsure. Now, what we strip, mirrors what we provide. That's sustainable because we revise the set of affected variables *whenever* we need to add a new variable, in relation to socket activation. If we start stripping more than what we provide ourselves, it becomes a game of tag with systemd. I don't know how stable the socket activation interface is, but I presume further environment variables could be introduced by systemd. We'd still be catching up the same. It could work if systemd commandeered an environment variable *prefix* (aka namespace) for socket activation purposes, and then we could filter out everything in one go. Unfortunately, LISTEN_ is just too generic for that. (IOW, I can imagine that systemd will be introducing a few more LISTEN_ variables in the future, but we still can't filter out everything LISTEN_*. SYSTEMD_SOCKET_ACTIVATION_ would have been a much better choice for systemd. Hindsight is 20/20.)
Which might be worth doing in a separate patch, in case it has to be backported across different versions of libnbd.
But overall, I agree with exposing the ability for the client to programatically set the name they want, whether by this series or by my idea of an alternative API that takes the socket name alongside the argv; and whether we keep to our 32-byte [[:alnum:]] limit or instead allow for a larger name up to 255 bytes in the regex range notation by ASCII byte value [\x20-\x39\x41-\x7e] (aka [ -9;-~], or [^\x00-\x1f\x7f-\xff]).
Laszlo
On 1/30/23 23:55, Richard W.M. Jones wrote:
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;
As long as we don't use a table+loop for the special variables, this seems OK to me. (For patch#1, I meant to add: wherever we know string_vector_append() can't fail, due to the reservation at the top, adding an assert() could improve readability; the reservation can get visually quite far from the repeated string_vector_append() calls.) Reviewed-by: Laszlo Ersek <lersek@redhat.com>
participants (3)
-
Eric Blake -
Laszlo Ersek -
Richard W.M. Jones