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

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

[-- Attachment #1.1: Type: text/plain, Size: 1571 bytes --]

On Wed, Apr 30, 2014 at 10:27:43AM +0100, Anil Madhavapeddy wrote:
> On 30 Apr 2014, at 02:11, Luis R. Rodriguez <mcgrof@do-not-panic.com> wrote:
> 
> > From: "Luis R. Rodriguez" <mcgrof@suse.com>
> > 
> > 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.
> > 
> <snip>
> 
> The definition of sd_active_socket_required (unit ->int) below doesn't
> match the C stub:
> 
> > +
> > +external sd_listen_fds: string -> Unix.file_descr = "ocaml_sd_listen_fds"
> > +external sd_active_socket_required: unit -> int = "ocaml_sd_active_socket_required"
> 
> > +
> > +CAMLprim value ocaml_sd_active_socket_required(void)
> > +{
> > +	CAMLparam0();
> > +	CAMLlocal1(ret);
> > +
> > +	ret = Val_int(0);
> > +
> > +	CAMLreturn(ret);
> > +}
> > +#endif
> 
> That should be:
> 
> CAMLprim value ocaml_sd_active_socket_required(value u)
> {
>   CAMLparam1(u);
>   CAMLlocal1(ret);
>   ret = Val_int(0);
>   CAMLreturn(ret);
> }

Do we have to require an argument though?

> (Since the function isn't doing any heap allocation, it's actually
> just safe to compress it to "return (Val_int(0))", but it's probably
> better to keep it like this to avoid any surprises).
> 
> As Dave notes, returning Val_true and setting the OCaml signature to
> bool would be a clearer interface.

OK thanks for the review! Let me know what you think of the other changes
I sent in reply to Dave.

  Luis

[-- Attachment #1.2: Type: application/pgp-signature, Size: 835 bytes --]

[-- Attachment #2: Type: text/plain, Size: 126 bytes --]

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