Re: [Xen-devel] [PATCH v5 5/7] libxl: allow creation of domains with a specified or random domid

["Durrant, Paul" <pdurrant@xxxxxxxxxxxx>]

> -----Original Message-----
> From: Ian Jackson <ian.jackson@xxxxxxxxxx>
> Sent: 17 February 2020 17:52
> To: Durrant, Paul <pdurrant@xxxxxxxxxxxx>
> Cc: xen-devel@xxxxxxxxxxxxxxxxxxxx; Wei Liu <wl@xxxxxxx>; Anthony Perard
> <anthony.perard@xxxxxxxxxx>; Andrew Cooper <Andrew.Cooper3@xxxxxxxxxx>;
> George Dunlap <George.Dunlap@xxxxxxxxxx>; Jan Beulich <jbeulich@xxxxxxxx>;
> Julien Grall <julien@xxxxxxx>; Konrad Rzeszutek Wilk
> <konrad.wilk@xxxxxxxxxx>; Stefano Stabellini <sstabellini@xxxxxxxxxx>;
> Jason Andryuk <jandryuk@xxxxxxxxx>
> Subject: Re: [PATCH v5 5/7] libxl: allow creation of domains with a
> specified or random domid
> 
> Paul Durrant writes ("[PATCH v5 5/7] libxl: allow creation of domains with
> a specified or random domid"):
> > This patch adds a 'domid' field to libxl_domain_create_info and then
> > modifies libxl__domain_make() to have Xen use that value if it is valid.
> > If the domid value is invalid then Xen will choose the domid, as before,
> > unless the value is the new special RANDOM_DOMID value added to the API.
> > This value instructs libxl__domain_make() to choose a random domid value
> > for Xen to use.
> >
> > If Xen determines that a domid specified to or chosen by
> > libxl__domain_make() co-incides with an existing domain then the create
> > operation will fail. In this case, if RANDOM_DOMID was specified to
> > libxl__domain_make() then a new random value will be chosen and the
> create
> > operation will be re-tried, otherwise libxl__domain_make() will fail.
> >
> > After Xen has successfully created a new domain, libxl__domain_make()
> will
> > check whether its domid matches any recently used domid values. If it
> does
> > then the domain will be destroyed. If the domid used in creation was
> > specified to libxl__domain_make() then it will fail at this point,
> > otherwise the create operation will be re-tried with either a new random
> > or Xen-selected domid value.
> >
> > NOTE: libxl__logv() is also modified to only log valid domid values in
> >       messages rather than any domid, valid or otherwise, that is not
> >       INVALID_DOMID.
> >
> > Signed-off-by: Paul Durrant <pdurrant@xxxxxxxxxx>
> > ---
> > Cc: Ian Jackson <ian.jackson@xxxxxxxxxxxxx>
> > Cc: Wei Liu <wl@xxxxxxx>
> > Cc: Anthony PERARD <anthony.perard@xxxxxxxxxx>
> > Cc: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
> > Cc: George Dunlap <George.Dunlap@xxxxxxxxxxxxx>
> > Cc: Jan Beulich <jbeulich@xxxxxxxx>
> > Cc: Julien Grall <julien@xxxxxxx>
> > Cc: Konrad Rzeszutek Wilk <konrad.wilk@xxxxxxxxxx>
> > Cc: Stefano Stabellini <sstabellini@xxxxxxxxxx>
> > Cc: Jason Andryuk <jandryuk@xxxxxxxxx>
> >
> > v5:
> >  - Flattened nested loops
> >
> > v4:
> >  - Not added Jason's R-b because of substantial change
> >  - Check for recent domid *after* creation
> >  - Re-worked commit comment
> >
> > v3:
> >  - Added DOMID_MASK definition used to mask randomized values
> >  - Use stack variable to avoid assuming endianness
> >
> > v2:
> >  - Re-worked to use a value from libxl_domain_create_info
> > ---
> >  tools/libxl/libxl.h          |  9 +++++
> >  tools/libxl/libxl_create.c   | 67 ++++++++++++++++++++++++++++++++----
> >  tools/libxl/libxl_internal.c |  2 +-
> >  tools/libxl/libxl_types.idl  |  1 +
> >  xen/include/public/xen.h     |  3 ++
> >  5 files changed, 74 insertions(+), 8 deletions(-)
> >
> > diff --git a/tools/libxl/libxl.h b/tools/libxl/libxl.h
> > index 1d235ecb1c..31c6f4b11a 100644
> > --- a/tools/libxl/libxl.h
> > +++ b/tools/libxl/libxl.h
> > @@ -1268,6 +1268,14 @@ void libxl_mac_copy(libxl_ctx *ctx, libxl_mac
> *dst, const libxl_mac *src);
> >   */
> >  #define LIBXL_HAVE_DOMAIN_NEED_MEMORY_CONFIG
> >
> > +/*
> > + * LIBXL_HAVE_CREATEINFO_DOMID
> > + *
> > + * libxl_domain_create_new() and libxl_domain_create_restore() will use
> > + * a domid specified in libxl_domain_create_info().
> > + */
> > +#define LIBXL_HAVE_CREATEINFO_DOMID
> > +
> >  typedef char **libxl_string_list;
> >  void libxl_string_list_dispose(libxl_string_list *sl);
> >  int libxl_string_list_length(const libxl_string_list *sl);
> > @@ -1528,6 +1536,7 @@ int libxl_ctx_free(libxl_ctx *ctx /* 0 is OK */);
> >  /* domain related functions */
> >
> >  #define INVALID_DOMID ~0
> > +#define RANDOM_DOMID (INVALID_DOMID - 1)
> >
> >  /* If the result is ERROR_ABORTED, the domain may or may not exist
> >   * (in a half-created state).  *domid will be valid and will be the
> > diff --git a/tools/libxl/libxl_create.c b/tools/libxl/libxl_create.c
> > index 3a7364e2ac..7fd4d713e7 100644
> > --- a/tools/libxl/libxl_create.c
> > +++ b/tools/libxl/libxl_create.c
> > @@ -555,8 +555,6 @@ int libxl__domain_make(libxl__gc *gc,
> libxl_domain_config *d_config,
> >      libxl_domain_create_info *info = &d_config->c_info;
> >      libxl_domain_build_info *b_info = &d_config->b_info;
> >
> > -    assert(soft_reset || *domid == INVALID_DOMID);
> > -
> >      uuid_string = libxl__uuid2string(gc, info->uuid);
> >      if (!uuid_string) {
> >          rc = ERROR_NOMEM;
> > @@ -600,11 +598,66 @@ int libxl__domain_make(libxl__gc *gc,
> libxl_domain_config *d_config,
> >              goto out;
> >          }
> >
> > -        ret = xc_domain_create(ctx->xch, domid, &create);
> > -        if (ret < 0) {
> > -            LOGED(ERROR, *domid, "domain creation fail");
> > -            rc = ERROR_FAIL;
> > -            goto out;
> > +        for (;;) {
> > +            bool recent;
> > +
> > +            if (info->domid == RANDOM_DOMID) {
> > +                uint16_t v;
> > +
> > +                ret = libxl__random_bytes(gc, (void *)&v, sizeof(v));
> > +                if (ret < 0)
> > +                    break;
> > +
> > +                v &= DOMID_MASK;
> > +                if (!libxl_domid_valid_guest(v))
> > +                    continue;
> > +
> > +                *domid = v;
> > +            } else
> > +                *domid = info->domid;
> 
> Style: { } on all or none of the same `if' series.  (CODING_STYLE)

OK. Too used to Xen style.

> 
> > +            /* The domid is not recent, so we're done */
> > +            if (!recent)
> > +                break;
> > +
> > +            /*
> > +             * If the domid was specified then there's no point in
> > +             * trying again.
> > +             */
> > +            if (libxl_domid_valid_guest(info->domid)) {
> > +                LOGED(ERROR, *domid, "domain id recently used");
> > +                rc = ERROR_FAIL;
> > +                goto out;
> > +            }
> > +
> > +            /* Try to destroy the domain again as we can't use it */
> > +            ret = xc_domain_destroy(ctx->xch, *domid);
> > +            if (ret < 0) {
> > +                LOGED(ERROR, *domid, "domain destroy fail");
> > +                *domid = INVALID_DOMID;
> > +                rc = ERROR_FAIL;
> > +                goto out;
> > +            }
> 
> These two seem to be in the wrong order.  Also if
> libxl__is_domid_recent fails, you leak the domain.
> 

No, the domain will not be leaked. The existing failure handling in libxl will 
clean up if *domid != INVALID_DOMID.

> This is sort of a result of you not treating `domid' as a `local
> [variable] referring to resources which might need cleaning up'.
> According to a strict reading of CODING_STYLE you should initialise it
> to -1 and the xc_domain_destroy out should be in the out block, but
> that would duplicate the call to destroy.
> 
> I don't mind exactly how you fix this, but please make sure not to
> leak the newly-created domain even in the error cases.
> 

I think the error handling is good (but obscured by the way libxl works), and 
there is no way to avoid leaking the domain if xc_domain_destroy() fails.

> > diff --git a/tools/libxl/libxl_internal.c b/tools/libxl/libxl_internal.c
> > index bbd4c6cba9..d93a75533f 100644
> > --- a/tools/libxl/libxl_internal.c
> > +++ b/tools/libxl/libxl_internal.c
> > @@ -234,7 +234,7 @@ void libxl__logv(libxl_ctx *ctx, xentoollog_level
> msglevel, int errnoval,
> >      fileline[sizeof(fileline)-1] = 0;
> >
> >      domain[0] = 0;
> > -    if (domid != INVALID_DOMID)
> > +    if (libxl_domid_valid_guest(domid))
> >          snprintf(domain, sizeof(domain), "Domain %"PRIu32":", domid);
> >   x:
> >      xtl_log(ctx->lg, msglevel, errnoval, "libxl",
> 
> This wants to be a separate patch.
> 

Ok.

> > diff --git a/xen/include/public/xen.h b/xen/include/public/xen.h
> > index d2198dffad..75b1619d0d 100644
> > --- a/xen/include/public/xen.h
> > +++ b/xen/include/public/xen.h
> > @@ -614,6 +614,9 @@ DEFINE_XEN_GUEST_HANDLE(mmuext_op_t);
> >  /* Idle domain. */
> >  #define DOMID_IDLE           xen_mk_uint(0x7FFF)
> >
> > +/* Mask for valid domain id values */
> > +#define DOMID_MASK           xen_mk_uint(0x7FFF)
> 
> This needs a hypervisor maintainer ack.
> 
> Please split it into its own patch, with a rationale, etc.
> 

Ok, but it has no rationale without the rest of this patch; I can only assert 
that it 'will be needed by a subsequent patch'.

  Paul

> Thanks,
> ian.

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel