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

[Rob Hoes <Rob.Hoes@xxxxxxxxxx>]

> This may seem like a daft question, but why does handles contain a value*
> rather than just a value ?

Right, the important thing is that this "value" is malloc, which is will be if 
it is part of the handles struct. I'll change that.

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

Ok, makes sense.

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

Ok.

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

Yes, it is probably clearer to change the name on the ocaml side to show better 
what the function really does. I was thinking about "fire_now".

I'll send an update soon.

Thanks,
Rob

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