10:27 a.m.
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.