[Libguestfs] [PATCH libnbd v2 2/2] api: Implement local command with systemd socket activation.

[Richard W.M. Jones]

On Fri, Nov 15, 2019 at 09:43:28AM -0600, Eric Blake wrote:
> On 10/1/19 9:11 AM, Richard W.M. Jones wrote:
> >On Tue, Oct 01, 2019 at 08:24:33AM -0500, Eric Blake wrote:
> >>>+#else /* !HAVE_EXECVPE */
> >>>+  SET_NEXT_STATE (%.DEAD)
> >>>+  set_error (ENOTSUP, "platform does not support socket activation");
> >>>+  return 0;
> >>>+#endif
> >>
> >>We probably ought to add a matching nbd_supports_socket_activation()
> >>feature function.
> >>
> >>Or, it would be possible to create a fallback for execvpe() on
> >>platforms that lack it by using execlpe() and our own path-walker
> >>utility function.  Can be done as a followup patch.  If we do that,
> >>then the mere presence of LIBNBD_HAVE_NBD_CONNECT_SA is witness
> >>enough of the functionality, rather than needing a runtime probe.
> >
> >I'm hoping we will find the time to write a replacement execvpe so
> >that we can implement this on all platforms.  That way we can avoid
> >having a redundant nbd_supports_socket_activation() call that (in
> >future) always returns true.
> 
> In fact, I just realized that it's quite easy to replace execvpe() -
> remember, POSIX says that execvp() sets the environment of the new
> process to the contents of 'environ', so all we really have to do is
> assign to environ after fork()ing.
> 
> From 3a019cb65329f290f5be71219b2acb1c8b687a1a Mon Sep 17 00:00:00 2001
> From: Eric Blake <eblake at redhat.com>
> Date: Fri, 15 Nov 2019 09:22:11 -0600
> Subject: [libnbd PATCH] states: Use execvp instead of execvpe
> 
> execvpe is a handy GNU extension, but it is trivial to fall back to
> the POSIX-mandated execvp by manually assigning to environ.  This
> allows socket activation to build on BSD systems.
> ---
>  configure.ac                                 |  4 ----
>  generator/states-connect-socket-activation.c | 16 +++++-----------
>  2 files changed, 5 insertions(+), 15 deletions(-)
> 
> diff --git a/configure.ac b/configure.ac
> index 2dbe97d..9bd6f73 100644
> --- a/configure.ac
> +++ b/configure.ac
> @@ -79,10 +79,6 @@ AC_CHECK_HEADERS([\
> 
>  AC_CHECK_HEADERS([linux/vm_sockets.h], [], [], [#include <sys/socket.h>])
> 
> -dnl Check for functions, all optional.
> -AC_CHECK_FUNCS([\
> -    execvpe])
> -
>  dnl Check for sys_errlist (optional).
>  AC_MSG_CHECKING([for sys_errlist])
>  AC_TRY_LINK([], [extern int sys_errlist; char *p = &sys_errlist;], [
> diff --git a/generator/states-connect-socket-activation.c
> b/generator/states-connect-socket-activation.c
> index 243ba36..ee08dff 100644
> --- a/generator/states-connect-socket-activation.c
> +++ b/generator/states-connect-socket-activation.c
> @@ -36,7 +36,9 @@
>  /* == strlen ("LISTEN_PID=") | strlen ("LISTEN_FDS=") */
>  #define PREFIX_LENGTH 11
> 
> -/* Prepare environment for calling execvpe when doing systemd socket
> +extern char **environ;
> +
> +/* Prepare environment for calling execvp when doing systemd socket
>   * activation.  Takes the current environment and copies it.  Removes
>   * any existing LISTEN_PID or LISTEN_FDS and replaces them with new
>   * variables.  env[0] is "LISTEN_PID=..." which is filled in by
> @@ -45,7 +47,6 @@
>  static char **
>  prepare_socket_activation_environment (void)
>  {
> -  extern char **environ;
>    char **env = NULL;
>    char *p0 = NULL, *p1 = NULL;
>    size_t i, len;
> @@ -97,7 +98,6 @@ prepare_socket_activation_environment (void)
> 
>  STATE_MACHINE {
>   CONNECT_SA.START:
> -#ifdef HAVE_EXECVPE
>    int s;
>    struct sockaddr_un addr;
>    char **env;
> @@ -200,7 +200,8 @@ STATE_MACHINE {
>      /* Restore SIGPIPE back to SIG_DFL. */
>      signal (SIGPIPE, SIG_DFL);
> 
> -    execvpe (h->argv[0], h->argv, env);
> +    environ = env;
> +    execvp (h->argv[0], h->argv);
>      nbd_internal_fork_safe_perror (h->argv[0]);
>      if (errno == ENOENT)
>        _exit (127);
> @@ -217,11 +218,4 @@ STATE_MACHINE {
>    memcpy (&h->connaddr, &addr, h->connaddrlen);
>    SET_NEXT_STATE (%^CONNECT.START);
>    return 0;
> -
> -#else /* !HAVE_EXECVPE */
> -  SET_NEXT_STATE (%.DEAD);
> -  set_error (ENOTSUP, "platform does not support socket activation");
> -  return 0;
> -#endif
> -
>  } /* END STATE MACHINE */

ACK thanks.

Be interesting if the *BSDs support systemd socket activation (in
qemu-nbd or elsewhere).  I'll have to check later.  Of course they
don't have systemd, but also socket activation is not really tied to
systemd.

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-p2v converts physical machines to virtual machines.  Boot with a
live CD or over the network (PXE) and turn machines into KVM guests.
http://libguestfs.org/virt-v2v