Re: [Xen-devel] [PATCH v6 03/13] oxenstored: add support for systemd active sockets

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

On Thu, 2014-06-12 at 18:18 -0700, Luis R. Rodriguez wrote:
> From: "Luis R. Rodriguez" <mcgrof@xxxxxxxx>
> 
> This adds systemd socket activation support for the Ocaml xenstored.
> Ocaml lacks systemd library support so we provide our own C helpers
> as is done with other functionality lacking on Ocaml.
> 
> Active sockets enables oxenstored to be loaded only if required by a system
> onto which Xen is installed on. Socket activation is handled by
> systemd, once a port for a service which claims a socket is used
> systemd will start the required services for it, on demand. For more
> details on socket activation refer to Lennart's socket-activation
> post regarding this [0].
> 
> An important difference with socket activation is that systemd will set
> FD_CLOEXEC for us on the socket before giving it to us, we'll sprinkly
> the Unix.set_close_on_exec for LSB init next as a separate commit.
> 
> Right now this code adds a no-op for this functionality, leaving the
> enablement to be done later once systemd is properly hooked into
> the build system. The socket activation is ordered in aligment with
> the socket activation order passed on to systemd.
> 
> [0] http://0pointer.de/blog/projects/socket-activation2.html

I suspect some of my comments against C xenstored will apply to the C
code here too...

> diff --git a/tools/ocaml/xenstored/systemd.ml 
> b/tools/ocaml/xenstored/systemd.ml
> new file mode 100644
> index 0000000..2aa39ea
> --- /dev/null
> +++ b/tools/ocaml/xenstored/systemd.ml

Ideally the systemd ocaml bindings would come from a suitable ocaml
library (opam or whatever). I suppose such a thing doesn't exist
already?

Perhaps Dave or Anil etc could advise on the feasibility of publishing
these bindings as a separate project. In general I'd much rather we
added build dependencies for things like that than incorporate things
which are nothing to do with Xen etc into the tree (we've done too much
of that in the past...)

> +CAMLprim value ocaml_sd_listen_fds(value connect_to)
> +{
> +     CAMLparam1(connect_to);
> +     CAMLlocal1(sock_ret);
> +     int sock = -EBADR, n;
> +
> +     n = sd_listen_fds(0);
> +     if (n <= 0) {
> +             sd_notifyf(0, "STATUS=Failed to get any active sockets: %s\n"
> +                        "ERRNO=%i",
> +                        strerror(errno),
> +                        errno);
> +             caml_failwith("ocaml_sd_listen_fds() failed to get any 
> sockets");
> +     } else if (n > 2) {
> +             fprintf(stderr, SD_ERR "Expected 2 fds but given %d\n", n);
> +             sd_notifyf(0, "STATUS=Mismatch on number (2): %s\n"
> +                        "ERRNO=%d",
> +                        strerror(EBADR),
> +                        EBADR);
> +             caml_failwith("ocaml_sd_listen_fds() mismatch");
> +     }
> +
> +     sock = oxen_verify_socket_socket((const char *) String_val(connect_to));

String_val() gives you a char *, which ought to be automatically
promoted to const as necessary. So I think the case is unnecessary and
only serves to potentially hide actual errors.

> +     if (sock <= 0) {
> +             fprintf(stderr, "failed to verify sock %s\n",
> +                     (const char *) String_val(connect_to));

Same, if less critical, here.

Is stderr the best place to shove this message? sd_notify is also used
elsewhere it seems.

> +             caml_failwith("ocaml_sd_listen_fds_init() invalid socket");
> +     }
> +
> +     sock_ret = Val_int(sock);
> +
> +     CAMLreturn(sock_ret);
> +}
> +

Ian.


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