Re: [Xen-devel] [PATCH 22/28] libxl: ocaml: event management

[Rob Hoes <Rob.Hoes@xxxxxxxxxx>]

> > +type event =
> > +   | POLLIN (* There is data to read *)
> > +   | POLLPRI (* There is urgent data to read *)
> > +   | POLLOUT (* Writing now will not block *)
> > +   | POLLERR (* Error condition (revents only) *)
> > +   | POLLHUP (* Device has been disconnected (revents only) *)
> > +   | POLLNVAL (* Invalid request: fd not open (revents only). *)
> 
> Can you reuse these from your poll library in the previous patch?

The previous patch only introduced them in C, this one adds the OCaml stuff.

> 
> > +int timeout_register(void *user, void **for_app_registration_out,
> > +                          struct timeval abs, void *for_libxl) {
> > +   return 0;
> > +}
> > +
> > +int timeout_modify(void *user, void **for_app_registration_update,
> > +                         struct timeval abs) {
> > +   return 0;
> > +}
> > +
> > +void timeout_deregister(void *user, void *for_app_registration) {
> > +   return;
> > +}
> 
> Worth failing noisily until these are implemented?

Yes, I'll raise some exceptions.

> > +
> > +value stub_xl_osevent_register_hooks(value ctx, value user) {
> > +   CAMLparam2(ctx, user);
> > +   libxl_osevent_hooks *hooks;
> > +   hooks = malloc(sizeof(*hooks));
> > +
> > +   hooks->fd_register = fd_register;
> > +   hooks->fd_modify = fd_modify;
> > +   hooks->fd_deregister = fd_deregister;
> > +   hooks->timeout_register = timeout_register;
> > +   hooks->timeout_modify = timeout_modify;
> > +   hooks->timeout_deregister = timeout_deregister;
> > +
> > +   libxl_osevent_register_hooks(CTX, hooks, (void *) user);
> 
> This user thing will be retained by libxl -- is that safe from an ocaml gc 
> point
> of view?

Good point. The original value may go out of scope in the OCaml program and 
will then be GC'ed. We should copy the value to avoid trouble. To do that, 
though, we need to know the type of the thing, which is currently polymorphic. 
I'll just go ahead and make it a string instead, because that seems to be the 
most useful.

Are these hooks and associated data every cleaned up by libxl? Or is the 
assumption that libxl_osevent_register_hooks is just called once at the 
beginning of the program, and everything starts till the end?

> > +   CAMLreturn((value) hooks);
> 
> Another instance of the problematic heap allocation pattern Dave pointed
> out?

Yes, we should turn this into a custom or abstract block as well.

> > +void event_occurs(void *user, const libxl_event *event) {
> > +   CAMLparam0();
> > +   CAMLlocalN(args, 2);
> > +   value *func = caml_named_value("xl_event_occurs_callback");
> > +
> > +   args[0] = (value) user;
> > +   args[1] = Val_event((libxl_event *) event);
> > +   //libxl_event_free(CTX, event); // no ctx here!
> 
> Is it leaked or do you free it somewhere else? I suppose "func" must do it?
> (which makes sense actually)

Hmm... This is awkward. The thing we are giving to "func" is the event 
translated into an Ocaml type, and not the C libxl_event*. And even if we give 
the libxl_event* to "func" as well, it still needs to know the ctx in order to 
free it (which it probably would, but won't make things easier to use). Is 
there no way to ask libxl to which ctx the event belongs?

> [...]
> > +value stub_xl_event_register_callbacks(value ctx, value user) {
> > +   CAMLparam2(ctx, user);
> > +   libxl_event_hooks *hooks;
> > +
> > +   hooks = malloc(sizeof(*hooks));
> 
> Another heap alloc?

Yes... another abstract block.

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