Re: [PATCH v2 2/4] tools/ocaml/libs/xc: add binding to xc_evtchn_status

[Andrew Cooper <Andrew.Cooper3@xxxxxxxxxx>]

On 02/12/2022 10:55, Edwin Török wrote:
> diff --git a/tools/ocaml/libs/xc/xenctrl_stubs.c 
> b/tools/ocaml/libs/xc/xenctrl_stubs.c
> index d30585f21c..a492ea17fd 100644
> --- a/tools/ocaml/libs/xc/xenctrl_stubs.c
> +++ b/tools/ocaml/libs/xc/xenctrl_stubs.c
> @@ -641,6 +645,69 @@ CAMLprim value stub_xc_evtchn_reset(value xch, value 
> domid)
>      CAMLreturn(Val_unit);
>  }
>  
> +CAMLprim value stub_xc_evtchn_status(value xch, value domid, value port)
> +{
> +    CAMLparam3(xch, domid, port);
> +    CAMLlocal4(result, result_status, stat, interdomain);
> +    xc_evtchn_status_t status = {
> +        .dom = _D(domid),
> +        .port = Int_val(port),
> +    };
> +    int rc;
> +
> +    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 )
> +        CAMLreturn(Val_none);
> +
> +    switch ( status.status )
> +    {

The EVTCHNSTAT_closed wants to be a case here, otherwise it's really
weird to read from a C point of view.

It would be fine to have a comment like this:

case EVTCHNSTAT_closed:
    CAMLreturn(Val_none); /* Early exit, no allocations needed. */

to help identify more clearly that this a bit of a special case.

> +    case EVTCHNSTAT_unbound:
> +        stat = caml_alloc(1, 0); /* 1st non-constant constructor */
> +        Store_field(stat, 0, Val_int(status.u.unbound.dom));
> +        break;
> +
> +    case EVTCHNSTAT_interdomain:
> +        interdomain = caml_alloc_tuple(2);
> +        Store_field(interdomain, 0, Val_int(status.u.interdomain.dom));
> +        Store_field(interdomain, 1, Val_int(status.u.interdomain.port));
> +        stat = caml_alloc(1, 1); /*  2nd non-constant constructor */
> +        Store_field(stat, 0, interdomain);
> +        break;

Newline here.

> +    case EVTCHNSTAT_pirq:
> +        stat = caml_alloc(1, 2); /* 3rd non-constant constructor */
> +        Store_field(stat, 0, Val_int(status.u.pirq));
> +        break;
> +
> +    case EVTCHNSTAT_virq:
> +        stat = caml_alloc(1, 3); /* 4th non-constant constructor */
> +        Store_field(stat, 0, Val_int(status.u.virq));
> +        break;
> +
> +    case EVTCHNSTAT_ipi:
> +        stat = Val_int(0); /* 1st constant constructor */
> +        break;
> +
> +    default:
> +        caml_failwith("Unknown evtchn status");
> +
> +    }
> +    result_status = caml_alloc_tuple(2);
> +    Store_field(result_status, 0, Val_int(status.vcpu));
> +    Store_field(result_status, 1, stat);
> +
> +    /* caml_alloc_some is missing in older versions of OCaml
> +     */

I'd just drop this comment.  It's going to be many many years before
Ocaml 4.12 drops off the bottom of the support list, so this observation
is unactionable.

All 3 of these are trivial to fix on commit, so Reviewed-by: Andrew
Cooper <andrew.cooper3@xxxxxxxxxx> otherwise.

~Andrew