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

[Rob Hoes <Rob.Hoes@xxxxxxxxxx>]

Ian Jackson 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.
> >
> > 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 ?

This macro is defined as follows:

    #define Is_exception_result(v) (((v) & 3) == 2)

So this won't cause any harm. I think the order is therefore correct, and since 
we don't want to register for_app with the GC in case of an exception (this may 
change if we'd try to interpret the exception and log it, later on).

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

Ok.

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

Ok, that makes sense.

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

Yes, agreed.

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

Hmm... I thought the for_app stuff is only for the registration bits? The 
osevent_occurred functions don't use or receive it? They do get for_libxl, but 
that's entirely in C and opaque to ocaml.

I do assume here that timeout_modify will be called only once for a given 
timeout registration. Is that correct?

I'll send an update for the comment and exception thing mentioned above.

Cheers,
Rob


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