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

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

> @@ -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:

        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.

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