Richard W.M. Jones
2023-Jan-30 22:55 UTC
[Libguestfs] [PATCH libnbd v2 4/4] generator/states-connect-socket-activation.c: Set LISTEN_FDNAMES
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
Eric Blake
2023-Jan-31 16:38 UTC
[Libguestfs] [PATCH libnbd v2 4/4] generator/states-connect-socket-activation.c: Set LISTEN_FDNAMES
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
Laszlo Ersek
2023-Feb-01 10:22 UTC
[Libguestfs] [PATCH libnbd v2 4/4] generator/states-connect-socket-activation.c: Set LISTEN_FDNAMES
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 at redhat.com>