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(a)redhat.com&gt; Reviewed-by: Richard W.M. Jones <rjones(a)redhat.com&gt; Reviewed-by: Eric Blake <eblake(a)redhat.com&gt; --- 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(a)redhat.com&gt; 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(a)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/n6lzl7ii5m4hu77n7rrzm3uih3tnx457e4y67rghki34b...;. - Documented that we are going to pass "unknown" if "socket_name" is set to the empty string <http://mid.mail-archive.com/oqwjnjvq4phqr76yum6zo5erfrm3tvmyewr5nxru3oxkl...] Signed-off-by: Laszlo Ersek <lersek(a)redhat.com&gt; Reviewed-by: Eric Blake <eblake(a)redhat.com&gt; --- 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(a)redhat.com&gt; --- 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;