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
_______________________________________________
Libguestfs mailing list
Libguestfs@redhat.com
https://listman.redhat.com/mailman/listinfo/libguestfs
