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

[Rob Hoes <Rob.Hoes@xxxxxxxxxx>]

On 12 Dec 2013, at 19:18, Ian Campbell <Ian.Campbell@xxxxxxxxxx> wrote:

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

Agreed, but I think my version had that property as well. Anyway, it looks like 
an assert is the way to go in this case (see below) :)

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

For the modify and deregister functions, I’d consider it a programming error if 
for_app is NULL (either in the bindings or libxl itself). This is because the 
register functions will always get a value passed in from the OCaml app (even 
if it is just a unit “()”), and this value is expected to be delivered to 
subsequent (related) modify or deregister calls.

Following your reasoning, we should use an assert for this.

For the register functions, for_app_registration_out is NULL on entry 
(according to libxl_event.h), but this is ignored by the bindings. There is an 
“if (for_app)” there to check the outcome of malloc. If malloc returns NULL, 
then the second category applies (external circumstance). According to the 
docs, a registration of modification hook may return ERROR_OSEVENT_REG_FAIL or 
any positive int in case it fails, which seems appropriate there.

I’ll update the patch.

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