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

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

On Tue, Jan 31, 2023 at 05:28:18PM +0000, Richard W.M. Jones
wrote:
> > > +  /* 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.
I only found it this morning.
> 
> 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?
You have a strong point there.  Just because systemd allows it doesn't
make it wise; I'm a big fan of "it's easier to resrict now and
loosen
later when we see the need", as being easier than "let's try and
be as
loose as we can now, then regret it later when we find it was a
security hole".  Limiting to alphanumeric and 32 bytes until someone
proves they have a use case for needing more than that works for me.

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

On 1/31/23 18:28, Richard W.M. Jones wrote:
> 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
This subthread is quite funny to me because I have mostly accepted the
reasoning behind the API being such as it is :) Thank you, Eric, for
remembering my qualms.
>  - 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).
"Combinatorial explosion" was certainly my thought here, too!

I guess the ship has sailed; upon seeing this patch, I only said,
"hrmpf, another knob, okay". It's consistent with the status quo.

Laszlo

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