Re: [Xen-devel] [PATCH v4 05/15] oxenstored: add support for systemd active sockets

["Luis R. Rodriguez" <mcgrof@xxxxxxxxxxxxxxxx>]

On Mon, May 12, 2014 at 6:57 AM, Ian Jackson <Ian.Jackson@xxxxxxxxxxxxx> wrote:
> Luis R. Rodriguez writes ("[PATCH v4 05/15] oxenstored: add support for 
> systemd active sockets"):
> ...
>> +/* Conforms to what we should send sd_is_socket_unix() */
>> +struct xen_systemd_active_socket {
>> +     int fd;
>> +     int type;
>> +     int listening;
>> +     const char *path;
>> +     size_t length;
>> +};
>
> This is quite a lot of extraneous stuff, AFAICT.  For example, we
> always set listening to 0.
>
> Why is this array not const ?

Its set to 0 as this is and only should be used for the different
xenstores for initialization, I have a some fixes and optimizations
for this where I ended up sharing most of this code into a library
that can be used by cenxstored and oxenstored, will send it out soon.

I'll make the entire thing const.

>> +/*
>> + * We list stdin, stdout and stderr simply for documentation purposes
>> + * and to help our array size fit the number of expected sockets we
>> + * as sd_listen_fds() will return 5 for example if you set the socket
>> + * service with 2 sockets.
>> + */
>
> This doesn't seem very convincing to me.

Not sure what you mean, this is describing the logic used by systemd +
the order in which systemd gets our declared sockets to help keep
order once we ask for them.

>> +CAMLprim value ocaml_sd_listen_fds(value connect_to)
>> +{
>> +     CAMLparam1(connect_to);
>> +     CAMLlocal1(sock_ret);
>> +     int n, r;
>> +     struct xen_systemd_active_socket *active_socket;
>> +
>> +     active_socket = get_xen_active_socket((const char *) 
>> String_val(connect_to));
>> +     if (!active_socket)
>> +             caml_failwith("ocaml_sd_listen_fds() got invalid request");
>
> The sole purpose of this is to convert the string to an entry in the
> the table ?  I.e., essentially, just the table index ?

A few things actually:

  1. Used to that the path is expected by the program / systemd
  2. Lets then tell systemd we are ready for that socket
  3. Verification of the integrity of the socket as we expected it to be set up
  4. Verification that we only get from systemd the number of sockets expected

>> +/*
>> + * If xenstored was built to depend on systemd libraries
>> + * we assume you want all the bells and whistles with
>> + * systemd.
>> + */
>
> This is not correct.  A distro might want to use the same binary for
> both systemd and non-systemd installations.  You need to use a runtime
> test.

Will give dynamic checking a try in my new series.

  Luis

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel