Xen devel - Dec 2013 - libxl: ocaml: fix tests makefile and osevent callbacks

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

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

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

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

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.

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>

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

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.

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

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.

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.

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

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

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

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.

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.

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

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.

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.

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

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

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

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.

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

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.

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

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

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