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

[Rob Hoes <Rob.Hoes@xxxxxxxxxx>]

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

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