[v4,1/7] libxl: add definition of INVALID_DOMID to the API

Commit Message

Paul Durrant Jan. 22, 2020, 2:44 p.m. UTC

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.

Signed-off-by: Paul Durrant <[e-mail]>
Acked-by: Ian Jackson <[e-mail]>
---
Cc: Wei Liu <[e-mail]>
Cc: Anthony PERARD <[e-mail]>
---
 tools/libxl/libxl.h          | 4 +++-
 tools/libxl/libxl_create.c   | 2 +-
 tools/libxl/libxl_internal.h | 1 -
 tools/xl/xl_utils.h          | 2 --
 4 files changed, 4 insertions(+), 5 deletions(-)

Comments

Roger Pau Monné Jan. 22, 2020, 2:52 p.m. UTC | #1

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?

If so could this be mentioned in the commit message?

Thanks, Roger.

Ian Jackson Jan. 30, 2020, 5:31 p.m. UTC | #2

Paul Durrant writes ("[PATCH v4 1/7] libxl: add definition of INVALID_DOMID to the API"):
> 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.

Hi.  (I'm replying to the 1/ here because I don't seem to have any 0/
in my inbox...)

Thanks for all this.  As well as your use case, which is in itself
valuable, I think this change is important for other reasons.
So I want to see it go in.

You'll see I have replied with some comments about the implementation.
I hope you won't be discouraged.  If you feel I am asking for too much
work I might be inclined to pick up some of this myself; if so, please
let me know.  I definitely don't want this to be dropped.

Thanks.

Regards,
Ian.

Durrant, Paul Jan. 30, 2020, 5:35 p.m. UTC | #3

> -----Original Message-----
> From: Xen-devel <[e-mail]> On Behalf Of Ian
> Jackson
> Sent: 30 January 2020 17:32
> To: Durrant, Paul <[e-mail]>
> Cc: Anthony Perard <[e-mail]>; xen-
> [e-mail]; Wei Liu <[e-mail]>
> Subject: Re: [Xen-devel] [PATCH v4 1/7] libxl: add definition of
> INVALID_DOMID to the API
> 
> Paul Durrant writes ("[PATCH v4 1/7] libxl: add definition of
> INVALID_DOMID to the API"):
> > 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.
> 
> Hi.  (I'm replying to the 1/ here because I don't seem to have any 0/
> in my inbox...)
> 

Oh, I must have forgot to copy the combined cc list onto the cover letter. Sorry about that.

> Thanks for all this.  As well as your use case, which is in itself
> valuable, I think this change is important for other reasons.
> So I want to see it go in.
> 
> You'll see I have replied with some comments about the implementation.
> I hope you won't be discouraged.  If you feel I am asking for too much
> work I might be inclined to pick up some of this myself; if so, please
> let me know.  I definitely don't want this to be dropped.

Don't worry, your feedback is fine... certainly not asking too much.

  Cheers,

    Paul

Ian Jackson Jan. 30, 2020, 5:51 p.m. UTC | #4

> > Hi.  (I'm replying to the 1/ here because I don't seem to have any 0/
> > in my inbox...)
> 
> Oh, I must have forgot to copy the combined cc list onto the cover letter. Sorry about that.

There's a bug in git-send-email in this area.

> Don't worry, your feedback is fine... certainly not asking too much.

Good, thanks,

Ian.

Durrant, Paul Jan. 31, 2020, 10:31 a.m. UTC | #5

> -----Original Message-----
> From: Xen-devel <[e-mail]> On Behalf Of
> Roger Pau Monné
> Sent: 22 January 2020 14:53
> To: Durrant, Paul <[e-mail]>
> Cc: Anthony PERARD <[e-mail]>; xen-
> [e-mail]; Ian Jackson <[e-mail]>; Wei
> Liu <[e-mail]>
> 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.

> 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.

  Paul

> Thanks, Roger.
> 
> _______________________________________________
> Xen-devel mailing list
> [e-mail]
> https://lists.xenproject.org/mailman/listinfo/xen-devel

Roger Pau Monné Jan. 31, 2020, 11:06 a.m. UTC | #6

On Fri, Jan 31, 2020 at 10:31:49AM +0000, Durrant, Paul wrote:
> > -----Original Message-----
> > From: Xen-devel <[e-mail]> On Behalf Of
> > Roger Pau Monné
> > Sent: 22 January 2020 14:53
> > To: Durrant, Paul <[e-mail]>
> > Cc: Anthony PERARD <[e-mail]>; xen-
> > [e-mail]; Ian Jackson <[e-mail]>; Wei
> > Liu <[e-mail]>
> > 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.

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.

> > 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.

Durrant, Paul Jan. 31, 2020, 11:10 a.m. UTC | #7

> -----Original Message-----
> From: Roger Pau Monné <[e-mail]>
> Sent: 31 January 2020 11:06
> To: Durrant, Paul <[e-mail]>
> Cc: Anthony PERARD <[e-mail]>; xen-
> [e-mail]; Ian Jackson <[e-mail]>; Wei
> Liu <[e-mail]>
> 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 <[e-mail]> On Behalf Of
> > > Roger Pau Monné
> > > Sent: 22 January 2020 14:53
> > > To: Durrant, Paul <[e-mail]>
> > > Cc: Anthony PERARD <[e-mail]>; xen-
> > > [e-mail]; Ian Jackson <[e-mail]>;
> Wei
> > > Liu <[e-mail]>
> > > 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.

Andrew Cooper Jan. 31, 2020, 12:07 p.m. UTC | #8

On 31/01/2020 11:10, Durrant, Paul wrote:
>> -----Original Message-----
>> From: Roger Pau Monné <[e-mail]>
>> Sent: 31 January 2020 11:06
>> To: Durrant, Paul <[e-mail]>
>> Cc: Anthony PERARD <[e-mail]>; xen-
>> [e-mail]; Ian Jackson <[e-mail]>; Wei
>> Liu <[e-mail]>
>> 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 <[e-mail]> On Behalf Of
>>>> Roger Pau Monné
>>>> Sent: 22 January 2020 14:53
>>>> To: Durrant, Paul <[e-mail]>
>>>> Cc: Anthony PERARD <[e-mail]>; xen-
>>>> [e-mail]; Ian Jackson <[e-mail]>;
>> Wei
>>>> Liu <[e-mail]>
>>>> 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?

Read the commit message where I did the blanket change to use uint32_t.

c/s 5b42c82f55

Does this really need to enter the API?

~Andrew

Durrant, Paul Jan. 31, 2020, 12:11 p.m. UTC | #9

> -----Original Message-----
> From: Andrew Cooper <[e-mail]>
> Sent: 31 January 2020 12:08
> To: Durrant, Paul <[e-mail]>; Roger Pau Monné
> <[e-mail]>
> Cc: Anthony PERARD <[e-mail]>; xen-
> [e-mail]; Ian Jackson <[e-mail]>; Wei
> Liu <[e-mail]>
> Subject: Re: [Xen-devel] [PATCH v4 1/7] libxl: add definition of
> INVALID_DOMID to the API
> 
> On 31/01/2020 11:10, Durrant, Paul wrote:
> >> -----Original Message-----
> >> From: Roger Pau Monné <[e-mail]>
> >> Sent: 31 January 2020 11:06
> >> To: Durrant, Paul <[e-mail]>
> >> Cc: Anthony PERARD <[e-mail]>; xen-
> >> [e-mail]; Ian Jackson <[e-mail]>;
> Wei
> >> Liu <[e-mail]>
> >> 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 <[e-mail]> On Behalf Of
> >>>> Roger Pau Monné
> >>>> Sent: 22 January 2020 14:53
> >>>> To: Durrant, Paul <[e-mail]>
> >>>> Cc: Anthony PERARD <[e-mail]>; xen-
> >>>> [e-mail]; Ian Jackson <[e-mail]>;
> >> Wei
> >>>> Liu <[e-mail]>
> >>>> 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?
> 
> Read the commit message where I did the blanket change to use uint32_t.
> 
> c/s 5b42c82f55
> 
> Does this really need to enter the API?
> 

I need a 'magic' value for RANDOM_DOMID so it seemed reasonable to make this part of the xl/libxl API too. I don't think libxc is in scope here.

  Paul

> ~Andrew

Patch

diff --git a/tools/libxl/libxl.h b/tools/libxl/libxl.h
index 54abb9db1f..18c1a2d6bf 100644
--- a/tools/libxl/libxl.h
+++ b/tools/libxl/libxl.h
@@ -1527,9 +1527,11 @@  int libxl_ctx_free(libxl_ctx *ctx /* 0 is OK */);
 
 /* domain related functions */
 
+#define INVALID_DOMID ~0
+
 /* 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
- * domain id, or -1, as appropriate */
+ * domain id, or INVALID_DOMID, as appropriate */
 
 int libxl_domain_create_new(libxl_ctx *ctx, libxl_domain_config *d_config,
                             uint32_t *domid,
diff --git a/tools/libxl/libxl_create.c b/tools/libxl/libxl_create.c
index 69fceff061..8a1bff6cd3 100644
--- a/tools/libxl/libxl_create.c
+++ b/tools/libxl/libxl_create.c
@@ -1773,7 +1773,7 @@  static void domcreate_complete(libxl__egc *egc,
             libxl__domain_destroy(egc, &dcs->dds);
             return;
         }
-        dcs->guest_domid = -1;
+        dcs->guest_domid = INVALID_DOMID;
     }
     dcs->callback(egc, dcs, rc, dcs->guest_domid);
 }
diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
index d919f91882..f2f753c72b 100644
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -121,7 +121,6 @@ 
 #define STUBDOM_SPECIAL_CONSOLES 3
 #define TAP_DEVICE_SUFFIX "-emu"
 #define DOMID_XS_PATH "domid"
-#define INVALID_DOMID ~0
 #define PVSHIM_BASENAME "xen-shim"
 #define PVSHIM_CMDLINE "pv-shim console=xen,pv"
 
diff --git a/tools/xl/xl_utils.h b/tools/xl/xl_utils.h
index 7b9ccca30a..d98b419f10 100644
--- a/tools/xl/xl_utils.h
+++ b/tools/xl/xl_utils.h
@@ -52,8 +52,6 @@ 
 #define STR_SKIP_PREFIX( a, b ) \
     ( STR_HAS_PREFIX(a, b) ? ((a) += strlen(b), 1) : 0 )
 
-#define INVALID_DOMID ~0
-
 #define LOG(_f, _a...)   dolog(__FILE__, __LINE__, __func__, _f "\n", ##_a)
 
 /*