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