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

[Dave Scott <Dave.Scott@xxxxxxxxxx>]

On 30 Apr 2014, at 18:30, Luis R. Rodriguez <mcgrof@xxxxxxxx> wrote:

> On Wed, Apr 30, 2014 at 08:35:50AM +0000, Dave Scott wrote:
>> On 30 Apr 2014, at 02:11, Luis R. Rodriguez <mcgrof@xxxxxxxxxxxxxxxx> wrote:
>>> An important different with socket activation is that systemd will set
>>> FD_CLOEXEC for us on the socket before giving it to us, Ocaml gets
>>> support for [1] Unix.set_cloexec but only as of 4.00.1+dev which isn't
>>> yet widely available on distributions.
>> 
>> I’m not familiar with systemd but I’m curious: if systems is setting the flag
>> on the socket before giving it to us, why would we still need the
>> Unix.set_cloexec function in OCaml?
> 
> systemd is setting the flag for us, my point was that Unix.set_cloexec is not 
> being
> set yet for non-systemd cases. This can go in as a separate patch, I for some
> reason couldn't find this on documentation so I just checked out ocaml code to
> find the right call, would this be OK as a separate patch:
> 
> diff --git a/tools/ocaml/xenstored/utils.ml b/tools/ocaml/xenstored/utils.ml
> index d3d2e31..b206898 100644
> --- a/tools/ocaml/xenstored/utils.ml
> +++ b/tools/ocaml/xenstored/utils.ml
> @@ -78,13 +78,13 @@ let create_regular_unix_socket name =
>         Unixext.mkdir_rec (Filename.dirname name) 0o700;
>         let sockaddr = Unix.ADDR_UNIX(name) in
>         let sock = Unix.socket Unix.PF_UNIX Unix.SOCK_STREAM 0 in
> +        Unix.set_close_on_exec sock;
>         Unix.bind sock sockaddr;
>         Unix.listen sock 1;
>         sock

Sure, a separate patch is fine. I don’t think oxenstored ever execs, but 
there’s no harm in being careful just in case that changes in future.

> 
>> ...
>>> diff --git a/tools/ocaml/xenstored/systemd.ml 
>>> b/tools/ocaml/xenstored/systemd.ml
>>> new file mode 100644
>>> index 0000000..cace794
>>> --- /dev/null
>>> +++ b/tools/ocaml/xenstored/systemd.ml
>>> @@ -0,0 +1,16 @@
>>> +(*
>>> + * Copyright (C) 2014 Luis R. Rodriguez <mcgrof@xxxxxxxx>
>>> + *
>>> + * This program is free software; you can redistribute it and/or modify
>>> + * it under the terms of the GNU Lesser General Public License as published
>>> + * by the Free Software Foundation; version 2.1 only. with the special
>>> + * exception on linking described in file LICENSE.
>>> + *
>>> + * This program is distributed in the hope that it will be useful,
>>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>>> + * GNU Lesser General Public License for more details.
>>> + *)
>>> +
>>> +external sd_listen_fds: string -> Unix.file_descr = "ocaml_sd_listen_fds"
>>> +external sd_active_socket_required: unit -> int = 
>>> “ocaml_sd_active_socket_required"
>> 
>> Minor comment: It would be clearer if sd_active_socket_required was unit ->
>> bool. In the C code you should use Val_true and Val_false from
>> <caml/mlvalues.h>.
> 
> Thanks! Like this? If so then I rolled this in, and can send as part of my v5.
> 
> diff --git a/tools/ocaml/xenstored/systemd.ml 
> b/tools/ocaml/xenstored/systemd.ml
> index cace794..baa1c00 100644
> --- a/tools/ocaml/xenstored/systemd.ml
> +++ b/tools/ocaml/xenstored/systemd.ml
> @@ -13,4 +13,4 @@
>  *)
> 
> external sd_listen_fds: string -> Unix.file_descr = "ocaml_sd_listen_fds"
> -external sd_active_socket_required: unit -> int = 
> "ocaml_sd_active_socket_required"
> +external sd_active_socket_required: unit -> bool = 
> "ocaml_sd_active_socket_required"
> diff --git a/tools/ocaml/xenstored/systemd.mli 
> b/tools/ocaml/xenstored/systemd.mli
> index a65ea5e..6f2db9c 100644
> --- a/tools/ocaml/xenstored/systemd.mli
> +++ b/tools/ocaml/xenstored/systemd.mli
> @@ -18,4 +18,4 @@
> val sd_listen_fds: string -> Unix.file_descr
> 
> (** Tells us whether or not systemd support was compiled in *)
> -val sd_active_socket_required: unit -> int
> +val sd_active_socket_required: unit -> bool
> diff --git a/tools/ocaml/xenstored/systemd_stubs.c 
> b/tools/ocaml/xenstored/systemd_stubs.c
> index ded9542..942ae19 100644
> --- a/tools/ocaml/xenstored/systemd_stubs.c
> +++ b/tools/ocaml/xenstored/systemd_stubs.c
> @@ -13,6 +13,7 @@
>  */
> 
> #include <string.h>
> +#include <stdbool.h>
> #include <caml/mlvalues.h>
> #include <caml/memory.h>
> #include <caml/alloc.h>
> @@ -139,7 +140,7 @@ CAMLprim value ocaml_sd_active_socket_required(void)
>       CAMLparam0();
>       CAMLlocal1(ret);
> 
> -     ret = Val_int(1);
> +     ret = Val_true;
> 
>       CAMLreturn(ret);
> }
> @@ -159,7 +160,7 @@ CAMLprim value ocaml_sd_active_socket_required(void)
>       CAMLparam0();
>       CAMLlocal1(ret);
> 
> -     ret = Val_int(0);
> +     ret = Val_false;
> 
>       CAMLreturn(ret);
> }
> diff --git a/tools/ocaml/xenstored/utils.ml b/tools/ocaml/xenstored/utils.ml
> index d3d2e31..50f05c1 100644
> --- a/tools/ocaml/xenstored/utils.ml
> +++ b/tools/ocaml/xenstored/utils.ml
> @@ -83,8 +83,7 @@ let create_regular_unix_socket name =
>         sock
> 
> let create_unix_socket name =
> -        let active_sockets = Systemd.sd_active_socket_required() in
> -        if active_sockets = 1 then
> +        if Systemd.sd_active_socket_required() then
>                 Systemd.sd_listen_fds name
>         else
>                 create_regular_unix_socket name
>> 
>> Everything else looks good to me!
> 
> Thanks, can I sprinkle an Acked-by: Dave Scott <Dave.Scott@xxxxxxxxxx> ?

If you also add the parameter that Anil pointed out, then

Acked-by: Dave Scott <Dave.Scott@xxxxxxxxxx>


Thanks,
Dave


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