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

[Edwin Torok <edvin.torok@xxxxxxxxxx>]

On Wed, 2020-11-18 at 18:13 +0000, Andrew Cooper wrote:
> 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?

I wanted to retain the old API for backwards compatibility, but you are
right that this could be done just on the OCaml level, I'll update the
patch.

If you upgrade Xen without upgrading xenopsd you'll get a fairly
obvious failure to start xenopsd due to the missing symbol, but that
could be solved with an appropriate dependency at the distro package
level. As long as matching Xen and xenopsd gets installed (even if not
booted into) xenopsd should succeed in restarting then.

Best regards,
--Edwin

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