Libguestfs - Jan 2023 - [PATCH libnbd v2 3/4] generator: Add APIs to get/set the socket activation socket name

To allow us to name the socket passed down to the NBD server when
calling nbd_connect_systemd_socket_activation(3), we need to add the
field to the handle and add access functions.
---
 generator/API.ml | 49 ++++++++++++++++++++++++++++++++++++++++++
 lib/handle.c     | 56 ++++++++++++++++++++++++++++++++++++++++++++++++
 lib/internal.h   |  1 +
 3 files changed, 106 insertions(+)

diff --git a/generator/API.ml b/generator/API.ml
index 25a612a2e8..08fc80960b 100644
--- a/generator/API.ml
+++ b/generator/API.ml
@@ -2036,15 +2036,62 @@   "connect_systemd_socket_activation", {
 
 When the NBD handle is closed the server subprocess
 is killed.
+
+=head3 Socket name
+
+The socket activation protocol lets you optionally give
+the socket a name.  If used, the name is passed to the
+NBD server using the C<LISTEN_FDNAMES> environment
+variable.  To provide a socket name, call
+L<nbd_set_socket_activation_name(3)> before calling
+the connect function.
 " ^ blocking_connect_call_description;
     see_also = [Link "aio_connect_systemd_socket_activation";
                 Link "connect_command"; Link
"kill_subprocess";
                 Link "set_opt_mode";
+                Link "set_socket_activation_name";
+                Link "get_socket_activation_name";
                 ExternalLink ("qemu-nbd", 1);
                 URLLink
"http://0pointer.de/blog/projects/socket-activation.html"];
     example = Some "examples/open-qcow2.c";
   };
 
+  "set_socket_activation_name", {
+    default_call with
+    args = [ String "socket_name" ]; ret = RErr;
+    shortdesc = "set the socket activation name";
+    longdesc = "\
+When running an NBD server using
+L<nbd_connect_systemd_socket_activation(3)> you can optionally
+name the socket.  Call this function before connecting to the
+server.
+
+Some servers such as L<qemu-storage-daemon(1)>
+can use this information to associate the socket with a name
+used on the command line, but most servers will ignore it.
+The name is passed through the C<LISTEN_FDNAMES> environment
+variable.
+
+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.";
+    see_also = [Link "connect_systemd_socket_activation";
+                Link "get_socket_activation_name"];
+  };
+
+  "get_socket_activation_name", {
+    default_call with
+    args = []; ret = RString;
+    shortdesc = "get the socket activation name";
+    longdesc = "\
+Return the socket name used when you call
+L<nbd_connect_systemd_socket_activation(3)> on the same
+handle.  By default this will return the empty string
+meaning that no name is passed to the server.";
+    see_also = [Link "connect_systemd_socket_activation";
+                Link "set_socket_activation_name"];
+  };
+
   "is_read_only", {
     default_call with
     args = []; ret = RBool;
@@ -3844,6 +3891,8 @@ let first_version    "aio_opt_structured_reply",
(1, 16);
   "opt_starttls", (1, 16);
   "aio_opt_starttls", (1, 16);
+  "set_socket_activation_name", (1, 16);
+  "get_socket_activation_name", (1, 16);
 
   (* These calls are proposed for a future version of libnbd, but
    * have not been added to any released version so far.
diff --git a/lib/handle.c b/lib/handle.c
index 4a186f8fa9..96c8b1f2b1 100644
--- a/lib/handle.c
+++ b/lib/handle.c
@@ -28,6 +28,7 @@
 #include <sys/types.h>
 #include <sys/wait.h>
 
+#include "ascii-ctype.h"
 #include "internal.h"
 
 static void
@@ -161,6 +162,7 @@ nbd_close (struct nbd_handle *h)
     waitpid (h->pid, NULL, 0);
 
   free (h->export_name);
+  free (h->sa_name);
   free (h->tls_certificates);
   free (h->tls_username);
   free (h->tls_psk_file);
@@ -200,6 +202,60 @@ nbd_unlocked_get_handle_name (struct nbd_handle *h)
   return copy;
 }
 
+int
+nbd_unlocked_set_socket_activation_name (struct nbd_handle *h,
+                                         const char *name)
+{
+  size_t i, len;
+  char *new_name;
+
+  len = strlen (name);
+
+  /* Setting it to empty string stores NULL in the handle. */
+  if (len == 0) {
+    free (h->sa_name);
+    h->sa_name = NULL;
+    return 0;
+  }
+
+  /* Check the proposed name is short and alphanumeric. */
+  if (len > 32) {
+    set_error (ENAMETOOLONG, "socket activation name should be "
+               "<= 32 characters");
+    return -1;
+  }
+  for (i = 0; i < len; ++i) {
+    if (! ascii_isalnum (name[i])) {
+      set_error (EINVAL, "socket activation name should contain "
+                 "only alphanumeric ASCII characters");
+      return -1;
+    }
+  }
+
+  new_name = strdup (name);
+  if (!new_name) {
+    set_error (errno, "strdup");
+    return -1;
+  }
+
+  free (h->sa_name);
+  h->sa_name = new_name;
+  return 0;
+}
+
+char *
+nbd_unlocked_get_socket_activation_name (struct nbd_handle *h)
+{
+  char *copy = strdup (h->sa_name ? h->sa_name : "");
+
+  if (!copy) {
+    set_error (errno, "strdup");
+    return NULL;
+  }
+
+  return copy;
+}
+
 uintptr_t
 nbd_unlocked_set_private_data (struct nbd_handle *h, uintptr_t data)
 {
diff --git a/lib/internal.h b/lib/internal.h
index bbbd26393f..19d7f0af60 100644
--- a/lib/internal.h
+++ b/lib/internal.h
@@ -101,6 +101,7 @@ struct nbd_handle {
   _Atomic uintptr_t private_data;
 
   char *export_name;            /* Export name, never NULL. */
+  char *sa_name;                /* Socket activation name, can be NULL. */
 
   /* TLS settings. */
   int tls;                      /* 0 = disable, 1 = enable, 2 = require */
-- 
2.39.0

On Mon, Jan 30, 2023 at 10:55:20PM +0000, Richard W.M. Jones
wrote:
> To allow us to name the socket passed down to the NBD server when
> calling nbd_connect_systemd_socket_activation(3), we need to add the
> field to the handle and add access functions.
> ---
>  generator/API.ml | 49 ++++++++++++++++++++++++++++++++++++++++++
>  lib/handle.c     | 56 ++++++++++++++++++++++++++++++++++++++++++++++++
>  lib/internal.h   |  1 +
>  3 files changed, 106 insertions(+)
> 
> diff --git a/generator/API.ml b/generator/API.ml
> index 25a612a2e8..08fc80960b 100644
> --- a/generator/API.ml
> +++ b/generator/API.ml
> @@ -2036,15 +2036,62 @@   "connect_systemd_socket_activation", {
>  
>  When the NBD handle is closed the server subprocess
>  is killed.
> +
> +=head3 Socket name
> +
> +The socket activation protocol lets you optionally give
> +the socket a name.  If used, the name is passed to the
> +NBD server using the C<LISTEN_FDNAMES> environment
> +variable.  To provide a socket name, call
> +L<nbd_set_socket_activation_name(3)> before calling
> +the connect function.
This creates an implicit client-side stateful API: to pass socket
names, you must call two APIs in the correct sequence:

nbd_set_socket_activation_name (h, "foo");
nbd_connect_systemd_socket_activation (h, argv);

I can live with that design, but I recall Laszlo questioning in the
past if we always need to force this stateful design onto clients, or
if it would be easier to instead add a alternative API that takes the
socket name as an additional parameter, in one shot:

nbd_connect_systemd_named_socket_activation (h, "foo", argv);
>  
> +int
> +nbd_unlocked_set_socket_activation_name (struct nbd_handle *h,
> +                                         const char *name)
> +{
> +  size_t i, len;
> +  char *new_name;
> +
> +  len = strlen (name);
> +
> +  /* Setting it to empty string stores NULL in the handle. */
> +  if (len == 0) {
> +    free (h->sa_name);
> +    h->sa_name = NULL;
> +    return 0;
> +  }
> +
> +  /* Check the proposed name is short and alphanumeric. */
> +  if (len > 32) {
> +    set_error (ENAMETOOLONG, "socket activation name should be "
> +               "<= 32 characters");
I don't mind keeping us strict to start with and loosening it later if
someone demonstrates why it is needed.  But systemd documents a larger
set of possible names:

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

FDNAME=?

    When used in combination with FDSTORE=1, specifies a name for the
    submitted file descriptors. When used with FDSTOREREMOVE=1,
    specifies the name for the file descriptors to remove. This name
    is passed to the service during activation, and may be queried
    using sd_listen_fds_with_names(3). File descriptors submitted
    without this field set, will implicitly get the name "stored"
    assigned. Note that, if multiple file descriptors are submitted at
    once, the specified name will be assigned to all of them. In order
    to assign different names to submitted file descriptors, submit
    them in separate invocations of sd_pid_notify_with_fds(). The name
    may consist of arbitrary ASCII characters except control
    characters or ":". It may not be longer than 255 characters. If a
    submitted name does not follow these restrictions, it is ignored.

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

On 1/30/23 23:55, Richard W.M. Jones wrote:
> To allow us to name the socket passed down to the NBD server when
> calling nbd_connect_systemd_socket_activation(3), we need to add the
> field to the handle and add access functions.
> ---
>  generator/API.ml | 49 ++++++++++++++++++++++++++++++++++++++++++
>  lib/handle.c     | 56 ++++++++++++++++++++++++++++++++++++++++++++++++
>  lib/internal.h   |  1 +
>  3 files changed, 106 insertions(+)
> 
> diff --git a/generator/API.ml b/generator/API.ml
> index 25a612a2e8..08fc80960b 100644
> --- a/generator/API.ml
> +++ b/generator/API.ml
> @@ -2036,15 +2036,62 @@   "connect_systemd_socket_activation", {
>  
>  When the NBD handle is closed the server subprocess
>  is killed.
> +
> +=head3 Socket name
> +
> +The socket activation protocol lets you optionally give
> +the socket a name.  If used, the name is passed to the
> +NBD server using the C<LISTEN_FDNAMES> environment
> +variable.  To provide a socket name, call
> +L<nbd_set_socket_activation_name(3)> before calling
> +the connect function.
>  " ^ blocking_connect_call_description;
>      see_also = [Link "aio_connect_systemd_socket_activation";
>                  Link "connect_command"; Link
"kill_subprocess";
>                  Link "set_opt_mode";
> +                Link "set_socket_activation_name";
> +                Link "get_socket_activation_name";
>                  ExternalLink ("qemu-nbd", 1);
>                  URLLink
"http://0pointer.de/blog/projects/socket-activation.html"];
>      example = Some "examples/open-qcow2.c";
>    };
>  
> +  "set_socket_activation_name", {
> +    default_call with
> +    args = [ String "socket_name" ]; ret = RErr;
> +    shortdesc = "set the socket activation name";
> +    longdesc = "\
> +When running an NBD server using
> +L<nbd_connect_systemd_socket_activation(3)> you can optionally
> +name the socket.  Call this function before connecting to the
> +server.
> +
> +Some servers such as L<qemu-storage-daemon(1)>
> +can use this information to associate the socket with a name
> +used on the command line, but most servers will ignore it.
> +The name is passed through the C<LISTEN_FDNAMES> environment
> +variable.
> +
> +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.";
> +    see_also = [Link "connect_systemd_socket_activation";
> +                Link "get_socket_activation_name"];
> +  };
> +
> +  "get_socket_activation_name", {
> +    default_call with
> +    args = []; ret = RString;
> +    shortdesc = "get the socket activation name";
> +    longdesc = "\
> +Return the socket name used when you call
> +L<nbd_connect_systemd_socket_activation(3)> on the same
> +handle.  By default this will return the empty string
> +meaning that no name is passed to the server.";
> +    see_also = [Link "connect_systemd_socket_activation";
> +                Link "set_socket_activation_name"];
> +  };
> +
>    "is_read_only", {
>      default_call with
>      args = []; ret = RBool;
> @@ -3844,6 +3891,8 @@ let first_version >   
"aio_opt_structured_reply", (1, 16);
>    "opt_starttls", (1, 16);
>    "aio_opt_starttls", (1, 16);
> +  "set_socket_activation_name", (1, 16);
> +  "get_socket_activation_name", (1, 16);
>  
>    (* These calls are proposed for a future version of libnbd, but
>     * have not been added to any released version so far.
> diff --git a/lib/handle.c b/lib/handle.c
> index 4a186f8fa9..96c8b1f2b1 100644
> --- a/lib/handle.c
> +++ b/lib/handle.c
> @@ -28,6 +28,7 @@
>  #include <sys/types.h>
>  #include <sys/wait.h>
>  
> +#include "ascii-ctype.h"
>  #include "internal.h"
>  
>  static void
> @@ -161,6 +162,7 @@ nbd_close (struct nbd_handle *h)
>      waitpid (h->pid, NULL, 0);
>  
>    free (h->export_name);
> +  free (h->sa_name);
>    free (h->tls_certificates);
>    free (h->tls_username);
>    free (h->tls_psk_file);
> @@ -200,6 +202,60 @@ nbd_unlocked_get_handle_name (struct nbd_handle *h)
>    return copy;
>  }
>  
> +int
> +nbd_unlocked_set_socket_activation_name (struct nbd_handle *h,
> +                                         const char *name)
> +{
> +  size_t i, len;
> +  char *new_name;
> +
> +  len = strlen (name);
> +
> +  /* Setting it to empty string stores NULL in the handle. */
> +  if (len == 0) {
> +    free (h->sa_name);
> +    h->sa_name = NULL;
> +    return 0;
> +  }
> +
> +  /* Check the proposed name is short and alphanumeric. */
> +  if (len > 32) {
> +    set_error (ENAMETOOLONG, "socket activation name should be "
> +               "<= 32 characters");
> +    return -1;
> +  }
> +  for (i = 0; i < len; ++i) {
> +    if (! ascii_isalnum (name[i])) {
> +      set_error (EINVAL, "socket activation name should contain
"
> +                 "only alphanumeric ASCII characters");
> +      return -1;
> +    }
> +  }
> +
> +  new_name = strdup (name);
> +  if (!new_name) {
> +    set_error (errno, "strdup");
> +    return -1;
> +  }
> +
> +  free (h->sa_name);
> +  h->sa_name = new_name;
> +  return 0;
> +}
> +
> +char *
> +nbd_unlocked_get_socket_activation_name (struct nbd_handle *h)
> +{
> +  char *copy = strdup (h->sa_name ? h->sa_name : "");
> +
> +  if (!copy) {
> +    set_error (errno, "strdup");
> +    return NULL;
> +  }
> +
> +  return copy;
> +}
> +
>  uintptr_t
>  nbd_unlocked_set_private_data (struct nbd_handle *h, uintptr_t data)
>  {
> diff --git a/lib/internal.h b/lib/internal.h
> index bbbd26393f..19d7f0af60 100644
> --- a/lib/internal.h
> +++ b/lib/internal.h
> @@ -101,6 +101,7 @@ struct nbd_handle {
>    _Atomic uintptr_t private_data;
>  
>    char *export_name;            /* Export name, never NULL. */
> +  char *sa_name;                /* Socket activation name, can be NULL. */
>  
>    /* TLS settings. */
>    int tls;                      /* 0 = disable, 1 = enable, 2 = require */
"lib/handle.c" includes <signal.h>, and <signal.h>
reserves symbols
starting with the prefix "sa_":

https://pubs.opengroup.org/onlinepubs/9699919799/functions/V2_chap02.html#tag_15_02_02

While the fields of unions and structures live in a separate namespace,
that's no protection from macros.

Otherwise the patch looks OK to me.

(I'm noticing we already have fields called "sa_tmpdir" and
"sa_sockpath"; that's not ideal, although it may not matter in
practice.)

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

Laszlo