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

[Ian Jackson <Ian.Jackson@xxxxxxxxxxxxx>]

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

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