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

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

On Thu, 2013-12-12 at 17:54 +0000, Rob Hoes wrote:
> 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.

The benefit of the goto style is that you consolidate all of the
function epilogue into once place rather than having to repeat it and
risk one half getting out of sync.
[...]
> > 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.

Under what circumstances can for_app ever be NULL? If it indicates a
programming error in the bindings themselves (which I think it does?)
then an assert is appropriate, since it should never happen.

If it indicates an error in the caller of the bindings or something
which can happen for some reason unrelated to the code itself (e.g some
external circumstance) then the error return would be appropriate.

Ian.


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