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

[Rob Hoes <Rob.Hoes@xxxxxxxxxx>]

Ian Campbell wrote:
> > @@ -1241,32 +1255,43 @@ int fd_modify(void *user, int fd, void
> > **for_app_registration_update,  {
> >     caml_leave_blocking_section();
> >     CAMLparam0();
> > -   CAMLlocalN(args, 3);
> > +   CAMLlocalN(args, 4);
> > +   int ret = 0;
> >     static value *func = NULL;
> >     value *p = (value *) user;
> > +   value *for_app = *for_app_registration_update;
> >
> > -   if (func == NULL) {
> > -           /* First time around, lookup by name */
> > -           func = caml_named_value("libxl_fd_modify");
> > -   }
> > +   /* if for_app == NULL, assume that something is wrong and don't
> callback */
> > +   if (for_app) {
> 
> if (!for_app) {
>       cleanup
>       return;
> }
> 
> Would be more natural and save perturbing the rest of the function so much.
> If the cleanup is the same as the tail of the function then the goto style
> of error handling is good too:

I think this is a matter or taste... To me, using goto isn't natural at all! I 
find the control flow a little clearer the way I did it.

However, I can change it to what you have suggested if that brings it more in 
line with the rest of the C code in the tree.

>       if (!for_app) {
>               ret = ERROR__....;
>               goto err;
>       }
>       [...]
>       ret = ...
> 
>  err:
>       CAMLdone;
>       caml_enter_blocking_section();
>       return ret;
> }
> 
> Actually, elsewhere you don't bother to check for_app. If this would
> indicate a major error then I think an assert would be perfectly
> reasonably.

I think the check is missing from fd_deregister, indeed. I need to add it there.

If it's an assert, I believe it would kill the whole application? I think 
that's a little too much. I think that returning ERROR_OSEVENT_REG_FAIL would 
be the right thing here, because I believe that libxl would send it back to the 
application and just fail the current operation.

Rob

> > +           if (func == NULL) {
> > +                   /* First time around, lookup by name */
> > +                   func = caml_named_value("libxl_fd_modify");
> > +           }
> >
> > -   args[0] = *p;
> > -   args[1] = Val_int(fd);
> > -   args[2] = Val_poll_events(events);
> > +           args[0] = *p;
> > +           args[1] = Val_int(fd);
> > +           args[2] = *for_app;
> > +           args[3] = Val_poll_events(events);
> > +
> > +           *for_app = caml_callbackN(*func, 4, args);
> > +           *for_app_registration_update = for_app;
> > +   }
> > +   else
> > +           ret = ERROR_OSEVENT_REG_FAIL;
> >
> > -   caml_callbackN(*func, 3, args);
> >     CAMLdone;
> >     caml_enter_blocking_section();
> > -   return 0;
> > +   return ret;
> >  }
> > +   for_app = malloc(sizeof(value));
> > +   if (for_app) {
> > +           *for_app = caml_callbackN(*func, 4, args);
> 
> Can the callbackN fail, eg. returning an exception or something?
> > @@ -1315,18 +1354,34 @@ 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;
> >
> > -   if (func == NULL) {
> > -           /* First time around, lookup by name */
> > -           func = caml_named_value("libxl_timeout_modify");
> > +   /* if for_app == NULL, assume that something is wrong and don't
> > +callback */
> 
> Same comment as the other occasion.
> 

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