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: 5107 bytes --]

On Wed, Apr 30, 2014 at 08:35:50AM +0000, Dave Scott wrote:
> On 30 Apr 2014, at 02:11, Luis R. Rodriguez <mcgrof@do-not-panic.com> 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
 
> ...
> > 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@suse.com>
> > + *
> > + * 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@citrix.com> ?

  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