Re: [Libguestfs] [PATCH libnbd v2 4/4] generator/states-connect-socket-activation.c: Set LISTEN_FDNAMES
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