[Libguestfs] [PATCH libnbd v2 4/4] generator/states-connect-socket-activation.c: Set LISTEN_FDNAMES

[Laszlo Ersek]

On 1/30/23 23:55, 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(-)
> 
> diff --git a/generator/states-connect-socket-activation.c b/generator/states-connect-socket-activation.c
> index 24544018fb..2ff191bb9f 100644
> --- a/generator/states-connect-socket-activation.c
> +++ b/generator/states-connect-socket-activation.c
> @@ -22,6 +22,7 @@
>  
>  #include <stdio.h>
>  #include <stdlib.h>
> +#include <stdbool.h>
>  #include <string.h>
>  #include <unistd.h>
>  #include <errno.h>
> @@ -38,20 +39,27 @@ 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
> - * CONNECT_SA.START, and env[1] is "LISTEN_FDS=1".
> + * any existing LISTEN_PID, LISTEN_FDS or LISTEN_FDNAAMES, and
> + * replaces them with new variables.
> + *
> + * env[0] is "LISTEN_PID=..." which is filled in by CONNECT_SA.START
> + *
> + * env[1] is "LISTEN_FDS=1"
> + *
> + * env[2] (if used) is "LISTEN_FDNAMES=" + h->sa_name
>   */
>  static int
> -prepare_socket_activation_environment (string_vector *env)
> +prepare_socket_activation_environment (struct nbd_handle *h,
> +                                       string_vector *env)
>  {
>    char *p;
>    size_t i;
> +  const bool using_name = h->sa_name != NULL;
>  
>    assert (env->len == 0);
>  
> -  /* Reserve slots env[0]..env[1] */
> -  if (string_vector_reserve (env, 2) == -1)
> +  /* Reserve slots in env. */
> +  if (string_vector_reserve (env, using_name ? 3 : 2) == -1)
>      goto err;
>    p = strdup ("LISTEN_PID=xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx");
>    if (p == NULL)
> @@ -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) {
>        char *copy = strdup (environ[i]);
>        if (copy == NULL)
>          goto err;
> @@ -148,7 +165,7 @@  CONNECT_SA.START:
>      return 0;
>    }
>  
> -  if (prepare_socket_activation_environment (&env) == -1) {
> +  if (prepare_socket_activation_environment (h, &env) == -1) {
>      SET_NEXT_STATE (%.DEAD);
>      close (s);
>      return 0;

As long as we don't use a table+loop for the special variables, this
seems OK to me.

(For patch#1, I meant to add: wherever we know string_vector_append()
can't fail, due to the reservation at the top, adding an assert() could
improve readability; the reservation can get visually quite far from the
repeated string_vector_append() calls.)

Reviewed-by: Laszlo Ersek <lersek at redhat.com>