libxl: ocaml: fix tests makefile and osevent callbacks

libxl: ocaml: fix tests makefile and osevent callbacks

[Rob Hoes]

It turned out that a little more is needed to make the OCaml bindings for libxl
"perfect"... ;)

These patches fix a few issues in the bindings to do with the osevent
callbacks, and the ocaml/test makefile (reported by Sander Eikelenboom). The
changes are fully contained within tools/ocaml.

I'd really appreciate it if these fixes can still be considered for Xen 4.4,
for the same reasons as the larger libxl/ocaml series that went in yesterday.

Cheers,
Rob

[PATCH 1/3] ocaml: do not install test binaries

[Rob Hoes]

Signed-off-by: Rob Hoes <rob.hoes@citrix.com>
---
 tools/ocaml/test/Makefile |    4 ----
 1 file changed, 4 deletions(-)

diff --git a/tools/ocaml/test/Makefile b/tools/ocaml/test/Makefile
index 827bd7c..20aac4d 100644
--- a/tools/ocaml/test/Makefile
+++ b/tools/ocaml/test/Makefile
@@ -49,8 +49,4 @@ all: $(PROGRAMS)
 
 bins: $(PROGRAMS)
 
-install: all
-	$(INSTALL_DIR) $(DESTDIR)$(BINDIR)
-	$(INSTALL_PROG) $(PROGRAMS) $(DESTDIR)$(BINDIR)
-
 include $(OCAML_TOPLEVEL)/Makefile.rules
-- 
1.7.10.4

Re: [PATCH 1/3] ocaml: do not install test binaries

[Ian Jackson]

Rob Hoes writes ("[PATCH 1/3] ocaml: do not install test binaries"):
> Signed-off-by: Rob Hoes <rob.hoes@citrix.com>

Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>

Re: [PATCH 1/3] ocaml: do not install test binaries

[Ian Campbell]

On Thu, 2013-12-12 at 16:43 +0000, Ian Jackson wrote:
> Rob Hoes writes ("[PATCH 1/3] ocaml: do not install test binaries"):
> > Signed-off-by: Rob Hoes <rob.hoes@citrix.com>
> 
> Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>

I'm afraid this doesn't quite work:
make[5]: Entering directory `/local/scratch/ianc/devel/committer.git/tools/ocam
l/test'
make[5]: *** No rule to make target `install'.  Stop.

I'm going to apply with the following incremental diff folded in:

diff --git a/tools/ocaml/test/Makefile b/tools/ocaml/test/Makefile
index 20aac4d..8033089 100644
--- a/tools/ocaml/test/Makefile
+++ b/tools/ocaml/test/Makefile
@@ -49,4 +49,6 @@ all: $(PROGRAMS)
 
 bins: $(PROGRAMS)
 
+install:
+
 include $(OCAML_TOPLEVEL)/Makefile.rules

[PATCH 2/3] libxl: ocaml: use int64 for timeval fields in the timeout_register callback

[Rob Hoes]

The original code works fine on 64-bit, but on 32-bit, the OCaml int (which is
1 bit smaller than the C int) is likely to overflow.

Signed-off-by: Rob Hoes <rob.hoes@citrix.com>
---
 tools/ocaml/libs/xl/xenlight.mli.in  |    2 +-
 tools/ocaml/libs/xl/xenlight_stubs.c |    8 ++++++--
 2 files changed, 7 insertions(+), 3 deletions(-)

diff --git a/tools/ocaml/libs/xl/xenlight.mli.in b/tools/ocaml/libs/xl/xenlight.mli.in
index 794dbf1..b9819e1 100644
--- a/tools/ocaml/libs/xl/xenlight.mli.in
+++ b/tools/ocaml/libs/xl/xenlight.mli.in
@@ -71,7 +71,7 @@ module Async : sig
 		fd_register:('a -> Unix.file_descr -> event list -> for_libxl -> unit) ->
 		fd_modify:('a -> Unix.file_descr -> event list -> unit) ->
 		fd_deregister:('a -> Unix.file_descr -> unit) ->
-		timeout_register:('a -> int -> int -> for_libxl -> unit) ->
+		timeout_register:('a -> int64 -> int64 -> for_libxl -> unit) ->
 		timeout_modify:('a -> unit) ->
 		osevent_hooks
 
diff --git a/tools/ocaml/libs/xl/xenlight_stubs.c b/tools/ocaml/libs/xl/xenlight_stubs.c
index a61c222..2e2606a 100644
--- a/tools/ocaml/libs/xl/xenlight_stubs.c
+++ b/tools/ocaml/libs/xl/xenlight_stubs.c
@@ -1286,6 +1286,7 @@ int timeout_register(void *user, void **for_app_registration_out,
 {
 	caml_leave_blocking_section();
 	CAMLparam0();
+	CAMLlocal2(sec, usec);
 	CAMLlocalN(args, 4);
 	static value *func = NULL;
 	value *p = (value *) user;
@@ -1295,9 +1296,12 @@ int timeout_register(void *user, void **for_app_registration_out,
 		func = caml_named_value("libxl_timeout_register");
 	}
 
+	sec = caml_copy_int64(abs.tv_sec);
+	usec = caml_copy_int64(abs.tv_usec);
+
 	args[0] = *p;
-	args[1] = Val_int(abs.tv_sec);
-	args[2] = Val_int(abs.tv_usec);
+	args[1] = sec;
+	args[2] = usec;
 	args[3] = (value) for_libxl;
 
 	caml_callbackN(*func, 4, args);
-- 
1.7.10.4

Re: [PATCH 2/3] libxl: ocaml: use int64 for timeval fields in the timeout_register callback

[Ian Jackson]

Rob Hoes writes ("[PATCH 2/3] libxl: ocaml: use int64 for timeval fields in the timeout_register callback"):
> The original code works fine on 64-bit, but on 32-bit, the OCaml int
> (which is 1 bit smaller than the C int) is likely to overflow.

The timeouts are in milliseconds.  2^30 ms is 1.07megaseconds, or 12.4
days.

If it would help I would be happy to have libxl promise never to use
such absurdly long timeouts.

Ian.

Re: [PATCH 2/3] libxl: ocaml: use int64 for timeval fields in the timeout_register callback

[Rob Hoes]

Ian Jackson wrote:
> Rob Hoes writes ("[PATCH 2/3] libxl: ocaml: use int64 for timeval fields
> in the timeout_register callback"):
> > The original code works fine on 64-bit, but on 32-bit, the OCaml int
> > (which is 1 bit smaller than the C int) is likely to overflow.
> 
> The timeouts are in milliseconds.  2^30 ms is 1.07megaseconds, or 12.4
> days.
> 
> If it would help I would be happy to have libxl promise never to use such
> absurdly long timeouts.

This was because the absolute time is given to timeout_register, and the tv_sec field of the timeval that comes out of gettimeofday() is just a little too large for a 31-bit signed integer...

I didn't see this before, because I was testing on a 64-bit box. When I moved to our 32-bit dom0, it started overflowing.

Cheers,
Rob

Re: [PATCH 2/3] libxl: ocaml: use int64 for timeval fields in the timeout_register callback

[Ian Jackson]

Rob Hoes writes ("RE: [PATCH 2/3] libxl: ocaml: use int64 for timeval fields in the timeout_register callback"):
> Ian Jackson wrote:
> > If it would help I would be happy to have libxl promise never to use such
> > absurdly long timeouts.
> 
> This was because the absolute time is given to timeout_register, and the tv_sec field of the timeval that comes out of gettimeofday() is just a little too large for a 31-bit signed integer...

Oh, right.

> I didn't see this before, because I was testing on a 64-bit box. When I moved to our 32-bit dom0, it started overflowing.

I had forgotten that the timeout registration presents an absolute
timeval.

Does ocaml not have a more natural format for a timeval than two
int64's ?

(The tv_nsec field is guaranteed to be < 1E9 so fits in 30 bits...)

Ian.

Re: [PATCH 2/3] libxl: ocaml: use int64 for timeval fields in the timeout_register callback

[Rob Hoes]

Ian Jackson wrote:
> Rob Hoes writes ("RE: [PATCH 2/3] libxl: ocaml: use int64 for timeval
> fields in the timeout_register callback"):
> > Ian Jackson wrote:
> > > If it would help I would be happy to have libxl promise never to use
> > > such absurdly long timeouts.
> >
> > This was because the absolute time is given to timeout_register, and the
> tv_sec field of the timeval that comes out of gettimeofday() is just a
> little too large for a 31-bit signed integer...
> 
> Oh, right.
> 
> > I didn't see this before, because I was testing on a 64-bit box. When I
> moved to our 32-bit dom0, it started overflowing.
> 
> I had forgotten that the timeout registration presents an absolute timeval.
> 
> Does ocaml not have a more natural format for a timeval than two int64's ?
> 
> (The tv_nsec field is guaranteed to be < 1E9 so fits in 30 bits...)

The OCaml Unix.gettimeofday function returns a number of type "float", which in OCaml is a 64-bit number. This may be an alternative.

I just thought it would be best to stay as close as possible to the C implementation in the bindings, and return the two separate numbers that we are given.

Rob

Re: [PATCH 2/3] libxl: ocaml: use int64 for timeval fields in the timeout_register callback

[Ian Jackson]

Rob Hoes writes ("RE: [PATCH 2/3] libxl: ocaml: use int64 for timeval fields in the timeout_register callback"):
> Ian Jackson wrote:
> > (The tv_nsec field is guaranteed to be < 1E9 so fits in 30 bits...)
> 
> The OCaml Unix.gettimeofday function returns a number of type "float", which in OCaml is a 64-bit number. This may be an alternative.

You mean it has a 64-bit mantissa, or it's 64 bits big (an IEEE
double) ?

Also that's a very strange way of representing a timeval (which is
mixed radix, since the two halves have relative weight 10^9 rather
than 2^n).

> I just thought it would be best to stay as close as possible to the
> C implementation in the bindings, and return the two separate
> numbers that we are given.

Right.  I think on the basis of what you've said, that makes sense,
although I'm not sure whether nsec should be 32 (30+sign) or 64.

Ian.

Re: [PATCH 2/3] libxl: ocaml: use int64 for timeval fields in the timeout_register callback

[Rob Hoes]

Ian Jackson wrote:
> Sent: 12 December 2013 5:14 PM
> To: Rob Hoes
> Cc: xen-devel@lists.xen.org; Ian Campbell; Dave Scott
> Subject: RE: [PATCH 2/3] libxl: ocaml: use int64 for timeval fields in the
> timeout_register callback
> 
> Rob Hoes writes ("RE: [PATCH 2/3] libxl: ocaml: use int64 for timeval
> fields in the timeout_register callback"):
> > Ian Jackson wrote:
> > > (The tv_nsec field is guaranteed to be < 1E9 so fits in 30 bits...)
> >
> > The OCaml Unix.gettimeofday function returns a number of type "float",
> which in OCaml is a 64-bit number. This may be an alternative.
> 
> You mean it has a 64-bit mantissa, or it's 64 bits big (an IEEE
> double) ?

64 bits big, according to the IEEE 754 standard (http://caml.inria.fr/pub/docs/manual-ocaml/libref/Pervasives.html),

> Also that's a very strange way of representing a timeval (which is mixed
> radix, since the two halves have relative weight 10^9 rather than 2^n).
> 
> > I just thought it would be best to stay as close as possible to the C
> > implementation in the bindings, and return the two separate numbers
> > that we are given.
> 
> Right.  I think on the basis of what you've said, that makes sense,
> although I'm not sure whether nsec should be 32 (30+sign) or 64.

I guess we can probably get away with 32 bits for that one, based on what we know the of the range of possible values. I'd just like to avoid any possible trouble, and didn't think those extra bits would cause too much harm.

Rob

Re: [PATCH 2/3] libxl: ocaml: use int64 for timeval fields in the timeout_register callback

[Ian Jackson]

Rob Hoes writes ("RE: [PATCH 2/3] libxl: ocaml: use int64 for timeval fields in the timeout_register callback"):
> Ian Jackson wrote:
> > You mean it has a 64-bit mantissa, or it's 64 bits big (an IEEE
> > double) ?
> 
> 64 bits big, according to the IEEE 754 standard (http://caml.inria.fr/pub/docs/manual-ocaml/libref/Pervasives.html),

Well, that's too small for a timeval, which has a 32-bit seconds value
(currently using 31 of those bits and heaven help us when we get to
0x80000000) and a ~30-bit nsecs value.

> I guess we can probably get away with 32 bits for that one, based on what we know the of the range of possible values. I'd just like to avoid any possible trouble, and didn't think those extra bits would cause too much harm.

Right.

Ian.

Re: [PATCH 2/3] libxl: ocaml: use int64 for timeval fields in the timeout_register callback

[Ian Jackson]

Ian Jackson writes ("RE: [PATCH 2/3] libxl: ocaml: use int64 for timeval fields in the timeout_register callback"):
> Well, that's too small for a timeval, which has a 32-bit seconds value
> (currently using 31 of those bits and heaven help us when we get to
> 0x80000000) and a ~30-bit nsecs value.

I'm thinking of timespecs not timevals.  But:

> > I guess we can probably get away with 32 bits for that one, based on what we know the of the range of possible values. I'd just like to avoid any possible trouble, and didn't think those extra bits would cause too much harm.
> 
> Right.

I now think this is fine.

Ian.

Re: [PATCH 2/3] libxl: ocaml: use int64 for timeval fields in the timeout_register callback

[Ian Campbell]

On Thu, 2013-12-12 at 18:32 +0000, Ian Jackson wrote:
> Ian Jackson writes ("RE: [PATCH 2/3] libxl: ocaml: use int64 for timeval fields in the timeout_register callback"):
> > Well, that's too small for a timeval, which has a 32-bit seconds value
> > (currently using 31 of those bits and heaven help us when we get to
> > 0x80000000) and a ~30-bit nsecs value.
> 
> I'm thinking of timespecs not timevals.  But:
> 
> > > I guess we can probably get away with 32 bits for that one, based on what we know the of the range of possible values. I'd just like to avoid any possible trouble, and didn't think those extra bits would cause too much harm.
> > 
> > Right.
> 
> I now think this is fine.

Sorry for not following along properly -- do you mean the original patch
is fine or that one of the other schemes discussed in this thread is
going to be OK once implemented?

Ian.

Re: [PATCH 2/3] libxl: ocaml: use int64 for timeval fields in the timeout_register callback

[Ian Jackson]

Ian Campbell writes ("Re: [PATCH 2/3] libxl: ocaml: use int64 for timeval fields in the timeout_register callback"):
> On Thu, 2013-12-12 at 18:32 +0000, Ian Jackson wrote:
> > I now think this is fine.
> 
> Sorry for not following along properly -- do you mean the original patch
> is fine or that one of the other schemes discussed in this thread is
> going to be OK once implemented?

I mean that I approve of the plan to use two int64's as in Rob's
proposal.

Ian.

Re: [PATCH 2/3] libxl: ocaml: use int64 for timeval fields in the timeout_register callback

[David Scott]

On 16/12/13 17:06, Ian Jackson wrote:
> Ian Campbell writes ("Re: [PATCH 2/3] libxl: ocaml: use int64 for timeval fields in the timeout_register callback"):
>> On Thu, 2013-12-12 at 18:32 +0000, Ian Jackson wrote:
>>> I now think this is fine.
>>
>> Sorry for not following along properly -- do you mean the original patch
>> is fine or that one of the other schemes discussed in this thread is
>> going to be OK once implemented?
>
> I mean that I approve of the plan to use two int64's as in Rob's
> proposal.

I'm also happy with the two int64 plan.

Acked-by: David Scott <dave.scott@eu.citrix.com>

Re: [PATCH 2/3] libxl: ocaml: use int64 for timeval fields in the timeout_register callback

[Ian Campbell]

On Thu, 2013-12-12 at 16:43 +0000, Ian Jackson wrote:
> Rob Hoes writes ("[PATCH 2/3] libxl: ocaml: use int64 for timeval fields in the timeout_register callback"):
> > The original code works fine on 64-bit, but on 32-bit, the OCaml int
> > (which is 1 bit smaller than the C int) is likely to overflow.
> 
> The timeouts are in milliseconds.  2^30 ms is 1.07megaseconds, or 12.4
> days.
> 
> If it would help I would be happy to have libxl promise never to use
> such absurdly long timeouts.

I see the conversation has moved on but I thought it would be worth
pointing out that for IDL defined datatypes we have declared that an
integer is a signed 24 bit type, exactly to help with languages which
steal bit or two. See the end of tools/libxl/idl.txt

The IDL has separate explicitly sized 32- and 64-bit types too.

Ian.

Re: [PATCH 2/3] libxl: ocaml: use int64 for timeval fields in the timeout_register callback

[Ian Jackson]

Rob Hoes writes ("[PATCH 2/3] libxl: ocaml: use int64 for timeval fields in the timeout_register callback"):
> The original code works fine on 64-bit, but on 32-bit, the OCaml int (which is
> 1 bit smaller than the C int) is likely to overflow.

Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>

Re: [PATCH 2/3] libxl: ocaml: use int64 for timeval fields in the timeout_register callback

[Ian Campbell]

On Tue, 2014-01-07 at 17:41 +0000, Ian Jackson wrote:
> Rob Hoes writes ("[PATCH 2/3] libxl: ocaml: use int64 for timeval fields in the timeout_register callback"):
> > The original code works fine on 64-bit, but on 32-bit, the OCaml int (which is
> > 1 bit smaller than the C int) is likely to overflow.
> 
> Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>

Applied, thanks.

[PATCH 3/3] libxl: ocaml: use 'for_app_registration' in osevent callbacks

[Rob Hoes]

This allows the application to pass a token to libxl in the fd/timeout
registration callbacks, which it receives back in modification or
deregistration callbacks.

It turns out that this is essential for timeout handling, in order to
identify which timeout to change on a modify event.

Signed-off-by: Rob Hoes <rob.hoes@citrix.com>
---
 tools/ocaml/libs/xl/xenlight.mli.in  |   10 ++--
 tools/ocaml/libs/xl/xenlight_stubs.c |   97 ++++++++++++++++++++++++++--------
 2 files changed, 81 insertions(+), 26 deletions(-)

diff --git a/tools/ocaml/libs/xl/xenlight.mli.in b/tools/ocaml/libs/xl/xenlight.mli.in
index b9819e1..277e81d 100644
--- a/tools/ocaml/libs/xl/xenlight.mli.in
+++ b/tools/ocaml/libs/xl/xenlight.mli.in
@@ -68,11 +68,11 @@ module Async : sig
 
 	val osevent_register_hooks : ctx ->
 		user:'a ->
-		fd_register:('a -> Unix.file_descr -> event list -> for_libxl -> unit) ->
-		fd_modify:('a -> Unix.file_descr -> event list -> unit) ->
-		fd_deregister:('a -> Unix.file_descr -> unit) ->
-		timeout_register:('a -> int64 -> int64 -> for_libxl -> unit) ->
-		timeout_modify:('a -> unit) ->
+		fd_register:('a -> Unix.file_descr -> event list -> for_libxl -> 'b) ->
+		fd_modify:('a -> Unix.file_descr -> 'b -> event list -> 'b) ->
+		fd_deregister:('a -> Unix.file_descr -> 'b -> unit) ->
+		timeout_register:('a -> int64 -> int64 -> for_libxl -> 'c) ->
+		timeout_modify:('a -> 'c -> 'c) ->
 		osevent_hooks
 
 	external osevent_occurred_fd : ctx -> for_libxl -> Unix.file_descr -> event list -> event list -> unit = "stub_libxl_osevent_occurred_fd"
diff --git a/tools/ocaml/libs/xl/xenlight_stubs.c b/tools/ocaml/libs/xl/xenlight_stubs.c
index 2e2606a..3c72eeb 100644
--- a/tools/ocaml/libs/xl/xenlight_stubs.c
+++ b/tools/ocaml/libs/xl/xenlight_stubs.c
@@ -1211,14 +1211,20 @@ value Val_poll_events(short events)
 	CAMLreturn(event_list);
 }
 
+/* The process for dealing with the for_app_registration_  values in the
+ * callbacks below (GC registrations etc) is similar to the way for_callback is
+ * handled in the asynchronous operations above. */
+
 int fd_register(void *user, int fd, void **for_app_registration_out,
                      short events, void *for_libxl)
 {
 	caml_leave_blocking_section();
 	CAMLparam0();
 	CAMLlocalN(args, 4);
+	int ret = 0;
 	static value *func = NULL;
 	value *p = (value *) user;
+	value *for_app;
 
 	if (func == NULL) {
 		/* First time around, lookup by name */
@@ -1230,10 +1236,18 @@ int fd_register(void *user, int fd, void **for_app_registration_out,
 	args[2] = Val_poll_events(events);
 	args[3] = (value) for_libxl;
 
-	caml_callbackN(*func, 4, args);
+	for_app = malloc(sizeof(value));
+	if (for_app) {
+		*for_app = caml_callbackN(*func, 4, args);
+		caml_register_global_root(for_app);
+		*for_app_registration_out = for_app;
+	}
+	else
+		ret = ERROR_OSEVENT_REG_FAIL;
+
 	CAMLdone;
 	caml_enter_blocking_section();
-	return 0;
+	return ret;
 }
 
 int fd_modify(void *user, int fd, void **for_app_registration_update,
@@ -1241,32 +1255,43 @@ int fd_modify(void *user, int fd, void **for_app_registration_update,
 {
 	caml_leave_blocking_section();
 	CAMLparam0();
-	CAMLlocalN(args, 3);
+	CAMLlocalN(args, 4);
+	int ret = 0;
 	static value *func = NULL;
 	value *p = (value *) user;
+	value *for_app = *for_app_registration_update;
 
-	if (func == NULL) {
-		/* First time around, lookup by name */
-		func = caml_named_value("libxl_fd_modify");
-	}
+	/* if for_app == NULL, assume that something is wrong and don't callback */
+	if (for_app) {
+		if (func == NULL) {
+			/* First time around, lookup by name */
+			func = caml_named_value("libxl_fd_modify");
+		}
 
-	args[0] = *p;
-	args[1] = Val_int(fd);
-	args[2] = Val_poll_events(events);
+		args[0] = *p;
+		args[1] = Val_int(fd);
+		args[2] = *for_app;
+		args[3] = Val_poll_events(events);
+
+		*for_app = caml_callbackN(*func, 4, args);
+		*for_app_registration_update = for_app;
+	}
+	else
+		ret = ERROR_OSEVENT_REG_FAIL;
 
-	caml_callbackN(*func, 3, args);
 	CAMLdone;
 	caml_enter_blocking_section();
-	return 0;
+	return ret;
 }
 
 void fd_deregister(void *user, int fd, void *for_app_registration)
 {
 	caml_leave_blocking_section();
 	CAMLparam0();
-	CAMLlocalN(args, 2);
+	CAMLlocalN(args, 3);
 	static value *func = NULL;
 	value *p = (value *) user;
+	value *for_app = for_app_registration;
 
 	if (func == NULL) {
 		/* First time around, lookup by name */
@@ -1275,8 +1300,12 @@ void fd_deregister(void *user, int fd, void *for_app_registration)
 
 	args[0] = *p;
 	args[1] = Val_int(fd);
+	args[2] = *for_app;
 
-	caml_callbackN(*func, 2, args);
+	caml_remove_global_root(for_app);
+	free(for_app);
+
+	caml_callbackN(*func, 3, args);
 	CAMLdone;
 	caml_enter_blocking_section();
 }
@@ -1288,8 +1317,10 @@ int timeout_register(void *user, void **for_app_registration_out,
 	CAMLparam0();
 	CAMLlocal2(sec, usec);
 	CAMLlocalN(args, 4);
+	int ret = 0;
 	static value *func = NULL;
 	value *p = (value *) user;
+	value *for_app;
 
 	if (func == NULL) {
 		/* First time around, lookup by name */
@@ -1304,10 +1335,18 @@ int timeout_register(void *user, void **for_app_registration_out,
 	args[2] = usec;
 	args[3] = (value) for_libxl;
 
-	caml_callbackN(*func, 4, args);
+	for_app = malloc(sizeof(value));
+	if (for_app) {
+		*for_app = caml_callbackN(*func, 4, args);
+		caml_register_global_root(for_app);
+		*for_app_registration_out = for_app;
+	}
+	else
+		ret = ERROR_OSEVENT_REG_FAIL;
+
 	CAMLdone;
 	caml_enter_blocking_section();
-	return 0;
+	return ret;
 }
 
 int timeout_modify(void *user, void **for_app_registration_update,
@@ -1315,18 +1354,34 @@ int timeout_modify(void *user, void **for_app_registration_update,
 {
 	caml_leave_blocking_section();
 	CAMLparam0();
+	CAMLlocalN(args, 2);
+	int ret = 0;
 	static value *func = NULL;
 	value *p = (value *) user;
+	value *for_app = *for_app_registration_update;
 
-	if (func == NULL) {
-		/* First time around, lookup by name */
-		func = caml_named_value("libxl_timeout_modify");
+	/* if for_app == NULL, assume that something is wrong and don't callback */
+	if (for_app) {
+		if (func == NULL) {
+			/* First time around, lookup by name */
+			func = caml_named_value("libxl_timeout_modify");
+		}
+
+		args[0] = *p;
+		args[1] = *for_app;
+
+		caml_callbackN(*func, 2, args);
+
+		caml_remove_global_root(for_app);
+		free(for_app);
+		*for_app_registration_update = NULL;
 	}
+	else
+		ret = ERROR_OSEVENT_REG_FAIL;
 
-	caml_callback(*func, *p);
 	CAMLdone;
 	caml_enter_blocking_section();
-	return 0;
+	return ret;
 }
 
 void timeout_deregister(void *user, void *for_app_registration)
-- 
1.7.10.4

Re: [PATCH 3/3] libxl: ocaml: use 'for_app_registration' in osevent callbacks

[Ian Campbell]

> @@ -1241,32 +1255,43 @@ int fd_modify(void *user, int fd, void **for_app_registration_update,
>  {
>  	caml_leave_blocking_section();
>  	CAMLparam0();
> -	CAMLlocalN(args, 3);
> +	CAMLlocalN(args, 4);
> +	int ret = 0;
>  	static value *func = NULL;
>  	value *p = (value *) user;
> +	value *for_app = *for_app_registration_update;
>  
> -	if (func == NULL) {
> -		/* First time around, lookup by name */
> -		func = caml_named_value("libxl_fd_modify");
> -	}
> +	/* if for_app == NULL, assume that something is wrong and don't callback */
> +	if (for_app) {

if (!for_app) {
	cleanup
	return;
}

Would be more natural and save perturbing the rest of the function so
much. If the cleanup is the same as the tail of the function then the
goto style of error handling is good too:

	if (!for_app) {
		ret = ERROR__....;
		goto err;
	}
	[...]
	ret = ...

 err:
 	CAMLdone;
  	caml_enter_blocking_section();
	return ret;
}

Actually, elsewhere you don't bother to check for_app. If this would
indicate a major error then I think an assert would be perfectly
reasonably.

> +		if (func == NULL) {
> +			/* First time around, lookup by name */
> +			func = caml_named_value("libxl_fd_modify");
> +		}
>  
> -	args[0] = *p;
> -	args[1] = Val_int(fd);
> -	args[2] = Val_poll_events(events);
> +		args[0] = *p;
> +		args[1] = Val_int(fd);
> +		args[2] = *for_app;
> +		args[3] = Val_poll_events(events);
> +
> +		*for_app = caml_callbackN(*func, 4, args);
> +		*for_app_registration_update = for_app;
> +	}
> +	else
> +		ret = ERROR_OSEVENT_REG_FAIL;
>  
> -	caml_callbackN(*func, 3, args);
>  	CAMLdone;
>  	caml_enter_blocking_section();
> -	return 0;
> +	return ret;
>  }
> +	for_app = malloc(sizeof(value));
> +	if (for_app) {
> +		*for_app = caml_callbackN(*func, 4, args);

Can the callbackN fail, eg. returning an exception or something?
> @@ -1315,18 +1354,34 @@ int timeout_modify(void *user, void **for_app_registration_update,
>  {
>  	caml_leave_blocking_section();
>  	CAMLparam0();
> +	CAMLlocalN(args, 2);
> +	int ret = 0;
>  	static value *func = NULL;
>  	value *p = (value *) user;
> +	value *for_app = *for_app_registration_update;
>  
> -	if (func == NULL) {
> -		/* First time around, lookup by name */
> -		func = caml_named_value("libxl_timeout_modify");
> +	/* if for_app == NULL, assume that something is wrong and don't callback */

Same comment as the other occasion.

Re: [PATCH 3/3] libxl: ocaml: use 'for_app_registration' in osevent callbacks

[Ian Jackson]

Ian Campbell writes ("Re: [PATCH 3/3] libxl: ocaml: use 'for_app_registration' in osevent callbacks"):
> > @@ -1241,32 +1255,43 @@ int fd_modify(void *user, int fd, void **for_app_registration_update,
...
> > +	/* if for_app == NULL, assume that something is wrong and don't callback */
> > +	if (for_app) {
> 
> if (!for_app) {
> 	cleanup
> 	return;
> }

Why not
  assert(for_app)
?

> Actually, elsewhere you don't bother to check for_app. If this would
> indicate a major error then I think an assert would be perfectly
> reasonably.

:-).

Ian.

Re: [PATCH 3/3] libxl: ocaml: use 'for_app_registration' in osevent callbacks

[Rob Hoes]

Ian Campbell wrote:
> > @@ -1241,32 +1255,43 @@ int fd_modify(void *user, int fd, void
> > **for_app_registration_update,  {
> >  	caml_leave_blocking_section();
> >  	CAMLparam0();
> > -	CAMLlocalN(args, 3);
> > +	CAMLlocalN(args, 4);
> > +	int ret = 0;
> >  	static value *func = NULL;
> >  	value *p = (value *) user;
> > +	value *for_app = *for_app_registration_update;
> >
> > -	if (func == NULL) {
> > -		/* First time around, lookup by name */
> > -		func = caml_named_value("libxl_fd_modify");
> > -	}
> > +	/* if for_app == NULL, assume that something is wrong and don't
> callback */
> > +	if (for_app) {
> 
> if (!for_app) {
> 	cleanup
> 	return;
> }
> 
> Would be more natural and save perturbing the rest of the function so much.
> If the cleanup is the same as the tail of the function then the goto style
> of error handling is good too:

I think this is a matter or taste... To me, using goto isn't natural at all! I find the control flow a little clearer the way I did it.

However, I can change it to what you have suggested if that brings it more in line with the rest of the C code in the tree.

> 	if (!for_app) {
> 		ret = ERROR__....;
> 		goto err;
> 	}
> 	[...]
> 	ret = ...
> 
>  err:
>  	CAMLdone;
>   	caml_enter_blocking_section();
> 	return ret;
> }
> 
> Actually, elsewhere you don't bother to check for_app. If this would
> indicate a major error then I think an assert would be perfectly
> reasonably.

I think the check is missing from fd_deregister, indeed. I need to add it there.

If it's an assert, I believe it would kill the whole application? I think that's a little too much. I think that returning ERROR_OSEVENT_REG_FAIL would be the right thing here, because I believe that libxl would send it back to the application and just fail the current operation.

Rob

> > +		if (func == NULL) {
> > +			/* First time around, lookup by name */
> > +			func = caml_named_value("libxl_fd_modify");
> > +		}
> >
> > -	args[0] = *p;
> > -	args[1] = Val_int(fd);
> > -	args[2] = Val_poll_events(events);
> > +		args[0] = *p;
> > +		args[1] = Val_int(fd);
> > +		args[2] = *for_app;
> > +		args[3] = Val_poll_events(events);
> > +
> > +		*for_app = caml_callbackN(*func, 4, args);
> > +		*for_app_registration_update = for_app;
> > +	}
> > +	else
> > +		ret = ERROR_OSEVENT_REG_FAIL;
> >
> > -	caml_callbackN(*func, 3, args);
> >  	CAMLdone;
> >  	caml_enter_blocking_section();
> > -	return 0;
> > +	return ret;
> >  }
> > +	for_app = malloc(sizeof(value));
> > +	if (for_app) {
> > +		*for_app = caml_callbackN(*func, 4, args);
> 
> Can the callbackN fail, eg. returning an exception or something?
> > @@ -1315,18 +1354,34 @@ int timeout_modify(void *user, void
> > **for_app_registration_update,  {
> >  	caml_leave_blocking_section();
> >  	CAMLparam0();
> > +	CAMLlocalN(args, 2);
> > +	int ret = 0;
> >  	static value *func = NULL;
> >  	value *p = (value *) user;
> > +	value *for_app = *for_app_registration_update;
> >
> > -	if (func == NULL) {
> > -		/* First time around, lookup by name */
> > -		func = caml_named_value("libxl_timeout_modify");
> > +	/* if for_app == NULL, assume that something is wrong and don't
> > +callback */
> 
> Same comment as the other occasion.
> 

Re: [PATCH 3/3] libxl: ocaml: use 'for_app_registration' in osevent callbacks

[Ian Campbell]

On Thu, 2013-12-12 at 17:54 +0000, Rob Hoes wrote:
> Ian Campbell wrote:
> > > @@ -1241,32 +1255,43 @@ int fd_modify(void *user, int fd, void
> > > **for_app_registration_update,  {
> > >  	caml_leave_blocking_section();
> > >  	CAMLparam0();
> > > -	CAMLlocalN(args, 3);
> > > +	CAMLlocalN(args, 4);
> > > +	int ret = 0;
> > >  	static value *func = NULL;
> > >  	value *p = (value *) user;
> > > +	value *for_app = *for_app_registration_update;
> > >
> > > -	if (func == NULL) {
> > > -		/* First time around, lookup by name */
> > > -		func = caml_named_value("libxl_fd_modify");
> > > -	}
> > > +	/* if for_app == NULL, assume that something is wrong and don't
> > callback */
> > > +	if (for_app) {
> > 
> > if (!for_app) {
> > 	cleanup
> > 	return;
> > }
> > 
> > Would be more natural and save perturbing the rest of the function so much.
> > If the cleanup is the same as the tail of the function then the goto style
> > of error handling is good too:
> 
> I think this is a matter or taste... To me, using goto isn't natural
> at all! I find the control flow a little clearer the way I did it.

The benefit of the goto style is that you consolidate all of the
function epilogue into once place rather than having to repeat it and
risk one half getting out of sync.
[...]
> > Actually, elsewhere you don't bother to check for_app. If this would
> > indicate a major error then I think an assert would be perfectly
> > reasonably.
> 
> I think the check is missing from fd_deregister, indeed. I need to add
> it there.
> 
> If it's an assert, I believe it would kill the whole application? I
> think that's a little too much. I think that returning
> ERROR_OSEVENT_REG_FAIL would be the right thing here, because I
> believe that libxl would send it back to the application and just fail
> the current operation.

Under what circumstances can for_app ever be NULL? If it indicates a
programming error in the bindings themselves (which I think it does?)
then an assert is appropriate, since it should never happen.

If it indicates an error in the caller of the bindings or something
which can happen for some reason unrelated to the code itself (e.g some
external circumstance) then the error return would be appropriate.

Ian.

Re: [PATCH 3/3] libxl: ocaml: use 'for_app_registration' in osevent callbacks

[Rob Hoes]

On 12 Dec 2013, at 19:18, Ian Campbell <Ian.Campbell@citrix.com> wrote:

> On Thu, 2013-12-12 at 17:54 +0000, Rob Hoes wrote:
>> Ian Campbell wrote:
>>>> @@ -1241,32 +1255,43 @@ int fd_modify(void *user, int fd, void
>>>> **for_app_registration_update,  {
>>>> 	caml_leave_blocking_section();
>>>> 	CAMLparam0();
>>>> -	CAMLlocalN(args, 3);
>>>> +	CAMLlocalN(args, 4);
>>>> +	int ret = 0;
>>>> 	static value *func = NULL;
>>>> 	value *p = (value *) user;
>>>> +	value *for_app = *for_app_registration_update;
>>>> 
>>>> -	if (func == NULL) {
>>>> -		/* First time around, lookup by name */
>>>> -		func = caml_named_value("libxl_fd_modify");
>>>> -	}
>>>> +	/* if for_app == NULL, assume that something is wrong and don't
>>> callback */
>>>> +	if (for_app) {
>>> 
>>> if (!for_app) {
>>> 	cleanup
>>> 	return;
>>> }
>>> 
>>> Would be more natural and save perturbing the rest of the function so much.
>>> If the cleanup is the same as the tail of the function then the goto style
>>> of error handling is good too:
>> 
>> I think this is a matter or taste... To me, using goto isn't natural
>> at all! I find the control flow a little clearer the way I did it.
> 
> The benefit of the goto style is that you consolidate all of the
> function epilogue into once place rather than having to repeat it and
> risk one half getting out of sync.

Agreed, but I think my version had that property as well. Anyway, it looks like an assert is the way to go in this case (see below) :)

> [...]
>>> Actually, elsewhere you don't bother to check for_app. If this would
>>> indicate a major error then I think an assert would be perfectly
>>> reasonably.
>> 
>> I think the check is missing from fd_deregister, indeed. I need to add
>> it there.
>> 
>> If it's an assert, I believe it would kill the whole application? I
>> think that's a little too much. I think that returning
>> ERROR_OSEVENT_REG_FAIL would be the right thing here, because I
>> believe that libxl would send it back to the application and just fail
>> the current operation.
> 
> Under what circumstances can for_app ever be NULL? If it indicates a
> programming error in the bindings themselves (which I think it does?)
> then an assert is appropriate, since it should never happen.
> 
> If it indicates an error in the caller of the bindings or something
> which can happen for some reason unrelated to the code itself (e.g some
> external circumstance) then the error return would be appropriate.

For the modify and deregister functions, I’d consider it a programming error if for_app is NULL (either in the bindings or libxl itself). This is because the register functions will always get a value passed in from the OCaml app (even if it is just a unit “()”), and this value is expected to be delivered to subsequent (related) modify or deregister calls.

Following your reasoning, we should use an assert for this.

For the register functions, for_app_registration_out is NULL on entry (according to libxl_event.h), but this is ignored by the bindings. There is an “if (for_app)” there to check the outcome of malloc. If malloc returns NULL, then the second category applies (external circumstance). According to the docs, a registration of modification hook may return ERROR_OSEVENT_REG_FAIL or any positive int in case it fails, which seems appropriate there.

I’ll update the patch.

Cheers,
Rob

Re: [PATCH 3/3] libxl: ocaml: use 'for_app_registration' in osevent callbacks

[Ian Campbell]

On Thu, 2013-12-12 at 21:27 +0000, Rob Hoes wrote:
> > If it indicates an error in the caller of the bindings or something
> > which can happen for some reason unrelated to the code itself (e.g some
> > external circumstance) then the error return would be appropriate.
> 
> For the modify and deregister functions, I’d consider it a programming
> error if for_app is NULL (either in the bindings or libxl itself).
> This is because the register functions will always get a value passed
> in from the OCaml app (even if it is just a unit “()”), and this value
> is expected to be delivered to subsequent (related) modify or
> deregister calls.
> 
> Following your reasoning, we should use an assert for this.

Agreed.

> For the register functions, for_app_registration_out is NULL on entry
> (according to libxl_event.h), but this is ignored by the bindings.
> There is an “if (for_app)” there to check the outcome of malloc. If
> malloc returns NULL, then the second category applies (external
> circumstance). According to the docs, a registration of modification
> hook may return ERROR_OSEVENT_REG_FAIL or any positive int in case it
> fails, which seems appropriate there.

Right, I should have been clearer that I was talking about the other
case and not the allocation failures.

> I’ll update the patch.

Thanks,
Ian.



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

Re: [PATCH 3/3] libxl: ocaml: use 'for_app_registration' in osevent callbacks

[Rob Hoes]

I forgot to reply to this:

On 12 Dec 2013, at 17:40, Ian Campbell <Ian.Campbell@citrix.com> wrote:
>> [...]
>> +	for_app = malloc(sizeof(value));
>> +	if (for_app) {
>> +		*for_app = caml_callbackN(*func, 4, args);
> 
> Can the callbackN fail, eg. returning an exception or something?

Yes, it can:

"If the function f does not return, but raises an exception that escapes the scope of the application, then this exception is propagated to the next enclosing OCaml code, skipping over the C code. That is, if an OCaml function f calls a C function g that calls back an OCaml function h that raises a stray exception, then the execution of g is interrupted and the exception is propagated back into f.” [http://caml.inria.fr/pub/docs/manual-ocaml-4.00/manual033.html#toc148]

It is possible to catch the exception in C, e.g.:

res = caml_callbackN_exn(*func, 4, args);
if(Is_exception_result(res)) {
        res = Extract_exception(res);
        ...

Perhaps it makes sense to do this, and return ERROR_OSEVENT_REG_FAIL when we catch an exception.

Cheers,
Rob

Re: [PATCH 3/3] libxl: ocaml: use 'for_app_registration' in osevent callbacks

[Ian Jackson]

Rob Hoes writes ("Re: [PATCH 3/3] libxl: ocaml: use 'for_app_registration' in osevent callbacks"):
> I forgot to reply to this:
> 
> On 12 Dec 2013, at 17:40, Ian Campbell <Ian.Campbell@citrix.com> wrote:
> >> [...]
> >> +	for_app = malloc(sizeof(value));
> >> +	if (for_app) {
> >> +		*for_app = caml_callbackN(*func, 4, args);
> > 
> > Can the callbackN fail, eg. returning an exception or something?
> 
> Yes, it can:
> 
> "If the function f does not return, but raises an exception that escapes the scope of the application, then this exception is propagated to the next enclosing OCaml code, skipping over the C code. That is, if an OCaml function f calls a C function g that calls back an OCaml function h that raises a stray exception, then the execution of g is interrupted and the exception is propagated back into f.” [http://caml.inria.fr/pub/docs/manual-ocaml-4.00/manual033.html#toc148]

How alarming.  I think this will therefore skip over all the function
epilogue and leave various stuff (including stuff on the now-unwould
stack frame) wired into the ocaml gc.

And of course it will unwind past libxl, leaving the libxl ctx lock
held.

> It is possible to catch the exception in C, e.g.:
> 
> res = caml_callbackN_exn(*func, 4, args);
> if(Is_exception_result(res)) {
>         res = Extract_exception(res);
>         ...
> 
> Perhaps it makes sense to do this, and return ERROR_OSEVENT_REG_FAIL when we catch an exception.

Yes, absolutely, you must do that.  Although the results are still not
going to be good.  In the worst case libxl will signal a disaster.

I didn't write in lixbl_event.h that the callbacks weren't allowed to
longjmp rather than returning because I thought it was obvious.

Ian.

[PATCH v2 3/3] libxl: ocaml: use 'for_app_registration' in osevent callbacks

[Rob Hoes]

This allows the application to pass a token to libxl in the fd/timeout
registration callbacks, which it receives back in modification or
deregistration callbacks.

It turns out that this is essential for timeout handling, in order to
identify which timeout to change on a modify event.

Signed-off-by: Rob Hoes <rob.hoes@citrix.com>

---
v2:
* assert if for_app == NULL
* catch any exceptions from callbacks
* use goto-style error handling ;)
---
 tools/ocaml/libs/xl/xenlight.mli.in  |   10 +--
 tools/ocaml/libs/xl/xenlight_stubs.c |  116 ++++++++++++++++++++++++++++++----
 2 files changed, 109 insertions(+), 17 deletions(-)

diff --git a/tools/ocaml/libs/xl/xenlight.mli.in b/tools/ocaml/libs/xl/xenlight.mli.in
index b9819e1..277e81d 100644
--- a/tools/ocaml/libs/xl/xenlight.mli.in
+++ b/tools/ocaml/libs/xl/xenlight.mli.in
@@ -68,11 +68,11 @@ module Async : sig
 
 	val osevent_register_hooks : ctx ->
 		user:'a ->
-		fd_register:('a -> Unix.file_descr -> event list -> for_libxl -> unit) ->
-		fd_modify:('a -> Unix.file_descr -> event list -> unit) ->
-		fd_deregister:('a -> Unix.file_descr -> unit) ->
-		timeout_register:('a -> int64 -> int64 -> for_libxl -> unit) ->
-		timeout_modify:('a -> unit) ->
+		fd_register:('a -> Unix.file_descr -> event list -> for_libxl -> 'b) ->
+		fd_modify:('a -> Unix.file_descr -> 'b -> event list -> 'b) ->
+		fd_deregister:('a -> Unix.file_descr -> 'b -> unit) ->
+		timeout_register:('a -> int64 -> int64 -> for_libxl -> 'c) ->
+		timeout_modify:('a -> 'c -> 'c) ->
 		osevent_hooks
 
 	external osevent_occurred_fd : ctx -> for_libxl -> Unix.file_descr -> event list -> event list -> unit = "stub_libxl_osevent_occurred_fd"
diff --git a/tools/ocaml/libs/xl/xenlight_stubs.c b/tools/ocaml/libs/xl/xenlight_stubs.c
index 2e2606a..997cbe6 100644
--- a/tools/ocaml/libs/xl/xenlight_stubs.c
+++ b/tools/ocaml/libs/xl/xenlight_stubs.c
@@ -31,6 +31,7 @@
 #include <libxl_utils.h>
 
 #include <unistd.h>
+#include <assert.h>
 
 #include "caml_xentoollog.h"
 
@@ -1211,14 +1212,20 @@ value Val_poll_events(short events)
 	CAMLreturn(event_list);
 }
 
+/* The process for dealing with the for_app_registration_  values in the
+ * callbacks below (GC registrations etc) is similar to the way for_callback is
+ * handled in the asynchronous operations above. */
+
 int fd_register(void *user, int fd, void **for_app_registration_out,
                      short events, void *for_libxl)
 {
 	caml_leave_blocking_section();
 	CAMLparam0();
 	CAMLlocalN(args, 4);
+	int ret = 0;
 	static value *func = NULL;
 	value *p = (value *) user;
+	value *for_app;
 
 	if (func == NULL) {
 		/* First time around, lookup by name */
@@ -1230,10 +1237,25 @@ int fd_register(void *user, int fd, void **for_app_registration_out,
 	args[2] = Val_poll_events(events);
 	args[3] = (value) for_libxl;
 
-	caml_callbackN(*func, 4, args);
+	for_app = malloc(sizeof(value));
+	if (!for_app) {
+		ret = ERROR_OSEVENT_REG_FAIL;
+		goto err;
+	}
+
+	*for_app = caml_callbackN_exn(*func, 4, args);
+	if (Is_exception_result(*for_app)) {
+		ret = ERROR_OSEVENT_REG_FAIL;
+		goto err;
+	}
+
+	caml_register_global_root(for_app);
+	*for_app_registration_out = for_app;
+
+err:
 	CAMLdone;
 	caml_enter_blocking_section();
-	return 0;
+	return ret;
 }
 
 int fd_modify(void *user, int fd, void **for_app_registration_update,
@@ -1241,9 +1263,14 @@ int fd_modify(void *user, int fd, void **for_app_registration_update,
 {
 	caml_leave_blocking_section();
 	CAMLparam0();
-	CAMLlocalN(args, 3);
+	CAMLlocalN(args, 4);
+	int ret = 0;
 	static value *func = NULL;
 	value *p = (value *) user;
+	value *for_app = *for_app_registration_update;
+
+	/* If for_app == NULL, assume that something is wrong */
+	assert(for_app);
 
 	if (func == NULL) {
 		/* First time around, lookup by name */
@@ -1252,21 +1279,37 @@ int fd_modify(void *user, int fd, void **for_app_registration_update,
 
 	args[0] = *p;
 	args[1] = Val_int(fd);
-	args[2] = Val_poll_events(events);
+	args[2] = *for_app;
+	args[3] = Val_poll_events(events);
+
+	*for_app = caml_callbackN_exn(*func, 4, args);
+	if (Is_exception_result(*for_app)) {
+		/* If an exception is caught, *for_app_registration_update is not
+		 * changed. It remains a valid pointer to a value that is registered
+		 * with the GC. */
+		ret = ERROR_OSEVENT_REG_FAIL;
+		goto err;
+	}
+
+	*for_app_registration_update = for_app;
 
-	caml_callbackN(*func, 3, args);
+err:
 	CAMLdone;
 	caml_enter_blocking_section();
-	return 0;
+	return ret;
 }
 
 void fd_deregister(void *user, int fd, void *for_app_registration)
 {
 	caml_leave_blocking_section();
 	CAMLparam0();
-	CAMLlocalN(args, 2);
+	CAMLlocalN(args, 3);
 	static value *func = NULL;
 	value *p = (value *) user;
+	value *for_app = for_app_registration;
+
+	/* If for_app == NULL, assume that something is wrong */
+	assert(for_app);
 
 	if (func == NULL) {
 		/* First time around, lookup by name */
@@ -1275,8 +1318,15 @@ void fd_deregister(void *user, int fd, void *for_app_registration)
 
 	args[0] = *p;
 	args[1] = Val_int(fd);
+	args[2] = *for_app;
+
+	caml_callbackN_exn(*func, 3, args);
+	/* If the callback were to raise an exception, this will be ignored;
+	 * this hook does not return error codes */
+
+	caml_remove_global_root(for_app);
+	free(for_app);
 
-	caml_callbackN(*func, 2, args);
 	CAMLdone;
 	caml_enter_blocking_section();
 }
@@ -1288,8 +1338,10 @@ int timeout_register(void *user, void **for_app_registration_out,
 	CAMLparam0();
 	CAMLlocal2(sec, usec);
 	CAMLlocalN(args, 4);
+	int ret = 0;
 	static value *func = NULL;
 	value *p = (value *) user;
+	value *for_app;
 
 	if (func == NULL) {
 		/* First time around, lookup by name */
@@ -1304,10 +1356,25 @@ int timeout_register(void *user, void **for_app_registration_out,
 	args[2] = usec;
 	args[3] = (value) for_libxl;
 
-	caml_callbackN(*func, 4, args);
+	for_app = malloc(sizeof(value));
+	if (!for_app) {
+		ret = ERROR_OSEVENT_REG_FAIL;
+		goto err;
+	}
+
+	*for_app = caml_callbackN_exn(*func, 4, args);
+	if (Is_exception_result(*for_app)) {
+		ret = ERROR_OSEVENT_REG_FAIL;
+		goto err;
+	}
+
+	caml_register_global_root(for_app);
+	*for_app_registration_out = for_app;
+
+err:
 	CAMLdone;
 	caml_enter_blocking_section();
-	return 0;
+	return ret;
 }
 
 int timeout_modify(void *user, void **for_app_registration_update,
@@ -1315,18 +1382,43 @@ int timeout_modify(void *user, void **for_app_registration_update,
 {
 	caml_leave_blocking_section();
 	CAMLparam0();
+	CAMLlocalN(args, 2);
+	int ret = 0;
 	static value *func = NULL;
 	value *p = (value *) user;
+	value *for_app = *for_app_registration_update;
+
+	/* If for_app == NULL, assume that something is wrong */
+	assert(for_app);
 
 	if (func == NULL) {
 		/* First time around, lookup by name */
 		func = caml_named_value("libxl_timeout_modify");
 	}
 
-	caml_callback(*func, *p);
+	args[0] = *p;
+	args[1] = *for_app;
+
+	*for_app = caml_callbackN_exn(*func, 2, args);
+
+	if (Is_exception_result(*for_app)) {
+		/* If an exception is caught, *for_app_registration_update is not
+		 * changed. It remains a valid pointer to a value that is registered
+		 * with the GC. */
+		ret = ERROR_OSEVENT_REG_FAIL;
+		goto err;
+	}
+
+	/* This modify hook causes the timeout to fire immediately. Deregister
+	 * won't be called, so we clean up our GC registration here. */
+	caml_remove_global_root(for_app);
+	free(for_app);
+	*for_app_registration_update = NULL;
+
+err:
 	CAMLdone;
 	caml_enter_blocking_section();
-	return 0;
+	return ret;
 }
 
 void timeout_deregister(void *user, void *for_app_registration)
-- 
1.7.10.4

Re: [PATCH v2 3/3] libxl: ocaml: use 'for_app_registration' in osevent callbacks

[Ian Jackson]

Rob Hoes writes ("[PATCH v2 3/3] libxl: ocaml: use 'for_app_registration' in osevent callbacks"):
> This allows the application to pass a token to libxl in the fd/timeout
> registration callbacks, which it receives back in modification or
> deregistration callbacks.
...
> +	*for_app = caml_callbackN_exn(*func, 4, args);
> +	if (Is_exception_result(*for_app)) {
> +		ret = ERROR_OSEVENT_REG_FAIL;
> +		goto err;
> +	}
> +
> +	caml_register_global_root(for_app);
> +	*for_app_registration_out = for_app;
> +
> +err:
>  	CAMLdone;
>  	caml_enter_blocking_section();
> -	return 0;
> +	return ret;

Doesn't this have the effect of throwing away the exception
information ?  Perhaps it should be logged somewhere.

Ian.

Re: [PATCH v2 3/3] libxl: ocaml: use 'for_app_registration' in osevent callbacks

[Rob Hoes]

On 13 Dec 2013, at 17:24, Ian Jackson <Ian.Jackson@eu.citrix.com> wrote:
> Rob Hoes writes ("[PATCH v2 3/3] libxl: ocaml: use 'for_app_registration' in osevent callbacks"):
>> This allows the application to pass a token to libxl in the fd/timeout
>> registration callbacks, which it receives back in modification or
>> deregistration callbacks.
> ...
>> +	*for_app = caml_callbackN_exn(*func, 4, args);
>> +	if (Is_exception_result(*for_app)) {
>> +		ret = ERROR_OSEVENT_REG_FAIL;
>> +		goto err;
>> +	}
>> +
>> +	caml_register_global_root(for_app);
>> +	*for_app_registration_out = for_app;
>> +
>> +err:
>> 	CAMLdone;
>> 	caml_enter_blocking_section();
>> -	return 0;
>> +	return ret;
> 
> Doesn't this have the effect of throwing away the exception
> information ?  Perhaps it should be logged somewhere.

That’s true.

What would be the best way to do that? Can we get the logger from the context in these stubs? If not, it’s going to be a little more complicated... If we can, then still we need to add the ctx to the “user” value that is passed around.

Rob

Re: [PATCH v2 3/3] libxl: ocaml: use 'for_app_registration' in osevent callbacks

[Ian Jackson]

Rob Hoes writes ("Re: [PATCH v2 3/3] libxl: ocaml: use 'for_app_registration' in osevent callbacks"):
> On 13 Dec 2013, at 17:24, Ian Jackson <Ian.Jackson@eu.citrix.com> wrote:
> > Doesn't this have the effect of throwing away the exception
> > information ?  Perhaps it should be logged somewhere.
> 
> That’s true.
> 
> What would be the best way to do that? Can we get the logger from the context in these stubs? If not, it’s going to be a little more complicated... If we can, then still we need to add the ctx to the “user” value that is passed around.

No, you can't read the logger handle out of libxl.  But you can keep a
copy yourself.

Ian.

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

Re: [PATCH v2 3/3] libxl: ocaml: use 'for_app_registration' in osevent callbacks

[Rob Hoes]

On 13 Dec 2013, at 18:21, Ian Jackson <Ian.Jackson@eu.citrix.com> wrote:
> Rob Hoes writes ("Re: [PATCH v2 3/3] libxl: ocaml: use 'for_app_registration' in osevent callbacks"):
>> On 13 Dec 2013, at 17:24, Ian Jackson <Ian.Jackson@eu.citrix.com> wrote:
>>> Doesn't this have the effect of throwing away the exception
>>> information ?  Perhaps it should be logged somewhere.
>> 
>> That’s true.
>> 
>> What would be the best way to do that? Can we get the logger from the context in these stubs? If not, it’s going to be a little more complicated... If we can, then still we need to add the ctx to the “user” value that is passed around.
> 
> No, you can't read the logger handle out of libxl.  But you can keep a
> copy yourself.

That’s going to be a larger change to the bindings than I was hoping. At this point, I’d prefer to postpone this, and do it as a improvement later on.

Cheers,
Rob

Re: [PATCH v2 3/3] libxl: ocaml: use 'for_app_registration' in osevent callbacks

[Ian Jackson]

Rob Hoes writes ("Re: [PATCH v2 3/3] libxl: ocaml: use 'for_app_registration' in osevent callbacks"):
> > No, you can't read the logger handle out of libxl.  But you can keep a
> > copy yourself.
> 
> That’s going to be a larger change to the bindings than I was hoping. At this point, I’d prefer to postpone this, and do it as a improvement later on.

OK.

Ian.

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

Re: [PATCH v2 3/3] libxl: ocaml: use 'for_app_registration' in osevent callbacks

[David Scott]

On 16/12/13 12:02, Ian Jackson wrote:
> Rob Hoes writes ("Re: [PATCH v2 3/3] libxl: ocaml: use 'for_app_registration' in osevent callbacks"):
>>> No, you can't read the logger handle out of libxl.  But you can keep a
>>> copy yourself.
>>
>> That’s going to be a larger change to the bindings than I was hoping. At this point, I’d prefer to postpone this, and do it as a improvement later on.
>
> OK.

This is also fine by me.

Acked-by: David Scott <dave.scott@eu.citrix.com>


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

Re: [PATCH v2 3/3] libxl: ocaml: use 'for_app_registration' in osevent callbacks

[Ian Campbell]

On Fri, 2013-12-13 at 18:04 +0000, Rob Hoes wrote:
> On 13 Dec 2013, at 17:24, Ian Jackson <Ian.Jackson@eu.citrix.com> wrote:
> > Rob Hoes writes ("[PATCH v2 3/3] libxl: ocaml: use 'for_app_registration' in osevent callbacks"):
> >> This allows the application to pass a token to libxl in the fd/timeout
> >> registration callbacks, which it receives back in modification or
> >> deregistration callbacks.
> > ...
> >> +	*for_app = caml_callbackN_exn(*func, 4, args);
> >> +	if (Is_exception_result(*for_app)) {
> >> +		ret = ERROR_OSEVENT_REG_FAIL;
> >> +		goto err;
> >> +	}
> >> +
> >> +	caml_register_global_root(for_app);
> >> +	*for_app_registration_out = for_app;
> >> +
> >> +err:
> >> 	CAMLdone;
> >> 	caml_enter_blocking_section();
> >> -	return 0;
> >> +	return ret;
> > 
> > Doesn't this have the effect of throwing away the exception
> > information ?  Perhaps it should be logged somewhere.
> 
> That’s true.

*for_app eventually ends up in a caller which is in the C bindings,
doesn't it? Can it not be picked up there and reused with caml_raise
after the appropriate C level cleanup?

> What would be the best way to do that? Can we get the logger from the
> context in these stubs? If not, it’s going to be a little more
> complicated... If we can, then still we need to add the ctx to the
> “user” value that is passed around.
> 
> Rob



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

Re: [PATCH v2 3/3] libxl: ocaml: use 'for_app_registration' in osevent callbacks

[Ian Campbell]

On Mon, 2013-12-16 at 11:35 +0000, Ian Campbell wrote:
> On Fri, 2013-12-13 at 18:04 +0000, Rob Hoes wrote:
> > On 13 Dec 2013, at 17:24, Ian Jackson <Ian.Jackson@eu.citrix.com> wrote:
> > > Rob Hoes writes ("[PATCH v2 3/3] libxl: ocaml: use 'for_app_registration' in osevent callbacks"):
> > >> This allows the application to pass a token to libxl in the fd/timeout
> > >> registration callbacks, which it receives back in modification or
> > >> deregistration callbacks.
> > > ...
> > >> +	*for_app = caml_callbackN_exn(*func, 4, args);
> > >> +	if (Is_exception_result(*for_app)) {
> > >> +		ret = ERROR_OSEVENT_REG_FAIL;
> > >> +		goto err;
> > >> +	}
> > >> +
> > >> +	caml_register_global_root(for_app);
> > >> +	*for_app_registration_out = for_app;
> > >> +
> > >> +err:
> > >> 	CAMLdone;
> > >> 	caml_enter_blocking_section();
> > >> -	return 0;
> > >> +	return ret;
> > > 
> > > Doesn't this have the effect of throwing away the exception
> > > information ?  Perhaps it should be logged somewhere.
> > 
> > That’s true.
> 
> *for_app eventually ends up in a caller which is in the C bindings,
> doesn't it? Can it not be picked up there and reused with caml_raise
> after the appropriate C level cleanup?

Nevermind -- that eventual caller is just some random call down into
libxl...

> 
> > What would be the best way to do that? Can we get the logger from the
> > context in these stubs? If not, it’s going to be a little more
> > complicated... If we can, then still we need to add the ctx to the
> > “user” value that is passed around.
> > 
> > Rob
> 
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel



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

Re: [PATCH v2 3/3] libxl: ocaml: use 'for_app_registration' in osevent callbacks

[Ian Jackson]

Rob Hoes writes ("[PATCH v2 3/3] libxl: ocaml: use 'for_app_registration' in osevent callbacks"):
> This allows the application to pass a token to libxl in the fd/timeout
> registration callbacks, which it receives back in modification or
> deregistration callbacks.
> 
> It turns out that this is essential for timeout handling, in order to
> identify which timeout to change on a modify event.
> 
> Signed-off-by: Rob Hoes <rob.hoes@citrix.com>
...
> -	caml_callbackN(*func, 4, args);
> +	for_app = malloc(sizeof(value));
> +	if (!for_app) {
> +		ret = ERROR_OSEVENT_REG_FAIL;
> +		goto err;
> +	}
> +
> +	*for_app = caml_callbackN_exn(*func, 4, args);
> +	if (Is_exception_result(*for_app)) {
> +		ret = ERROR_OSEVENT_REG_FAIL;
> +		goto err;
> +	}
> +
> +	caml_register_global_root(for_app);
> +	*for_app_registration_out = for_app;

I expect you have thought this through properly, and perhaps even
explained it already, but: is the ordering of these operations
(particularly, of the caml_register_global_root) guaranteed to be
correct ?

Eg, can Is_exception_result call the gc ?

>  int fd_modify(void *user, int fd, void **for_app_registration_update,
> @@ -1241,9 +1263,14 @@ int fd_modify(void *user, int fd, void **for_app_registration_update,
>  {
...
> +	/* If for_app == NULL, assume that something is wrong */
> +	assert(for_app);

While I'm reading this in detail, this is a slightly odd wording for
the comment, now that this is an assertion.  You probably mean
something like "If for_app == NULL, something is very wrong".
(Another occurrence of this later.)

>  void fd_deregister(void *user, int fd, void *for_app_registration)
>  {
...
> +	caml_callbackN_exn(*func, 3, args);
> +	/* If the callback were to raise an exception, this will be ignored;
> +	 * this hook does not return error codes */

Can you not do anything better here ?  I think crashing the whole
application would be better than carrying on and later calling back
into libxl with a stale for_libxl pointer!

> -	caml_callbackN(*func, 4, args);
> +	for_app = malloc(sizeof(value));
> +	if (!for_app) {
> +		ret = ERROR_OSEVENT_REG_FAIL;
> +		goto err;
> +	}
> +
> +	*for_app = caml_callbackN_exn(*func, 4, args);
> +	if (Is_exception_result(*for_app)) {
> +		ret = ERROR_OSEVENT_REG_FAIL;
> +		goto err;
> +	}
> +
> +	caml_register_global_root(for_app);
> +	*for_app_registration_out = for_app;

Aren't these functions getting incredibly formulaic ?  I guess it is
too late for 4.4 but if possible, later, I would like to see the
common stuff factored out.

>  int timeout_modify(void *user, void **for_app_registration_update,
> @@ -1315,18 +1382,43 @@ int timeout_modify(void *user, void **for_app_registration_update,
>  {
>  	caml_leave_blocking_section();
>  	CAMLparam0();
> +	CAMLlocalN(args, 2);
> +	int ret = 0;
...
> +	/* This modify hook causes the timeout to fire immediately. Deregister
> +	 * won't be called, so we clean up our GC registration here. */
> +	caml_remove_global_root(for_app);
> +	free(for_app);
> +	*for_app_registration_update = NULL;

This can't be right, because what the timeout modify callback is
supposed to do is arrange for stub_libxl_osevent_occurred_timeout to
be called.

And looking at that, I see that stub_libxl_osevent_occurred_timeout
doesn't destroy the for_app.

Ian.

Re: [PATCH v2 3/3] libxl: ocaml: use 'for_app_registration' in osevent callbacks

[Rob Hoes]

Ian Jackson wrote:
> Rob Hoes writes ("[PATCH v2 3/3] libxl: ocaml: use 'for_app_registration'
> in osevent callbacks"):
> > This allows the application to pass a token to libxl in the fd/timeout
> > registration callbacks, which it receives back in modification or
> > deregistration callbacks.
> >
> > It turns out that this is essential for timeout handling, in order to
> > identify which timeout to change on a modify event.
> >
> > Signed-off-by: Rob Hoes <rob.hoes@citrix.com>
> ...
> > -	caml_callbackN(*func, 4, args);
> > +	for_app = malloc(sizeof(value));
> > +	if (!for_app) {
> > +		ret = ERROR_OSEVENT_REG_FAIL;
> > +		goto err;
> > +	}
> > +
> > +	*for_app = caml_callbackN_exn(*func, 4, args);
> > +	if (Is_exception_result(*for_app)) {
> > +		ret = ERROR_OSEVENT_REG_FAIL;
> > +		goto err;
> > +	}
> > +
> > +	caml_register_global_root(for_app);
> > +	*for_app_registration_out = for_app;
> 
> I expect you have thought this through properly, and perhaps even
> explained it already, but: is the ordering of these operations
> (particularly, of the caml_register_global_root) guaranteed to be
> correct ?
> 
> Eg, can Is_exception_result call the gc ?

This macro is defined as follows:

    #define Is_exception_result(v) (((v) & 3) == 2)

So this won't cause any harm. I think the order is therefore correct, and since we don't want to register for_app with the GC in case of an exception (this may change if we'd try to interpret the exception and log it, later on).

> >  int fd_modify(void *user, int fd, void **for_app_registration_update,
> > @@ -1241,9 +1263,14 @@ int fd_modify(void *user, int fd, void
> > **for_app_registration_update,  {
> ...
> > +	/* If for_app == NULL, assume that something is wrong */
> > +	assert(for_app);
> 
> While I'm reading this in detail, this is a slightly odd wording for the
> comment, now that this is an assertion.  You probably mean something like
> "If for_app == NULL, something is very wrong".
> (Another occurrence of this later.)

Ok.

> >  void fd_deregister(void *user, int fd, void *for_app_registration)  {
> ...
> > +	caml_callbackN_exn(*func, 3, args);
> > +	/* If the callback were to raise an exception, this will be ignored;
> > +	 * this hook does not return error codes */
> 
> Can you not do anything better here ?  I think crashing the whole
> application would be better than carrying on and later calling back into
> libxl with a stale for_libxl pointer!

Ok, that makes sense.

> > -	caml_callbackN(*func, 4, args);
> > +	for_app = malloc(sizeof(value));
> > +	if (!for_app) {
> > +		ret = ERROR_OSEVENT_REG_FAIL;
> > +		goto err;
> > +	}
> > +
> > +	*for_app = caml_callbackN_exn(*func, 4, args);
> > +	if (Is_exception_result(*for_app)) {
> > +		ret = ERROR_OSEVENT_REG_FAIL;
> > +		goto err;
> > +	}
> > +
> > +	caml_register_global_root(for_app);
> > +	*for_app_registration_out = for_app;
> 
> Aren't these functions getting incredibly formulaic ?  I guess it is too
> late for 4.4 but if possible, later, I would like to see the common stuff
> factored out.

Yes, agreed.

> >  int timeout_modify(void *user, void **for_app_registration_update, @@
> > -1315,18 +1382,43 @@ int timeout_modify(void *user, void
> > **for_app_registration_update,  {
> >  	caml_leave_blocking_section();
> >  	CAMLparam0();
> > +	CAMLlocalN(args, 2);
> > +	int ret = 0;
> ...
> > +	/* This modify hook causes the timeout to fire immediately.
> Deregister
> > +	 * won't be called, so we clean up our GC registration here. */
> > +	caml_remove_global_root(for_app);
> > +	free(for_app);
> > +	*for_app_registration_update = NULL;
> 
> This can't be right, because what the timeout modify callback is supposed
> to do is arrange for stub_libxl_osevent_occurred_timeout to be called.
> 
> And looking at that, I see that stub_libxl_osevent_occurred_timeout
> doesn't destroy the for_app.

Hmm... I thought the for_app stuff is only for the registration bits? The osevent_occurred functions don't use or receive it? They do get for_libxl, but that's entirely in C and opaque to ocaml.

I do assume here that timeout_modify will be called only once for a given timeout registration. Is that correct?

I'll send an update for the comment and exception thing mentioned above.

Cheers,
Rob

Re: [PATCH v2 3/3] libxl: ocaml: use 'for_app_registration' in osevent callbacks

[Ian Jackson]

Rob Hoes writes ("RE: [PATCH v2 3/3] libxl: ocaml: use 'for_app_registration' in osevent callbacks"):
> Ian Jackson wrote:
> > And looking at that, I see that stub_libxl_osevent_occurred_timeout
> > doesn't destroy the for_app.
> 
> Hmm... I thought the for_app stuff is only for the registration
> bits? The osevent_occurred functions don't use or receive it? They
> do get for_libxl, but that's entirely in C and opaque to ocaml.

The usual sequence of events is
  timeout_register
      with your new patch:
           stashes for_libxl value in ocaml gc
           calls ocaml libxl_timeout_register with for_libxl
           stashes that function's return in for_app and adds it to the gc
  timeout occurs
      the timeout machinery calls stub_libxl_osevent_occurred_timeout
          with the for_libxl value it has kept somehow
      stub_libxl_osevent_occurred_timeout calls
          libxl_osevent_occurred_timeout

Now the timeout is gone and nothing will deal with it again.  Who
cleans up the for_app value ?

Perhaps you are confused and don't realise that timeouts are one-shot.
See the comment next to libxl_osevent_occurred_timeout.

> I do assume here that timeout_modify will be called only once for a
> given timeout registration. Is that correct?

The specification is that it may be called more than once, or not at
all.  The cleanup needs to be done in
stub_libxl_osevent_occurred_timeout.

(And you don't probably don't want a binding for timeout_deregister.
That's only there for compatibility with what are now old libxls, and
only if those libxls don't have the race patches which are necessary
for reliable operation.)

Ian.

Re: [PATCH v2 3/3] libxl: ocaml: use 'for_app_registration' in osevent callbacks

[Rob Hoes]

Ian Jackson wrote:
> Rob Hoes writes ("RE: [PATCH v2 3/3] libxl: ocaml: use
> 'for_app_registration' in osevent callbacks"):
> > Ian Jackson wrote:
> > > And looking at that, I see that stub_libxl_osevent_occurred_timeout
> > > doesn't destroy the for_app.
> >
> > Hmm... I thought the for_app stuff is only for the registration bits?
> > The osevent_occurred functions don't use or receive it? They do get
> > for_libxl, but that's entirely in C and opaque to ocaml.
> 
> The usual sequence of events is
>   timeout_register
>       with your new patch:
>            stashes for_libxl value in ocaml gc
>            calls ocaml libxl_timeout_register with for_libxl
>            stashes that function's return in for_app and adds it to the gc
>   timeout occurs
>       the timeout machinery calls stub_libxl_osevent_occurred_timeout
>           with the for_libxl value it has kept somehow
>       stub_libxl_osevent_occurred_timeout calls
>           libxl_osevent_occurred_timeout
> 
> Now the timeout is gone and nothing will deal with it again.  Who cleans
> up the for_app value ?
> 
> Perhaps you are confused and don't realise that timeouts are one-shot.
> See the comment next to libxl_osevent_occurred_timeout.

One part of my brain knew that, but another part wrote this function... :)

I'll send an update.

> > I do assume here that timeout_modify will be called only once for a
> > given timeout registration. Is that correct?
> 
> The specification is that it may be called more than once, or not at all.
> The cleanup needs to be done in stub_libxl_osevent_occurred_timeout.
> 
> (And you don't probably don't want a binding for timeout_deregister.
> That's only there for compatibility with what are now old libxls, and only
> if those libxls don't have the race patches which are necessary for
> reliable operation.)

It is already not there on the ocaml side for this reason. There is just a stub that raises an error, which is given to osevent_register_hooks (just to be sure). I should probably just put an abort in there rather than it raising an ocaml exception as it does now...

Cheers,
Rob

Re: [PATCH v2 3/3] libxl: ocaml: use 'for_app_registration' in osevent callbacks

[Ian Jackson]

Rob Hoes writes ("RE: [PATCH v2 3/3] libxl: ocaml: use 'for_app_registration' in osevent callbacks"):
> Ian Jackson wrote:
> > Perhaps you are confused and don't realise that timeouts are one-shot.
> > See the comment next to libxl_osevent_occurred_timeout.
> 
> One part of my brain knew that, but another part wrote this function... :)
> 
> I'll send an update.

Ah :-).  Thanks.

> > (And you don't probably don't want a binding for timeout_deregister.
> > That's only there for compatibility with what are now old libxls, and only
> > if those libxls don't have the race patches which are necessary for
> > reliable operation.)
> 
> It is already not there on the ocaml side for this reason. There is just a stub that raises an error, which is given to osevent_register_hooks (just to be sure). I should probably just put an abort in there rather than it raising an ocaml exception as it does now...

Oh, I didn't look closely enough.  But, yes, an abort is probably
better.

Ian.

[PATCH v3 3/3] libxl: ocaml: use 'for_app_registration' in osevent callbacks

[Rob Hoes]

This allows the application to pass a token to libxl in the fd/timeout
registration callbacks, which it receives back in modification or
deregistration callbacks.

It turns out that this is essential for timeout handling, in order to
identify which timeout to change on a modify event.

Signed-off-by: Rob Hoes <rob.hoes@citrix.com>
Acked-by: David Scott <dave.scott@eu.citrix.com>

---
v2:
* assert if for_app == NULL
* catch any exceptions from callbacks
* use goto-style error handling ;)

v3:
* for timeouts, cleanup for_app in occurred_timeout (not in
  timeout_deregister)
* improve comments
* abort in fd_deregister when the app raises an exception
---
 tools/ocaml/libs/xl/xenlight.mli.in  |   10 +--
 tools/ocaml/libs/xl/xenlight_stubs.c |  146 +++++++++++++++++++++++++++++-----
 2 files changed, 133 insertions(+), 23 deletions(-)

diff --git a/tools/ocaml/libs/xl/xenlight.mli.in b/tools/ocaml/libs/xl/xenlight.mli.in
index b9819e1..277e81d 100644
--- a/tools/ocaml/libs/xl/xenlight.mli.in
+++ b/tools/ocaml/libs/xl/xenlight.mli.in
@@ -68,11 +68,11 @@ module Async : sig
 
 	val osevent_register_hooks : ctx ->
 		user:'a ->
-		fd_register:('a -> Unix.file_descr -> event list -> for_libxl -> unit) ->
-		fd_modify:('a -> Unix.file_descr -> event list -> unit) ->
-		fd_deregister:('a -> Unix.file_descr -> unit) ->
-		timeout_register:('a -> int64 -> int64 -> for_libxl -> unit) ->
-		timeout_modify:('a -> unit) ->
+		fd_register:('a -> Unix.file_descr -> event list -> for_libxl -> 'b) ->
+		fd_modify:('a -> Unix.file_descr -> 'b -> event list -> 'b) ->
+		fd_deregister:('a -> Unix.file_descr -> 'b -> unit) ->
+		timeout_register:('a -> int64 -> int64 -> for_libxl -> 'c) ->
+		timeout_modify:('a -> 'c -> 'c) ->
 		osevent_hooks
 
 	external osevent_occurred_fd : ctx -> for_libxl -> Unix.file_descr -> event list -> event list -> unit = "stub_libxl_osevent_occurred_fd"
diff --git a/tools/ocaml/libs/xl/xenlight_stubs.c b/tools/ocaml/libs/xl/xenlight_stubs.c
index 2e2606a..50cd223 100644
--- a/tools/ocaml/libs/xl/xenlight_stubs.c
+++ b/tools/ocaml/libs/xl/xenlight_stubs.c
@@ -31,6 +31,7 @@
 #include <libxl_utils.h>
 
 #include <unistd.h>
+#include <assert.h>
 
 #include "caml_xentoollog.h"
 
@@ -1211,14 +1212,20 @@ value Val_poll_events(short events)
 	CAMLreturn(event_list);
 }
 
+/* The process for dealing with the for_app_registration_  values in the
+ * callbacks below (GC registrations etc) is similar to the way for_callback is
+ * handled in the asynchronous operations above. */
+
 int fd_register(void *user, int fd, void **for_app_registration_out,
                      short events, void *for_libxl)
 {
 	caml_leave_blocking_section();
 	CAMLparam0();
 	CAMLlocalN(args, 4);
+	int ret = 0;
 	static value *func = NULL;
 	value *p = (value *) user;
+	value *for_app;
 
 	if (func == NULL) {
 		/* First time around, lookup by name */
@@ -1230,10 +1237,25 @@ int fd_register(void *user, int fd, void **for_app_registration_out,
 	args[2] = Val_poll_events(events);
 	args[3] = (value) for_libxl;
 
-	caml_callbackN(*func, 4, args);
+	for_app = malloc(sizeof(value));
+	if (!for_app) {
+		ret = ERROR_OSEVENT_REG_FAIL;
+		goto err;
+	}
+
+	*for_app = caml_callbackN_exn(*func, 4, args);
+	if (Is_exception_result(*for_app)) {
+		ret = ERROR_OSEVENT_REG_FAIL;
+		goto err;
+	}
+
+	caml_register_global_root(for_app);
+	*for_app_registration_out = for_app;
+
+err:
 	CAMLdone;
 	caml_enter_blocking_section();
-	return 0;
+	return ret;
 }
 
 int fd_modify(void *user, int fd, void **for_app_registration_update,
@@ -1241,9 +1263,14 @@ int fd_modify(void *user, int fd, void **for_app_registration_update,
 {
 	caml_leave_blocking_section();
 	CAMLparam0();
-	CAMLlocalN(args, 3);
+	CAMLlocalN(args, 4);
+	int ret = 0;
 	static value *func = NULL;
 	value *p = (value *) user;
+	value *for_app = *for_app_registration_update;
+
+	/* If for_app == NULL, then something is very wrong */
+	assert(for_app);
 
 	if (func == NULL) {
 		/* First time around, lookup by name */
@@ -1252,21 +1279,37 @@ int fd_modify(void *user, int fd, void **for_app_registration_update,
 
 	args[0] = *p;
 	args[1] = Val_int(fd);
-	args[2] = Val_poll_events(events);
+	args[2] = *for_app;
+	args[3] = Val_poll_events(events);
+
+	*for_app = caml_callbackN_exn(*func, 4, args);
+	if (Is_exception_result(*for_app)) {
+		/* If an exception is caught, *for_app_registration_update is not
+		 * changed. It remains a valid pointer to a value that is registered
+		 * with the GC. */
+		ret = ERROR_OSEVENT_REG_FAIL;
+		goto err;
+	}
+
+	*for_app_registration_update = for_app;
 
-	caml_callbackN(*func, 3, args);
+err:
 	CAMLdone;
 	caml_enter_blocking_section();
-	return 0;
+	return ret;
 }
 
 void fd_deregister(void *user, int fd, void *for_app_registration)
 {
 	caml_leave_blocking_section();
 	CAMLparam0();
-	CAMLlocalN(args, 2);
+	CAMLlocalN(args, 3);
 	static value *func = NULL;
 	value *p = (value *) user;
+	value *for_app = for_app_registration;
+
+	/* If for_app == NULL, then something is very wrong */
+	assert(for_app);
 
 	if (func == NULL) {
 		/* First time around, lookup by name */
@@ -1275,12 +1318,26 @@ void fd_deregister(void *user, int fd, void *for_app_registration)
 
 	args[0] = *p;
 	args[1] = Val_int(fd);
+	args[2] = *for_app;
+
+	caml_callbackN_exn(*func, 3, args);
+	/* This hook does not return error codes, so the best thing we can do
+	 * to avoid trouble, if we catch an exception from the app, is abort. */
+	if (Is_exception_result(*for_app))
+		abort();
+
+	caml_remove_global_root(for_app);
+	free(for_app);
 
-	caml_callbackN(*func, 2, args);
 	CAMLdone;
 	caml_enter_blocking_section();
 }
 
+struct for_libxl_timeout {
+	void *for_libxl;
+	value *for_app;
+};
+
 int timeout_register(void *user, void **for_app_registration_out,
                           struct timeval abs, void *for_libxl)
 {
@@ -1288,8 +1345,10 @@ int timeout_register(void *user, void **for_app_registration_out,
 	CAMLparam0();
 	CAMLlocal2(sec, usec);
 	CAMLlocalN(args, 4);
+	int ret = 0;
 	static value *func = NULL;
 	value *p = (value *) user;
+	struct for_libxl_timeout *handles;
 
 	if (func == NULL) {
 		/* First time around, lookup by name */
@@ -1299,15 +1358,41 @@ int timeout_register(void *user, void **for_app_registration_out,
 	sec = caml_copy_int64(abs.tv_sec);
 	usec = caml_copy_int64(abs.tv_usec);
 
+	/* This struct of "handles" will contain "for_libxl" as well as "for_app".
+	 * We'll give a pointer to the struct to the app, and get it back in
+	 * occurred_timeout, where we can clean it all up. */
+	handles = malloc(sizeof(*handles));
+	if (!handles) {
+		ret = ERROR_OSEVENT_REG_FAIL;
+		goto err;
+	}
+
+	handles->for_libxl = for_libxl;
+
 	args[0] = *p;
 	args[1] = sec;
 	args[2] = usec;
-	args[3] = (value) for_libxl;
+	args[3] = (value) handles;
 
-	caml_callbackN(*func, 4, args);
+	handles->for_app = malloc(sizeof(value));
+	if (!handles->for_app) {
+		ret = ERROR_OSEVENT_REG_FAIL;
+		goto err;
+	}
+
+	*(handles->for_app) = caml_callbackN_exn(*func, 4, args);
+	if (Is_exception_result(*(handles->for_app))) {
+		ret = ERROR_OSEVENT_REG_FAIL;
+		goto err;
+	}
+
+	caml_register_global_root(handles->for_app);
+	*for_app_registration_out = handles->for_app;
+
+err:
 	CAMLdone;
 	caml_enter_blocking_section();
-	return 0;
+	return ret;
 }
 
 int timeout_modify(void *user, void **for_app_registration_update,
@@ -1315,25 +1400,45 @@ int timeout_modify(void *user, void **for_app_registration_update,
 {
 	caml_leave_blocking_section();
 	CAMLparam0();
+	CAMLlocalN(args, 2);
+	int ret = 0;
 	static value *func = NULL;
 	value *p = (value *) user;
+	value *for_app = *for_app_registration_update;
+
+	/* If for_app == NULL, then something is very wrong */
+	assert(for_app);
 
 	if (func == NULL) {
 		/* First time around, lookup by name */
 		func = caml_named_value("libxl_timeout_modify");
 	}
 
-	caml_callback(*func, *p);
+	args[0] = *p;
+	args[1] = *for_app;
+
+	*for_app = caml_callbackN_exn(*func, 2, args);
+
+	if (Is_exception_result(*for_app)) {
+		/* If an exception is caught, *for_app_registration_update is not
+		 * changed. It remains a valid pointer to a value that is registered
+		 * with the GC. */
+		ret = ERROR_OSEVENT_REG_FAIL;
+		goto err;
+	}
+
+	*for_app_registration_update = for_app;
+
+err:
 	CAMLdone;
 	caml_enter_blocking_section();
-	return 0;
+	return ret;
 }
 
 void timeout_deregister(void *user, void *for_app_registration)
 {
-	caml_leave_blocking_section();
-	failwith_xl(ERROR_FAIL, "timeout_deregister not yet implemented");
-	caml_enter_blocking_section();
+	/* This hook will never be called by libxl. */
+	abort();
 }
 
 value stub_libxl_osevent_register_hooks(value ctx, value user)
@@ -1386,12 +1491,17 @@ value stub_libxl_osevent_occurred_fd(value ctx, value for_libxl, value fd,
 
 value stub_libxl_osevent_occurred_timeout(value ctx, value for_libxl)
 {
-	CAMLparam2(ctx, for_libxl);
+	CAMLparam1(ctx);
+	struct for_libxl_timeout *handles = (struct for_libxl_timeout *) for_libxl;
 
 	caml_enter_blocking_section();
-	libxl_osevent_occurred_timeout(CTX, (void *) for_libxl);
+	libxl_osevent_occurred_timeout(CTX, (void *) handles->for_libxl);
 	caml_leave_blocking_section();
 
+	caml_remove_global_root(handles->for_app);
+	free(handles->for_app);
+	free(handles);
+
 	CAMLreturn(Val_unit);
 }
 
-- 
1.7.10.4

Re: [PATCH v3 3/3] libxl: ocaml: use 'for_app_registration' in osevent callbacks

[Ian Jackson]

Rob Hoes writes ("[PATCH v3 3/3] libxl: ocaml: use 'for_app_registration' in osevent callbacks"):
> This allows the application to pass a token to libxl in the fd/timeout
> registration callbacks, which it receives back in modification or
> deregistration callbacks.
> 
> It turns out that this is essential for timeout handling, in order to
> identify which timeout to change on a modify event.
...
> +	handles = malloc(sizeof(*handles));
> +	if (!handles) {
> +		ret = ERROR_OSEVENT_REG_FAIL;
> +		goto err;
> +	}
> +
> +	handles->for_libxl = for_libxl;
> +
>  	args[0] = *p;
>  	args[1] = sec;
>  	args[2] = usec;
> -	args[3] = (value) for_libxl;
> +	args[3] = (value) handles;
>  
> -	caml_callbackN(*func, 4, args);
> +	handles->for_app = malloc(sizeof(value));

This may seem like a daft question, but why does handles contain
a value* rather than just a value ?

Also, your error paths fail to free handles (or handles->for_app,
although I'm implicitly proposing that the latter be abolished).

> +	*for_app_registration_out = handles->for_app;

I think this is counterintuitive.  Why not pass handles to
for_app_registration_out ?  I know that your timeout_modify doesn't
need handles->for_libxl, but it would seem clearer to have your shim
proxy everything neatly: i.e., to always pass its own context
("handles") to both sides.

>  int timeout_modify(void *user, void **for_app_registration_update,
> @@ -1315,25 +1400,45 @@ int timeout_modify(void *user, void **for_app_registration_update,
>  {
>  	caml_leave_blocking_section();
>  	CAMLparam0();
> +	CAMLlocalN(args, 2);
> +	int ret = 0;
>  	static value *func = NULL;
>  	value *p = (value *) user;
> +	value *for_app = *for_app_registration_update;
...
> -	caml_callback(*func, *p);
> +	args[0] = *p;
> +	args[1] = *for_app;

I see that you're relying on the promise in modern libxl.h that abs is
always {0,0} meaning "right away".

You should probably assert that.

Also, perhaps, your ocaml function should have a different name.
"libxl_timeout_modify_now" or something.  We have avoided renaming it
in the C version to avoid API churn.  Of course you may prefer to keep
the two names identical, in which case presumably this will be
addressed in the documentation.  (Um.  What documentation.)  Anyway,
I wanted you to think about this question and explicitly choose to
give it a different name, or to avoid doing so :-).

Thanks,
Ian.

Re: [PATCH v3 3/3] libxl: ocaml: use 'for_app_registration' in osevent callbacks

[Rob Hoes]

> This may seem like a daft question, but why does handles contain a value*
> rather than just a value ?

Right, the important thing is that this "value" is malloc, which is will be if it is part of the handles struct. I'll change that.

> Also, your error paths fail to free handles (or handles->for_app, although
> I'm implicitly proposing that the latter be abolished).
> 
> > +	*for_app_registration_out = handles->for_app;
> 
> I think this is counterintuitive.  Why not pass handles to
> for_app_registration_out ?  I know that your timeout_modify doesn't need
> handles->for_libxl, but it would seem clearer to have your shim proxy
> everything neatly: i.e., to always pass its own context
> ("handles") to both sides.

Ok, makes sense.

> >  int timeout_modify(void *user, void **for_app_registration_update, @@
> > -1315,25 +1400,45 @@ int timeout_modify(void *user, void
> > **for_app_registration_update,  {
> >  	caml_leave_blocking_section();
> >  	CAMLparam0();
> > +	CAMLlocalN(args, 2);
> > +	int ret = 0;
> >  	static value *func = NULL;
> >  	value *p = (value *) user;
> > +	value *for_app = *for_app_registration_update;
> ...
> > -	caml_callback(*func, *p);
> > +	args[0] = *p;
> > +	args[1] = *for_app;
> 
> I see that you're relying on the promise in modern libxl.h that abs is
> always {0,0} meaning "right away".
> 
> You should probably assert that.

Ok.

> Also, perhaps, your ocaml function should have a different name.
> "libxl_timeout_modify_now" or something.  We have avoided renaming it in
> the C version to avoid API churn.  Of course you may prefer to keep the
> two names identical, in which case presumably this will be addressed in
> the documentation.  (Um.  What documentation.)  Anyway, I wanted you to
> think about this question and explicitly choose to give it a different
> name, or to avoid doing so :-).

Yes, it is probably clearer to change the name on the ocaml side to show better what the function really does. I was thinking about "fire_now".

I'll send an update soon.

Thanks,
Rob

[PATCH v4 3/3] libxl: ocaml: use 'for_app_registration' in osevent callbacks

[Rob Hoes]

This allows the application to pass a token to libxl in the fd/timeout
registration callbacks, which it receives back in modification or
deregistration callbacks.

It turns out that this is essential for timeout handling, in order to
identify which timeout to change on a modify event.

Signed-off-by: Rob Hoes <rob.hoes@citrix.com>
Acked-by: David Scott <dave.scott@eu.citrix.com>

---
v2:
* assert if for_app == NULL
* catch any exceptions from callbacks
* use goto-style error handling ;)

v3:
* for timeouts, cleanup for_app in occurred_timeout (not in
  timeout_deregister)
* improve comments
* abort in fd_deregister when the app raises an exception

v4:
* made for_app a value inside the handles struct, rather than a value*
* ensure handles are cleaned up in the error path
* rename timeout_modify to timeout_fire_now on the ocaml side
---
 tools/ocaml/libs/xl/xenlight.ml.in   |    4 +-
 tools/ocaml/libs/xl/xenlight.mli.in  |   10 +--
 tools/ocaml/libs/xl/xenlight_stubs.c |  147 +++++++++++++++++++++++++++++-----
 3 files changed, 135 insertions(+), 26 deletions(-)

diff --git a/tools/ocaml/libs/xl/xenlight.ml.in b/tools/ocaml/libs/xl/xenlight.ml.in
index 47f3487..80e620a 100644
--- a/tools/ocaml/libs/xl/xenlight.ml.in
+++ b/tools/ocaml/libs/xl/xenlight.ml.in
@@ -68,12 +68,12 @@ module Async = struct
 	external osevent_occurred_fd : ctx -> for_libxl -> Unix.file_descr -> event list -> event list -> unit = "stub_libxl_osevent_occurred_fd"
 	external osevent_occurred_timeout : ctx -> for_libxl -> unit = "stub_libxl_osevent_occurred_timeout"
 
-	let osevent_register_hooks ctx ~user ~fd_register ~fd_modify ~fd_deregister ~timeout_register ~timeout_modify =
+	let osevent_register_hooks ctx ~user ~fd_register ~fd_modify ~fd_deregister ~timeout_register ~timeout_fire_now =
 		Callback.register "libxl_fd_register" fd_register;
 		Callback.register "libxl_fd_modify" fd_modify;
 		Callback.register "libxl_fd_deregister" fd_deregister;
 		Callback.register "libxl_timeout_register" timeout_register;
-		Callback.register "libxl_timeout_modify" timeout_modify;
+		Callback.register "libxl_timeout_fire_now" timeout_fire_now;
 		osevent_register_hooks' ctx user
 
 	let async_register_callback ~async_callback =
diff --git a/tools/ocaml/libs/xl/xenlight.mli.in b/tools/ocaml/libs/xl/xenlight.mli.in
index b9819e1..b2c06b5 100644
--- a/tools/ocaml/libs/xl/xenlight.mli.in
+++ b/tools/ocaml/libs/xl/xenlight.mli.in
@@ -68,11 +68,11 @@ module Async : sig
 
 	val osevent_register_hooks : ctx ->
 		user:'a ->
-		fd_register:('a -> Unix.file_descr -> event list -> for_libxl -> unit) ->
-		fd_modify:('a -> Unix.file_descr -> event list -> unit) ->
-		fd_deregister:('a -> Unix.file_descr -> unit) ->
-		timeout_register:('a -> int64 -> int64 -> for_libxl -> unit) ->
-		timeout_modify:('a -> unit) ->
+		fd_register:('a -> Unix.file_descr -> event list -> for_libxl -> 'b) ->
+		fd_modify:('a -> Unix.file_descr -> 'b -> event list -> 'b) ->
+		fd_deregister:('a -> Unix.file_descr -> 'b -> unit) ->
+		timeout_register:('a -> int64 -> int64 -> for_libxl -> 'c) ->
+		timeout_fire_now:('a -> 'c -> 'c) ->
 		osevent_hooks
 
 	external osevent_occurred_fd : ctx -> for_libxl -> Unix.file_descr -> event list -> event list -> unit = "stub_libxl_osevent_occurred_fd"
diff --git a/tools/ocaml/libs/xl/xenlight_stubs.c b/tools/ocaml/libs/xl/xenlight_stubs.c
index 2e2606a..21fbe00 100644
--- a/tools/ocaml/libs/xl/xenlight_stubs.c
+++ b/tools/ocaml/libs/xl/xenlight_stubs.c
@@ -31,6 +31,7 @@
 #include <libxl_utils.h>
 
 #include <unistd.h>
+#include <assert.h>
 
 #include "caml_xentoollog.h"
 
@@ -1211,14 +1212,20 @@ value Val_poll_events(short events)
 	CAMLreturn(event_list);
 }
 
+/* The process for dealing with the for_app_registration_  values in the
+ * callbacks below (GC registrations etc) is similar to the way for_callback is
+ * handled in the asynchronous operations above. */
+
 int fd_register(void *user, int fd, void **for_app_registration_out,
                      short events, void *for_libxl)
 {
 	caml_leave_blocking_section();
 	CAMLparam0();
 	CAMLlocalN(args, 4);
+	int ret = 0;
 	static value *func = NULL;
 	value *p = (value *) user;
+	value *for_app;
 
 	if (func == NULL) {
 		/* First time around, lookup by name */
@@ -1230,10 +1237,25 @@ int fd_register(void *user, int fd, void **for_app_registration_out,
 	args[2] = Val_poll_events(events);
 	args[3] = (value) for_libxl;
 
-	caml_callbackN(*func, 4, args);
+	for_app = malloc(sizeof(value));
+	if (!for_app) {
+		ret = ERROR_OSEVENT_REG_FAIL;
+		goto err;
+	}
+
+	*for_app = caml_callbackN_exn(*func, 4, args);
+	if (Is_exception_result(*for_app)) {
+		ret = ERROR_OSEVENT_REG_FAIL;
+		goto err;
+	}
+
+	caml_register_global_root(for_app);
+	*for_app_registration_out = for_app;
+
+err:
 	CAMLdone;
 	caml_enter_blocking_section();
-	return 0;
+	return ret;
 }
 
 int fd_modify(void *user, int fd, void **for_app_registration_update,
@@ -1241,9 +1263,14 @@ int fd_modify(void *user, int fd, void **for_app_registration_update,
 {
 	caml_leave_blocking_section();
 	CAMLparam0();
-	CAMLlocalN(args, 3);
+	CAMLlocalN(args, 4);
+	int ret = 0;
 	static value *func = NULL;
 	value *p = (value *) user;
+	value *for_app = *for_app_registration_update;
+
+	/* If for_app == NULL, then something is very wrong */
+	assert(for_app);
 
 	if (func == NULL) {
 		/* First time around, lookup by name */
@@ -1252,21 +1279,37 @@ int fd_modify(void *user, int fd, void **for_app_registration_update,
 
 	args[0] = *p;
 	args[1] = Val_int(fd);
-	args[2] = Val_poll_events(events);
+	args[2] = *for_app;
+	args[3] = Val_poll_events(events);
+
+	*for_app = caml_callbackN_exn(*func, 4, args);
+	if (Is_exception_result(*for_app)) {
+		/* If an exception is caught, *for_app_registration_update is not
+		 * changed. It remains a valid pointer to a value that is registered
+		 * with the GC. */
+		ret = ERROR_OSEVENT_REG_FAIL;
+		goto err;
+	}
+
+	*for_app_registration_update = for_app;
 
-	caml_callbackN(*func, 3, args);
+err:
 	CAMLdone;
 	caml_enter_blocking_section();
-	return 0;
+	return ret;
 }
 
 void fd_deregister(void *user, int fd, void *for_app_registration)
 {
 	caml_leave_blocking_section();
 	CAMLparam0();
-	CAMLlocalN(args, 2);
+	CAMLlocalN(args, 3);
 	static value *func = NULL;
 	value *p = (value *) user;
+	value *for_app = for_app_registration;
+
+	/* If for_app == NULL, then something is very wrong */
+	assert(for_app);
 
 	if (func == NULL) {
 		/* First time around, lookup by name */
@@ -1275,12 +1318,26 @@ void fd_deregister(void *user, int fd, void *for_app_registration)
 
 	args[0] = *p;
 	args[1] = Val_int(fd);
+	args[2] = *for_app;
+
+	caml_callbackN_exn(*func, 3, args);
+	/* This hook does not return error codes, so the best thing we can do
+	 * to avoid trouble, if we catch an exception from the app, is abort. */
+	if (Is_exception_result(*for_app))
+		abort();
+
+	caml_remove_global_root(for_app);
+	free(for_app);
 
-	caml_callbackN(*func, 2, args);
 	CAMLdone;
 	caml_enter_blocking_section();
 }
 
+struct timeout_handles {
+	void *for_libxl;
+	value for_app;
+};
+
 int timeout_register(void *user, void **for_app_registration_out,
                           struct timeval abs, void *for_libxl)
 {
@@ -1288,8 +1345,10 @@ int timeout_register(void *user, void **for_app_registration_out,
 	CAMLparam0();
 	CAMLlocal2(sec, usec);
 	CAMLlocalN(args, 4);
+	int ret = 0;
 	static value *func = NULL;
 	value *p = (value *) user;
+	struct timeout_handles *handles;
 
 	if (func == NULL) {
 		/* First time around, lookup by name */
@@ -1299,15 +1358,37 @@ int timeout_register(void *user, void **for_app_registration_out,
 	sec = caml_copy_int64(abs.tv_sec);
 	usec = caml_copy_int64(abs.tv_usec);
 
+	/* This struct of "handles" will contain "for_libxl" as well as "for_app".
+	 * We'll give a pointer to the struct to the app, and get it back in
+	 * occurred_timeout, where we can clean it all up. */
+	handles = malloc(sizeof(*handles));
+	if (!handles) {
+		ret = ERROR_OSEVENT_REG_FAIL;
+		goto err;
+	}
+
+	handles->for_libxl = for_libxl;
+
 	args[0] = *p;
 	args[1] = sec;
 	args[2] = usec;
-	args[3] = (value) for_libxl;
+	args[3] = (value) handles;
+
+	handles->for_app = caml_callbackN_exn(*func, 4, args);
+	if (Is_exception_result(handles->for_app)) {
+		ret = ERROR_OSEVENT_REG_FAIL;
+		caml_remove_global_root(&handles->for_app);
+		free(handles);
+		goto err;
+	}
 
-	caml_callbackN(*func, 4, args);
+	caml_register_global_root(&handles->for_app);
+	*for_app_registration_out = handles;
+
+err:
 	CAMLdone;
 	caml_enter_blocking_section();
-	return 0;
+	return ret;
 }
 
 int timeout_modify(void *user, void **for_app_registration_update,
@@ -1315,25 +1396,49 @@ int timeout_modify(void *user, void **for_app_registration_update,
 {
 	caml_leave_blocking_section();
 	CAMLparam0();
+	CAMLlocal1(for_app_update);
+	CAMLlocalN(args, 2);
+	int ret = 0;
 	static value *func = NULL;
 	value *p = (value *) user;
+	struct timeout_handles *handles = *for_app_registration_update;
+
+	/* If for_app == NULL, then something is very wrong */
+	assert(handles->for_app);
+
+	/* Libxl currently promises that timeout_modify is only ever called with
+	 * abs={0,0}, meaning "right away". We cannot deal with other values. */
+	assert(abs.tv_sec == 0 && abs.tv_usec == 0);
 
 	if (func == NULL) {
 		/* First time around, lookup by name */
-		func = caml_named_value("libxl_timeout_modify");
+		func = caml_named_value("libxl_timeout_fire_now");
 	}
 
-	caml_callback(*func, *p);
+	args[0] = *p;
+	args[1] = handles->for_app;
+
+	for_app_update = caml_callbackN_exn(*func, 2, args);
+	if (Is_exception_result(for_app_update)) {
+		/* If an exception is caught, *for_app_registration_update is not
+		 * changed. It remains a valid pointer to a value that is registered
+		 * with the GC. */
+		ret = ERROR_OSEVENT_REG_FAIL;
+		goto err;
+	}
+
+	handles->for_app = for_app_update;
+
+err:
 	CAMLdone;
 	caml_enter_blocking_section();
-	return 0;
+	return ret;
 }
 
 void timeout_deregister(void *user, void *for_app_registration)
 {
-	caml_leave_blocking_section();
-	failwith_xl(ERROR_FAIL, "timeout_deregister not yet implemented");
-	caml_enter_blocking_section();
+	/* This hook will never be called by libxl. */
+	abort();
 }
 
 value stub_libxl_osevent_register_hooks(value ctx, value user)
@@ -1386,12 +1491,16 @@ value stub_libxl_osevent_occurred_fd(value ctx, value for_libxl, value fd,
 
 value stub_libxl_osevent_occurred_timeout(value ctx, value for_libxl)
 {
-	CAMLparam2(ctx, for_libxl);
+	CAMLparam1(ctx);
+	struct timeout_handles *handles = (struct timeout_handles *) for_libxl;
 
 	caml_enter_blocking_section();
-	libxl_osevent_occurred_timeout(CTX, (void *) for_libxl);
+	libxl_osevent_occurred_timeout(CTX, (void *) handles->for_libxl);
 	caml_leave_blocking_section();
 
+	caml_remove_global_root(&handles->for_app);
+	free(handles);
+
 	CAMLreturn(Val_unit);
 }
 
-- 
1.7.10.4

Re: [PATCH v4 3/3] libxl: ocaml: use 'for_app_registration' in osevent callbacks

[Ian Jackson]

Rob Hoes writes ("[PATCH v4 3/3] libxl: ocaml: use 'for_app_registration' in osevent callbacks"):
> This allows the application to pass a token to libxl in the fd/timeout
> registration callbacks, which it receives back in modification or
> deregistration callbacks.
...
>  int fd_register(void *user, int fd, void **for_app_registration_out,
>                       short events, void *for_libxl)
>  {
...
> -	caml_callbackN(*func, 4, args);
> +	for_app = malloc(sizeof(value));
...
> +	*for_app = caml_callbackN_exn(*func, 4, args);
> +	if (Is_exception_result(*for_app)) {
> +		ret = ERROR_OSEVENT_REG_FAIL;
> +		goto err;

Doesn't this leak for_app ?  ISTR spotting this before but perhaps I
forgot to mention it.

> +err:
>  	CAMLdone;
>  	caml_enter_blocking_section();
> -	return 0;
> +	return ret;
>  }


And:

>  int timeout_register(void *user, void **for_app_registration_out,
>                            struct timeval abs, void *for_libxl)
...
> +	caml_register_global_root(&handles->for_app);
...
> +	*for_app_registration_out = handles;
>  }
>  
>  int timeout_modify(void *user, void **for_app_registration_update,
...
> +	handles->for_app = for_app_update;
> +

This is allowed, then ?  (Updating foo when &foo has been registered
as a global root.)  I guess so.


Finally:

>  value stub_libxl_osevent_occurred_timeout(value ctx, value for_libxl)
>  {

Calling this formal parameter "for_libxl" is confusing.  It's actually
the value passed to the ocaml register function, ie handles but with a
different type, and not "for_libxl" at all.


Nearly there ... the rest is fine!

Thanks,
Ian.

Re: [PATCH v4 3/3] libxl: ocaml: use 'for_app_registration' in osevent callbacks

[Rob Hoes]

Ian Jackson wrote:
> Rob Hoes writes ("[PATCH v4 3/3] libxl: ocaml: use 'for_app_registration'
> in osevent callbacks"):
> > This allows the application to pass a token to libxl in the fd/timeout
> > registration callbacks, which it receives back in modification or
> > deregistration callbacks.
> ...
> >  int fd_register(void *user, int fd, void **for_app_registration_out,
> >                       short events, void *for_libxl)  {
> ...
> > -	caml_callbackN(*func, 4, args);
> > +	for_app = malloc(sizeof(value));
> ...
> > +	*for_app = caml_callbackN_exn(*func, 4, args);
> > +	if (Is_exception_result(*for_app)) {
> > +		ret = ERROR_OSEVENT_REG_FAIL;
> > +		goto err;
> 
> Doesn't this leak for_app ?  ISTR spotting this before but perhaps I
> forgot to mention it.

Yep, I forgot a "free" there...

> > +err:
> >  	CAMLdone;
> >  	caml_enter_blocking_section();
> > -	return 0;
> > +	return ret;
> >  }
> 
> 
> And:
> 
> >  int timeout_register(void *user, void **for_app_registration_out,
> >                            struct timeval abs, void *for_libxl)
> ...
> > +	caml_register_global_root(&handles->for_app);
> ...
> > +	*for_app_registration_out = handles;
> >  }
> >
> >  int timeout_modify(void *user, void **for_app_registration_update,
> ...
> > +	handles->for_app = for_app_update;
> > +
> 
> This is allowed, then ?  (Updating foo when &foo has been registered as a
> global root.)  I guess so.

Yes, I believe so.

Foo is of type "value", which is typically a pointer to something on the ocaml heap. When the GC runs, it may move the thing on the heap. You therefore need to give the GC the address of foo, so it can update foo itself (the heap pointer). Between GC runs, you can change foo to point to something else on the heap. This just means that what foo used to point to may now be GC'ed (unless it is referred to by another root).

> Finally:
> 
> >  value stub_libxl_osevent_occurred_timeout(value ctx, value for_libxl)
> > {
> 
> Calling this formal parameter "for_libxl" is confusing.  It's actually the
> value passed to the ocaml register function, ie handles but with a
> different type, and not "for_libxl" at all.

Ok, I'll rename it.

> 
> Nearly there ... the rest is fine!
> 
> Thanks,
> Ian.

Cheers,
Rob

[PATCH v5 3/3] libxl: ocaml: use 'for_app_registration' in osevent callbacks

[Rob Hoes]

This allows the application to pass a token to libxl in the fd/timeout
registration callbacks, which it receives back in modification or
deregistration callbacks.

It turns out that this is essential for timeout handling, in order to
identify which timeout to change on a modify event.

Signed-off-by: Rob Hoes <rob.hoes@citrix.com>
Acked-by: David Scott <dave.scott@eu.citrix.com>

---
v2:
* assert if for_app == NULL
* catch any exceptions from callbacks
* use goto-style error handling ;)

v3:
* for timeouts, cleanup for_app in occurred_timeout (not in
  timeout_deregister)
* improve comments
* abort in fd_deregister when the app raises an exception

v4:
* made for_app a value inside the handles struct, rather than a value*
* ensure handles are cleaned up in the error path
* rename timeout_modify to timeout_fire_now on the ocaml side

v5:
* add a missing "free"
* rename for_libxl param in of stub_libxl_occurred_timeout to handles
---
 tools/ocaml/libs/xl/xenlight.ml.in   |    4 +-
 tools/ocaml/libs/xl/xenlight.mli.in  |   10 +--
 tools/ocaml/libs/xl/xenlight_stubs.c |  149 +++++++++++++++++++++++++++++-----
 3 files changed, 136 insertions(+), 27 deletions(-)

diff --git a/tools/ocaml/libs/xl/xenlight.ml.in b/tools/ocaml/libs/xl/xenlight.ml.in
index 47f3487..80e620a 100644
--- a/tools/ocaml/libs/xl/xenlight.ml.in
+++ b/tools/ocaml/libs/xl/xenlight.ml.in
@@ -68,12 +68,12 @@ module Async = struct
 	external osevent_occurred_fd : ctx -> for_libxl -> Unix.file_descr -> event list -> event list -> unit = "stub_libxl_osevent_occurred_fd"
 	external osevent_occurred_timeout : ctx -> for_libxl -> unit = "stub_libxl_osevent_occurred_timeout"
 
-	let osevent_register_hooks ctx ~user ~fd_register ~fd_modify ~fd_deregister ~timeout_register ~timeout_modify =
+	let osevent_register_hooks ctx ~user ~fd_register ~fd_modify ~fd_deregister ~timeout_register ~timeout_fire_now =
 		Callback.register "libxl_fd_register" fd_register;
 		Callback.register "libxl_fd_modify" fd_modify;
 		Callback.register "libxl_fd_deregister" fd_deregister;
 		Callback.register "libxl_timeout_register" timeout_register;
-		Callback.register "libxl_timeout_modify" timeout_modify;
+		Callback.register "libxl_timeout_fire_now" timeout_fire_now;
 		osevent_register_hooks' ctx user
 
 	let async_register_callback ~async_callback =
diff --git a/tools/ocaml/libs/xl/xenlight.mli.in b/tools/ocaml/libs/xl/xenlight.mli.in
index b9819e1..b2c06b5 100644
--- a/tools/ocaml/libs/xl/xenlight.mli.in
+++ b/tools/ocaml/libs/xl/xenlight.mli.in
@@ -68,11 +68,11 @@ module Async : sig
 
 	val osevent_register_hooks : ctx ->
 		user:'a ->
-		fd_register:('a -> Unix.file_descr -> event list -> for_libxl -> unit) ->
-		fd_modify:('a -> Unix.file_descr -> event list -> unit) ->
-		fd_deregister:('a -> Unix.file_descr -> unit) ->
-		timeout_register:('a -> int64 -> int64 -> for_libxl -> unit) ->
-		timeout_modify:('a -> unit) ->
+		fd_register:('a -> Unix.file_descr -> event list -> for_libxl -> 'b) ->
+		fd_modify:('a -> Unix.file_descr -> 'b -> event list -> 'b) ->
+		fd_deregister:('a -> Unix.file_descr -> 'b -> unit) ->
+		timeout_register:('a -> int64 -> int64 -> for_libxl -> 'c) ->
+		timeout_fire_now:('a -> 'c -> 'c) ->
 		osevent_hooks
 
 	external osevent_occurred_fd : ctx -> for_libxl -> Unix.file_descr -> event list -> event list -> unit = "stub_libxl_osevent_occurred_fd"
diff --git a/tools/ocaml/libs/xl/xenlight_stubs.c b/tools/ocaml/libs/xl/xenlight_stubs.c
index 2e2606a..23f253a 100644
--- a/tools/ocaml/libs/xl/xenlight_stubs.c
+++ b/tools/ocaml/libs/xl/xenlight_stubs.c
@@ -31,6 +31,7 @@
 #include <libxl_utils.h>
 
 #include <unistd.h>
+#include <assert.h>
 
 #include "caml_xentoollog.h"
 
@@ -1211,14 +1212,20 @@ value Val_poll_events(short events)
 	CAMLreturn(event_list);
 }
 
+/* The process for dealing with the for_app_registration_  values in the
+ * callbacks below (GC registrations etc) is similar to the way for_callback is
+ * handled in the asynchronous operations above. */
+
 int fd_register(void *user, int fd, void **for_app_registration_out,
                      short events, void *for_libxl)
 {
 	caml_leave_blocking_section();
 	CAMLparam0();
 	CAMLlocalN(args, 4);
+	int ret = 0;
 	static value *func = NULL;
 	value *p = (value *) user;
+	value *for_app;
 
 	if (func == NULL) {
 		/* First time around, lookup by name */
@@ -1230,10 +1237,26 @@ int fd_register(void *user, int fd, void **for_app_registration_out,
 	args[2] = Val_poll_events(events);
 	args[3] = (value) for_libxl;
 
-	caml_callbackN(*func, 4, args);
+	for_app = malloc(sizeof(value));
+	if (!for_app) {
+		ret = ERROR_OSEVENT_REG_FAIL;
+		goto err;
+	}
+
+	*for_app = caml_callbackN_exn(*func, 4, args);
+	if (Is_exception_result(*for_app)) {
+		ret = ERROR_OSEVENT_REG_FAIL;
+		free(for_app);
+		goto err;
+	}
+
+	caml_register_global_root(for_app);
+	*for_app_registration_out = for_app;
+
+err:
 	CAMLdone;
 	caml_enter_blocking_section();
-	return 0;
+	return ret;
 }
 
 int fd_modify(void *user, int fd, void **for_app_registration_update,
@@ -1241,9 +1264,14 @@ int fd_modify(void *user, int fd, void **for_app_registration_update,
 {
 	caml_leave_blocking_section();
 	CAMLparam0();
-	CAMLlocalN(args, 3);
+	CAMLlocalN(args, 4);
+	int ret = 0;
 	static value *func = NULL;
 	value *p = (value *) user;
+	value *for_app = *for_app_registration_update;
+
+	/* If for_app == NULL, then something is very wrong */
+	assert(for_app);
 
 	if (func == NULL) {
 		/* First time around, lookup by name */
@@ -1252,21 +1280,37 @@ int fd_modify(void *user, int fd, void **for_app_registration_update,
 
 	args[0] = *p;
 	args[1] = Val_int(fd);
-	args[2] = Val_poll_events(events);
+	args[2] = *for_app;
+	args[3] = Val_poll_events(events);
+
+	*for_app = caml_callbackN_exn(*func, 4, args);
+	if (Is_exception_result(*for_app)) {
+		/* If an exception is caught, *for_app_registration_update is not
+		 * changed. It remains a valid pointer to a value that is registered
+		 * with the GC. */
+		ret = ERROR_OSEVENT_REG_FAIL;
+		goto err;
+	}
+
+	*for_app_registration_update = for_app;
 
-	caml_callbackN(*func, 3, args);
+err:
 	CAMLdone;
 	caml_enter_blocking_section();
-	return 0;
+	return ret;
 }
 
 void fd_deregister(void *user, int fd, void *for_app_registration)
 {
 	caml_leave_blocking_section();
 	CAMLparam0();
-	CAMLlocalN(args, 2);
+	CAMLlocalN(args, 3);
 	static value *func = NULL;
 	value *p = (value *) user;
+	value *for_app = for_app_registration;
+
+	/* If for_app == NULL, then something is very wrong */
+	assert(for_app);
 
 	if (func == NULL) {
 		/* First time around, lookup by name */
@@ -1275,12 +1319,26 @@ void fd_deregister(void *user, int fd, void *for_app_registration)
 
 	args[0] = *p;
 	args[1] = Val_int(fd);
+	args[2] = *for_app;
+
+	caml_callbackN_exn(*func, 3, args);
+	/* This hook does not return error codes, so the best thing we can do
+	 * to avoid trouble, if we catch an exception from the app, is abort. */
+	if (Is_exception_result(*for_app))
+		abort();
+
+	caml_remove_global_root(for_app);
+	free(for_app);
 
-	caml_callbackN(*func, 2, args);
 	CAMLdone;
 	caml_enter_blocking_section();
 }
 
+struct timeout_handles {
+	void *for_libxl;
+	value for_app;
+};
+
 int timeout_register(void *user, void **for_app_registration_out,
                           struct timeval abs, void *for_libxl)
 {
@@ -1288,8 +1346,10 @@ int timeout_register(void *user, void **for_app_registration_out,
 	CAMLparam0();
 	CAMLlocal2(sec, usec);
 	CAMLlocalN(args, 4);
+	int ret = 0;
 	static value *func = NULL;
 	value *p = (value *) user;
+	struct timeout_handles *handles;
 
 	if (func == NULL) {
 		/* First time around, lookup by name */
@@ -1299,15 +1359,36 @@ int timeout_register(void *user, void **for_app_registration_out,
 	sec = caml_copy_int64(abs.tv_sec);
 	usec = caml_copy_int64(abs.tv_usec);
 
+	/* This struct of "handles" will contain "for_libxl" as well as "for_app".
+	 * We'll give a pointer to the struct to the app, and get it back in
+	 * occurred_timeout, where we can clean it all up. */
+	handles = malloc(sizeof(*handles));
+	if (!handles) {
+		ret = ERROR_OSEVENT_REG_FAIL;
+		goto err;
+	}
+
+	handles->for_libxl = for_libxl;
+
 	args[0] = *p;
 	args[1] = sec;
 	args[2] = usec;
-	args[3] = (value) for_libxl;
+	args[3] = (value) handles;
 
-	caml_callbackN(*func, 4, args);
+	handles->for_app = caml_callbackN_exn(*func, 4, args);
+	if (Is_exception_result(handles->for_app)) {
+		ret = ERROR_OSEVENT_REG_FAIL;
+		free(handles);
+		goto err;
+	}
+
+	caml_register_global_root(&handles->for_app);
+	*for_app_registration_out = handles;
+
+err:
 	CAMLdone;
 	caml_enter_blocking_section();
-	return 0;
+	return ret;
 }
 
 int timeout_modify(void *user, void **for_app_registration_update,
@@ -1315,25 +1396,49 @@ int timeout_modify(void *user, void **for_app_registration_update,
 {
 	caml_leave_blocking_section();
 	CAMLparam0();
+	CAMLlocal1(for_app_update);
+	CAMLlocalN(args, 2);
+	int ret = 0;
 	static value *func = NULL;
 	value *p = (value *) user;
+	struct timeout_handles *handles = *for_app_registration_update;
+
+	/* If for_app == NULL, then something is very wrong */
+	assert(handles->for_app);
+
+	/* Libxl currently promises that timeout_modify is only ever called with
+	 * abs={0,0}, meaning "right away". We cannot deal with other values. */
+	assert(abs.tv_sec == 0 && abs.tv_usec == 0);
 
 	if (func == NULL) {
 		/* First time around, lookup by name */
-		func = caml_named_value("libxl_timeout_modify");
+		func = caml_named_value("libxl_timeout_fire_now");
+	}
+
+	args[0] = *p;
+	args[1] = handles->for_app;
+
+	for_app_update = caml_callbackN_exn(*func, 2, args);
+	if (Is_exception_result(for_app_update)) {
+		/* If an exception is caught, *for_app_registration_update is not
+		 * changed. It remains a valid pointer to a value that is registered
+		 * with the GC. */
+		ret = ERROR_OSEVENT_REG_FAIL;
+		goto err;
 	}
 
-	caml_callback(*func, *p);
+	handles->for_app = for_app_update;
+
+err:
 	CAMLdone;
 	caml_enter_blocking_section();
-	return 0;
+	return ret;
 }
 
 void timeout_deregister(void *user, void *for_app_registration)
 {
-	caml_leave_blocking_section();
-	failwith_xl(ERROR_FAIL, "timeout_deregister not yet implemented");
-	caml_enter_blocking_section();
+	/* This hook will never be called by libxl. */
+	abort();
 }
 
 value stub_libxl_osevent_register_hooks(value ctx, value user)
@@ -1384,14 +1489,18 @@ value stub_libxl_osevent_occurred_fd(value ctx, value for_libxl, value fd,
 	CAMLreturn(Val_unit);
 }
 
-value stub_libxl_osevent_occurred_timeout(value ctx, value for_libxl)
+value stub_libxl_osevent_occurred_timeout(value ctx, value handles)
 {
-	CAMLparam2(ctx, for_libxl);
+	CAMLparam1(ctx);
+	struct timeout_handles *c_handles = (struct timeout_handles *) handles;
 
 	caml_enter_blocking_section();
-	libxl_osevent_occurred_timeout(CTX, (void *) for_libxl);
+	libxl_osevent_occurred_timeout(CTX, (void *) c_handles->for_libxl);
 	caml_leave_blocking_section();
 
+	caml_remove_global_root(&c_handles->for_app);
+	free(c_handles);
+
 	CAMLreturn(Val_unit);
 }
 
-- 
1.7.10.4

Re: [PATCH v5 3/3] libxl: ocaml: use 'for_app_registration' in osevent callbacks

[Ian Jackson]

Rob Hoes writes ("[PATCH v5 3/3] libxl: ocaml: use 'for_app_registration' in osevent callbacks"):
> This allows the application to pass a token to libxl in the fd/timeout
> registration callbacks, which it receives back in modification or
> deregistration callbacks.

Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>

Thanks!

Ian.

Re: libxl: ocaml: fix tests makefile and osevent callbacks

[Ian Campbell]

On Thu, 2013-12-12 at 16:36 +0000, Rob Hoes wrote:
> I'd really appreciate it if these fixes can still be considered for Xen 4.4,
> for the same reasons as the larger libxl/ocaml series that went in yesterday.

I haven't delved into these threads but I think we are waiting for a v2?

Ian.

Re: libxl: ocaml: fix tests makefile and osevent callbacks

[Rob Hoes]

Ian Campbell wrote:
> Sent: 07 January 2014 12:27 PM
> To: Rob Hoes
> Cc: xen-devel@lists.xen.org; Ian Jackson; Dave Scott
> Subject: Re: libxl: ocaml: fix tests makefile and osevent callbacks
> 
> On Thu, 2013-12-12 at 16:36 +0000, Rob Hoes wrote:
> > I'd really appreciate it if these fixes can still be considered for
> > Xen 4.4, for the same reasons as the larger libxl/ocaml series that went
> in yesterday.
> 
> I haven't delved into these threads but I think we are waiting for a v2?

There were 3 patches:
1: Already applied (the Makefile fix).
2: Dave acked it on 20/12. Ian J was happy with the patch after some discussion (but did not formally ack it yet). No v2 was needed.
3: I sent a v2 on 13/12. Dave acked it on 20/12. We had some discussion and I think you and Ian J seemed happy (but did not formally ack yet).

Cheers,
Rob

Re: libxl: ocaml: fix tests makefile and osevent callbacks

[Ian Campbell]

On Tue, 2014-01-07 at 14:13 +0000, Rob Hoes wrote:
> 2: Dave acked it on 20/12. Ian J was happy with the patch after some discussion (but did not formally ack it yet). No v2 was needed.

I wasn't sure about Ian's response being approval:
        I mean that I approve of the plan to use two int64's as in Rob's
        proposal.

unless "the plan" == "the patch"?

> 3: I sent a v2 on 13/12. Dave acked it on 20/12. We had some discussion and I think you and Ian J seemed happy (but did not formally ack yet).

Thanks, I had missed v2 of #3.

Ian -- can you formally ack both of these if you are happy with them
please.

Release wise I don't see any point in shipping the ocaml bindings
without these issues fixed, so Release-ack-by: Ian Campbell.

Ian.