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-rjo...@redhat.com>.] >> >> Signed-off-by: Laszlo Ersek <ler...@redhat.com> >> Reviewed-by: Richard W.M. Jones <rjo...@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 _______________________________________________ Libguestfs mailing list Libguestfs@redhat.com https://listman.redhat.com/mailman/listinfo/libguestfs