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

[Ian Jackson <Ian.Jackson@xxxxxxxxxxxxx>]

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.

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