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

[Ian Jackson <Ian.Jackson@xxxxxxxxxxxxx>]

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.

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