[Libguestfs] [libnbd PATCH v3 19/19] socket activation: set LISTEN_FDNAMES

[Laszlo Ersek]

On 3/23/23 20:27, Eric Blake wrote:
> On Thu, Mar 23, 2023 at 01:10:16PM +0100, Laszlo Ersek 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.
>>
>> https://lists.nongnu.org/archive/html/qemu-devel/2023-01/msg06114.html
>>
>> [Original commit message and upstream discussion reference by Rich Jones;
>> at
>> <https://listman.redhat.com/archives/libguestfs/2023-January/030558.html>
>> / msgid <20230130225521.1771496-5-rjones at redhat.com>.]
>>
>> Signed-off-by: Laszlo Ersek <lersek at redhat.com>
>> Reviewed-by: Richard W.M. Jones <rjones at redhat.com>
>> ---
>>
>
>> @@ -245,6 +245,9 @@  CONNECT_SA.START:
>>                   "LISTEN_PID=", "xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx", &pid_ofs);
>>    SACT_VAR_PUSH (sact_var, &num_vars,
>>                   "LISTEN_FDS=", "1", NULL);
>> +  if (h->sact_name != NULL)
>> +    SACT_VAR_PUSH (sact_var, &num_vars,
>> +                   "LISTEN_FDNAMES=", h->sact_name, NULL);
>>    if (prepare_socket_activation_environment (&env, sact_var, num_vars) == -1)
>
> If I'm reading this correctly, this does wipe an inherited
> LISTEN_FDNAMES from the environment in the case where the application
> linked with libnbd started life with a (different) socket activation,
> but now the user wants to connect to an nbd server without setting a
> name (default usage, or explicitly requested a name of "").

Good observation; this is a functionality gap that goes back to v1 of
this patch. (I've not investigated the specifics of systemd socket
activation before; but see below.)

> Put another way, SACT_VAR_PUSH as written appears to be only additive
> for replacement purposes (if I pushed a variable, I intend to override
> it in the child process, so don't copy it from environ if one was
> previously there), but not effective for deletion purposes (I don't
> intend to set the variable, but if it is already set in environ, I
> want it omitted in the child's copy).
>
> Is there a way to rework this so that you can pass NULL as the fourth
> parameter as an indication of an unset request (vs. "" when you want
> it set to the empty string)?  At which point, you would drop the 'if
> (h->sact_name != NULL)', and just blindly use SACT_VAR_PUSH(,
> h->sact_name,).  That has ripple effects earlier in the series to
> support those semantics.

For understanding your point, I have had to read up on systemd socket
activation:

  https://www.freedesktop.org/software/systemd/man/sd_listen_fds.html

Let me quote a part:

> Under specific conditions, the following automatic file descriptor names are returned:
>
>   [...]
>   "unknown" -- The process received no name for the specific file
>                descriptor from the service manager.
>   [...]

I've also checked the implementation of sd_listen_fds_with_names():

  https://github.com/systemd/systemd/blob/main/src/libsystemd/sd-daemon/sd-daemon.c

Let me quote the function:

> _public_ int sd_listen_fds_with_names(int unset_environment, char ***names) {
>         _cleanup_strv_free_ char **l = NULL;
>         bool have_names;
>         int n_names = 0, n_fds;
>         const char *e;
>         int r;
>
>         if (!names)
>                 return sd_listen_fds(unset_environment);
>
>         e = getenv("LISTEN_FDNAMES");
>         if (e) {
>                 n_names = strv_split_full(&l, e, ":", EXTRACT_DONT_COALESCE_SEPARATORS);
>                 if (n_names < 0) {
>                         unsetenv_all(unset_environment);
>                         return n_names;
>                 }
>
>                 have_names = true;
>         } else
>                 have_names = false;
>
>         n_fds = sd_listen_fds(unset_environment);
>         if (n_fds <= 0)
>                 return n_fds;
>
>         if (have_names) {
>                 if (n_names != n_fds)
>                         return -EINVAL;
>         } else {
>                 r = strv_extend_n(&l, "unknown", n_fds);
>                 if (r < 0)
>                         return r;
>         }
>
>         *names = TAKE_PTR(l);
>
>         return n_fds;
> }

Based on those:

(1) If we pass LISTEN_PID=xxx and LISTEN_FDS=1, and don't pass
    LISTEN_FDNAMES at all, then sd_listen_fds_with_names() will function
    in the nbd server properly, and will return a "names" array with a
    single non-NULL element "unknown", and a terminating null pointer.
    The "unknown" value is specified behavior that socket-activated
    services can rely upon.

(2) If we pass LISTEN_PID=xxx and LISTEN_FDS=1, and just "pass through"
    an *inherited* LISTEN_FDNAMES variable, then it will (in the general
    case) confuse the nbd server that we start. Namely, if
    LISTEN_FDNAMES has multiple colon-separated elements (more than 1),
    or is the empty string (= 0 elements), then
    sd_listen_fds_with_names() in the nbd server will fail the "n_names
    != n_fds" check, and return (-EINVAL). If LISTEN_FDNAMES happens to
    have one element, then sd_listen_fds_with_names() will succeed, but
    the returned name will confuse the nbd server.

(3) If we pass LISTEN_PID=xxx and LISTEN_FDS=1, and rework the logic in
    this patch to pass LISTEN_FDNAMES="" in case "h->sact_name" is NULL,
    then sd_listen_fds_with_names() will succeed in the nbd server, but
    the returned single name ("") will most likely confuse it.

(4) If we pass LISTEN_PID=xxx and LISTEN_FDS=1, and rework the logic in
    this patch to pass LISTEN_FDNAMES="unknown" in case "h->sact_name"
    is NULL, then sd_listen_fds_with_names() will succeed in the nbd
    server, and the single returned name ("unknown") will merge into
    case (1), i.e., as if we had not passed (or had removed)
    LISTEN_FDNAMES in the environment.

Therefore I propose that we implement (4). We're already populating the
LISTEN_* variables ourselves (that is, not relying on systemd library or
daemon logic to fill them in). I see nothing wrong with setting
LISTEN_FDNAMES="unknown" ourselves; again that value is publicly
specified behavior.

Case (4) also appears consistent with repeated calls to
sd_listen_fds_with_names(). If "unset_environment" is set to nonzero in
one of those calls, then further calls will see the internal
sd_listen_fds() call return 0, and behave as expected. Whereas if
"unset_environment" is never set to zero in those repeated calls, then
"unknown" will continue to be returned from LISTEN_FDNAMES (as if via
case (1)).

Under case (4), we should also update the API documentation in the
previous patch ("generator: Add APIs to get/set the socket activation
socket name"):

> +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.";

we can say there, 'then the name "unknown" will be seen by the server'.

Thanks for the careful review!
Laszlo