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 ("RE: [PATCH v2 3/3] libxl: ocaml: use
> 'for_app_registration' in osevent callbacks"):
> > Ian Jackson wrote:
> > > 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.
> 
> The usual sequence of events is
>   timeout_register
>       with your new patch:
>            stashes for_libxl value in ocaml gc
>            calls ocaml libxl_timeout_register with for_libxl
>            stashes that function's return in for_app and adds it to the gc
>   timeout occurs
>       the timeout machinery calls stub_libxl_osevent_occurred_timeout
>           with the for_libxl value it has kept somehow
>       stub_libxl_osevent_occurred_timeout calls
>           libxl_osevent_occurred_timeout
> 
> Now the timeout is gone and nothing will deal with it again.  Who cleans
> up the for_app value ?
> 
> Perhaps you are confused and don't realise that timeouts are one-shot.
> See the comment next to libxl_osevent_occurred_timeout.

One part of my brain knew that, but another part wrote this function... :)

I'll send an update.

> > I do assume here that timeout_modify will be called only once for a
> > given timeout registration. Is that correct?
> 
> The specification is that it may be called more than once, or not at all.
> The cleanup needs to be done in stub_libxl_osevent_occurred_timeout.
> 
> (And you don't probably don't want a binding for timeout_deregister.
> That's only there for compatibility with what are now old libxls, and only
> if those libxls don't have the race patches which are necessary for
> reliable operation.)

It is already not there on the ocaml side for this reason. There is just a stub 
that raises an error, which is given to osevent_register_hooks (just to be 
sure). I should probably just put an abort in there rather than it raising an 
ocaml exception as it does now...

Cheers,
Rob


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