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

["Luis R. Rodriguez" <mcgrof@suse.com>]

On Wed, Jul 02, 2014 at 02:08:23PM +0100, Ian Campbell wrote:
> On Thu, 2014-06-12 at 18:18 -0700, Luis R. Rodriguez wrote:
> > 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?

Not that I was able to find but Ocaml folks can chime in.

> 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...)

Can't this be done as a separate step? Otherwise Ocaml is holding
Xen behind, has that compromise has already been made and accepted?

> > +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.

OK.

> > +	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.

stderr will make it out to the journal, the other errors are to also notify
systemd, additional messages can be pegged with fprintf(stderr, ...). We
want to be very explicit about the root cause of the issue if one triggered.

  Luis