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