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

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 "").

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.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

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