Re: [PATCH v1 2/5] tools/ocaml/libs/xc: add binding to xc_evtchn_status

[Andrew Cooper <Andrew.Cooper3@xxxxxxxxxx>]

On 01/12/2022 13:35, Edwin Torok wrote:
>> On 1 Dec 2022, at 11:34, Andrew Cooper <Andrew.Cooper3@xxxxxxxxxx> wrote:
>>
>> On 30/11/2022 17:32, Edwin Török wrote:
>>> +
>>> +    caml_enter_blocking_section();
>>> +    rc = xc_evtchn_status(_H(xch), &status);
>>> +    caml_leave_blocking_section();
>>> +
>>> +    if ( rc < 0 )
>>> +        failwith_xc(_H(xch));
>>> +
>>> +    if ( status.status == EVTCHNSTAT_closed )
>>> +        result = Val_none;
>>> +    else
>>> +    {
>> This is actually one example where using a second CAMLreturn would
>> simply things substantially.
>>
>> switch ( status.status )
>> {
>> case EVTCHNSTAT_closed:
>>     CAMLreturn(Val_none);
>>
>> case EVTCHNSTAT_unbound:
>>     ...
>>
>> Would remove the need for the outer if/else.
>
> CAMLreturn has some macro magic to ensure it gets paired with the toplevel 
> CAMLparam correctly (one of them opens a { scope and the other closes it, or 
> something like that),
> so I'd avoid putting it into the middle of other syntactic elements, it might 
> just cause the build to fail (either now or in the future).

From the manual:

"The macros CAMLreturn, CAMLreturn0, and CAMLreturnT are used to replace
the C keyword return. Every occurrence of return x must be replaced by ..."

It is common in C to have multiple returns, and the manual does say
"Every occurrence" without stating any requirement for there to be a
single occurrence.

CAMLreturn can't syntactically work splitting a scope with CAMLparam,
because then this wouldn't compile:

CAMLprim value stub_xc_evtchn_status(value foo)
{
    CAMLparam1(foo);
    int bar = 0;

retry:
    if ( bar )
        CAMLreturn(foo);

    bar++
    goto retry;
}

CAMLreturn does use a normal "do { ... } while (0)" construct simply for
being a macro, and forces paring with CAMLparamX by referencing the
caml__frame local variable by name.


So I really do think that multiple CAMLreturns are fine and cannot
reasonably be broken in the future.

But if we do really still want to keep a single return, then a "goto
done" would be acceptable here and still better than the if/else.

>> caml_alloc_some() is perhaps less interesting as it only appeared in
>> Ocaml 4.12 AFAICT, but we could also have some ifdefary for that at the
>> top of the file.
>>
>> I don't know whether we have opencoded options elsewhere in the
>> bindings, but it certainly would be reduce the amount opencoding that
>> exists for standard patterns.
>
> perhaps we can look into doing that cleanup as a separate patch.

Probably best, yes.

~Andrew