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

[Richard W.M. Jones]

On Tue, Jan 31, 2023 at 10:29:00AM -0600, Eric Blake wrote:
> 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);

While I'm not totally opposed to this, I would say a few things:

 - the API is already stateful, even used in the most basic way,
   eg you must connect before you can do other things

 - having the state modified by various nbd_set_* calls allows us to
   easily add more variants, instead of having to create a
   combinatorial future set of nbd_connect_systemd_*_socket_activation
   calls

So I would lean towards my design.  (Also it's the same thing we
already do, eg. for opt mode).

> > +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.

I didn't know about this documentation before.

Arbitrary ASCII characters doesn't sound that great though.  I'm sure
that q-s-d will want its own much more strict limitations, eg. I
assume you can't possibly support any characters which are meaningful
to JSON / QMP.  Any thoughts on that?

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
libguestfs lets you edit virtual machines.  Supports shell scripting,
bindings from many languages.  http://libguestfs.org