Re: [PATCH v1 4/4] tools/ocaml/libs/xc: backward compatible domid control at domain creation time

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 17/11/2020 18:24, Edwin Török wrote:
> One can specify the domid to use when creating the domain, but this was 
> hardcoded to 0.
>
> Keep the existing `domain_create` function (and the type of its parameters) 
> as is to make
> backwards compatibility easier.
> Introduce a new `domain_create_domid` OCaml API that allows specifying the 
> domid.
> A new version of xenopsd can choose to start using this, while old versions 
> of xenopsd will keep
> building and using the old API.
>
> Controlling the domid can be useful during testing or migration.
>
> Signed-off-by: Edwin Török <edvin.torok@xxxxxxxxxx>
> ---
>  tools/ocaml/libs/xc/xenctrl.ml      | 3 +++
>  tools/ocaml/libs/xc/xenctrl.mli     | 2 ++
>  tools/ocaml/libs/xc/xenctrl_stubs.c | 9 +++++++--
>  3 files changed, 12 insertions(+), 2 deletions(-)
>
> diff --git a/tools/ocaml/libs/xc/xenctrl.ml b/tools/ocaml/libs/xc/xenctrl.ml
> index e878699b0a..9d720886e9 100644
> --- a/tools/ocaml/libs/xc/xenctrl.ml
> +++ b/tools/ocaml/libs/xc/xenctrl.ml
> @@ -182,6 +182,9 @@ let with_intf f =
>  external domain_create: handle -> domctl_create_config -> domid
>         = "stub_xc_domain_create"
>  
> +external domain_create_domid: handle -> domctl_create_config -> domid -> 
> domid
> +       = "stub_xc_domain_create_domid"

Wouldn't this be better as handle -> domid -> domctl_create_config ->
domid ?

I'm not overwhelmed with the name, but
"domain_create_{specific,with}_domid" don't seem much better either.

> +
>  external domain_sethandle: handle -> domid -> string -> unit
>         = "stub_xc_domain_sethandle"
>  
> diff --git a/tools/ocaml/libs/xc/xenctrl.mli b/tools/ocaml/libs/xc/xenctrl.mli
> index e64907df8e..e629022901 100644
> --- a/tools/ocaml/libs/xc/xenctrl.mli
> +++ b/tools/ocaml/libs/xc/xenctrl.mli
> @@ -145,6 +145,8 @@ val close_handle: unit -> unit
>  
>  external domain_create : handle -> domctl_create_config -> domid
>    = "stub_xc_domain_create"
> +external domain_create_domid : handle -> domctl_create_config -> domid -> 
> domid
> +  = "stub_xc_domain_create_domid"
>  external domain_sethandle : handle -> domid -> string -> unit = 
> "stub_xc_domain_sethandle"
>  external domain_max_vcpus : handle -> domid -> int -> unit
>    = "stub_xc_domain_max_vcpus"
> diff --git a/tools/ocaml/libs/xc/xenctrl_stubs.c 
> b/tools/ocaml/libs/xc/xenctrl_stubs.c
> index 94aba38a42..bb718fd164 100644
> --- a/tools/ocaml/libs/xc/xenctrl_stubs.c
> +++ b/tools/ocaml/libs/xc/xenctrl_stubs.c
> @@ -175,7 +175,7 @@ static unsigned int ocaml_list_to_c_bitmap(value l)
>       return val;
>  }
>  
> -CAMLprim value stub_xc_domain_create(value xch, value config)
> +CAMLprim value stub_xc_domain_create_domid(value xch, value config, value 
> want_domid)
>  {
>       CAMLparam2(xch, config);
>       CAMLlocal2(l, arch_domconfig);
> @@ -191,7 +191,7 @@ CAMLprim value stub_xc_domain_create(value xch, value 
> config)
>  #define VAL_MAX_MAPTRACK_FRAMES Field(config, 7)
>  #define VAL_ARCH                Field(config, 8)
>  
> -     uint32_t domid = 0;
> +     uint32_t domid = Int_val(want_domid);

wanted_domid would be a slightly better name, because it isn't ambiguous
with a boolean flag.

>       int result;
>       struct xen_domctl_createdomain cfg = {
>               .ssidref = Int32_val(VAL_SSIDREF),
> @@ -262,6 +262,11 @@ CAMLprim value stub_xc_domain_create(value xch, value 
> config)
>       CAMLreturn(Val_int(domid));
>  }
>  
> +CAMLprim value stub_xc_domain_create(value xch, value config, value 
> want_domid)
> +{
> +    return stub_xc_domain_create_domid(xch, config, Val_int(0));
> +}

Using
https://xenbits.xen.org/gitweb/?p=xen.git;a=commitdiff;h=36d94c17fa1e48cc9fb9ed15bc9a2237a1738bbb
as reverse inspiration, couldn't we do the insertion of 0 at the Ocaml
level and avoid doubling up the C stub?

~Andrew

> +
>  CAMLprim value stub_xc_domain_max_vcpus(value xch, value domid,
>                                          value max_vcpus)
>  {