Re: [Xen-devel] [PATCH v4 1/7] libxl: add definition of INVALID_DOMID to the API

["Durrant, Paul" <pdurrant@amazon.co.uk>]

> -----Original Message-----
> From: Roger Pau Monné <roger.pau@citrix.com>
> Sent: 31 January 2020 11:06
> To: Durrant, Paul <pdurrant@amazon.co.uk>
> Cc: Anthony PERARD <anthony.perard@citrix.com>; xen-
> devel@lists.xenproject.org; Ian Jackson <ian.jackson@eu.citrix.com>; Wei
> Liu <wl@xen.org>
> Subject: Re: [Xen-devel] [PATCH v4 1/7] libxl: add definition of
> INVALID_DOMID to the API
> 
> On Fri, Jan 31, 2020 at 10:31:49AM +0000, Durrant, Paul wrote:
> > > -----Original Message-----
> > > From: Xen-devel <xen-devel-bounces@lists.xenproject.org> On Behalf Of
> > > Roger Pau Monné
> > > Sent: 22 January 2020 14:53
> > > To: Durrant, Paul <pdurrant@amazon.co.uk>
> > > Cc: Anthony PERARD <anthony.perard@citrix.com>; xen-
> > > devel@lists.xenproject.org; Ian Jackson <ian.jackson@eu.citrix.com>;
> Wei
> > > Liu <wl@xen.org>
> > > Subject: Re: [Xen-devel] [PATCH v4 1/7] libxl: add definition of
> > > INVALID_DOMID to the API
> > >
> > > On Wed, Jan 22, 2020 at 02:44:40PM +0000, Paul Durrant wrote:
> > > > Currently both xl and libxl have internal definitions of
> INVALID_DOMID
> > > > which happen to be identical. However, for the purposes of
> describing
> > > the
> > > > behaviour of libxl_domain_create_new/restore() it is useful to have
> a
> > > > specified invalid value for a domain id.
> > > >
> > > > This patch therefore moves the libxl definition from
> libxl_internal.h to
> > > > libxl.h and removes the internal definition from xl_utils.h. The
> > > hardcoded
> > > > '-1' passed back via domcreate_complete() is then updated to
> > > INVALID_DOMID
> > > > and comment above libxl_domain_create_new/restore() is accordingly
> > > > modified.
> > >
> > > Urg, it's kind of ugly to add another definition of invalid domid when
> > > there's already DOMID_INVALID in the public headers. I guess there's a
> > > reason I'm missing for not using DOMID_INVALID instead of introducing
> > > a new value?
> > >
> >
> > TBH I don't know. As far as xl/libxl goes, domids are uint32_ts so maybe
> DOMID_INVALID was for some reason not considered appropriate? Bear in
> mind, this patch is not truly introducing a new value; it's just making
> something that was internal but identical in both xl and libxl part of the
> interface.
> 
> Hm, OK. It seems quite a mess that libxl uses a 32bit value when the
> hypervisor is using a 16bit field, but I guess there's nothing that
> can be done at this point to fix this.
> 
> Since this will be part of the public interface now, it might make
> sense to define it to the same value as DOMID_INVALID (0x7FF4). And
> make sure domid values passed to libxl are truncated to 16bits.

That sounds like feature creep that I'd rather not go near in this patch. I suspect a can of worms awaits.

> 
> Maybe it's not that relevant, but domids passed to libxl would need to
> be sanitized in order to make sure Xen's DOMID_INVALID is not treated
> as a valid domid value from libxl's PoV. There are also other special
> domids that need to be filtered.

There is already a validity check: libxl_domid_valid_guest().

  Paul

> 
> > > If so could this be mentioned in the commit message?
> > >
> >
> > I'll add a note to the commit comment to point out that this is not
> changing any value used by the toolstack.
> 
> Thanks!
> 
> Roger.
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel