Xen devel - Mar 2012 - [PATCH] tools: Reorder and add includes in libvchan

libxenvchan.h includes xen/sys/evtchn.h, which in the XenServer patchqueue
may contain domid_t values.  Just re-order the order of includes to make sure
that Xen-defined values make sense to the compiler.

Signed-off-by: George Dunlap <george.dunlap@eu.citrix.com>

diff --git a/tools/libvchan/init.c b/tools/libvchan/init.c
--- a/tools/libvchan/init.c
+++ b/tools/libvchan/init.c
@@ -41,6 +41,7 @@
 #include <fcntl.h>
 
 #include <xs.h>
+#include <xen/xen.h>
 #include <xen/sys/evtchn.h>
 #include <xen/sys/gntalloc.h>
 #include <xen/sys/gntdev.h>
diff --git a/tools/libvchan/libxenvchan.h b/tools/libvchan/libxenvchan.h
--- a/tools/libvchan/libxenvchan.h
+++ b/tools/libvchan/libxenvchan.h
@@ -43,9 +43,9 @@
  *  compile time, so the macros in ring.h cannot be used to access the rings.
  */
 
+#include <xenctrl.h>
 #include <xen/io/libxenvchan.h>
 #include <xen/sys/evtchn.h>
-#include <xenctrl.h>
 
 struct libxenvchan_ring {
 	/* Pointer into the shared page. Offsets into buffer. */

George Dunlap writes ("[Xen-devel] [PATCH] tools: Reorder and add includes
in libvchan"):
> libxenvchan.h includes xen/sys/evtchn.h, which in the XenServer patchqueue
> may contain domid_t values.  Just re-order the order of includes to make
sure
> that Xen-defined values make sense to the compiler.
Surely this should be fixed by having (XenServer''s) evtchn.h include
xen.h itself ?

Ian.

On Tue, 2012-03-13 at 16:30 +0000, Ian Jackson wrote:
> George Dunlap writes ("[Xen-devel] [PATCH] tools: Reorder and add
includes in libvchan"):
> > libxenvchan.h includes xen/sys/evtchn.h, which in the XenServer
patchqueue
> > may contain domid_t values.  Just re-order the order of includes to
make sure
> > that Xen-defined values make sense to the compiler.
> 
> Surely this should be fixed by having (XenServer''s) evtchn.h
include
> xen.h itself ?
Well, that''s what I thought, but it''s not what ijc thought
(IIRC).  I''m
not really fussed either way, as long as I can get this patch out of the
patchqueue.  Just let me know what you guys want.

 -George

On Tue, 2012-03-13 at 16:36 +0000, George Dunlap wrote:
> On Tue, 2012-03-13 at 16:30 +0000, Ian Jackson wrote:
> > George Dunlap writes ("[Xen-devel] [PATCH] tools: Reorder and add
includes in libvchan"):
> > > libxenvchan.h includes xen/sys/evtchn.h, which in the XenServer
patchqueue
> > > may contain domid_t values.  Just re-order the order of includes
to make sure
> > > that Xen-defined values make sense to the compiler.
> > 
> > Surely this should be fixed by having (XenServer''s) evtchn.h
include
> > xen.h itself ?
> 
> Well, that''s what I thought, but it''s not what ijc
thought (IIRC).
I can''t remember that, but that''s probably just me being
forgetful
and/or I''ve not made the link between that conversation and this patch.

What is the related patch which adds the domid''s to evtchn.h? Are we
going to see it upstream at some point too?

I notice that upstream tools/include/xen-sys/Linux/evtchn.h (which I
presume to be the header in question) uses "unsigned int" for the
various domain ids which it contains. ISTM that either a) XS should use
"unsigned int" too or b) they should be domid_t in upstream too.

I expect s/unsigned int/domid_t/ == an ABI change so I think only "a)"
is actually an option, although maybe "b)" is an option if you are
careful/clever with the padding.
>   I''m
> not really fussed either way, as long as I can get this patch out of the
> patchqueue.  Just let me know what you guys want.
> 
>  -George
>

On Tue, 2012-03-13 at 16:52 +0000, Ian Campbell wrote:
> On Tue, 2012-03-13 at 16:36 +0000, George Dunlap wrote:
> > On Tue, 2012-03-13 at 16:30 +0000, Ian Jackson wrote:
> > > George Dunlap writes ("[Xen-devel] [PATCH] tools: Reorder
and add includes in libvchan"):
> > > > libxenvchan.h includes xen/sys/evtchn.h, which in the
XenServer patchqueue
> > > > may contain domid_t values.  Just re-order the order of
includes to make sure
> > > > that Xen-defined values make sense to the compiler.
> > > 
> > > Surely this should be fixed by having (XenServer''s)
evtchn.h include
> > > xen.h itself ?
> > 
> > Well, that''s what I thought, but it''s not what ijc
thought (IIRC).
> 
> I can''t remember that, but that''s probably just me being
forgetful
> and/or I''ve not made the link between that conversation and this
patch.
Yeah, I''m having trouble reconstructing the exact conversation too.  It
just had to do with it being OK to assume that the #include-r had
included other things already.
> What is the related patch which adds the domid''s to evtchn.h? Are
we
> going to see it upstream at some point too?
Unfortunately not; it''s the ioctl having to do with the qemu privilege
restriction stuff; not suitable for upstream (IIRC), since it requires
Linux to know details of, and be able to enforce, the hypercall
interaction between the tools and Xen.
> I notice that upstream tools/include/xen-sys/Linux/evtchn.h (which I
> presume to be the header in question) uses "unsigned int" for the
> various domain ids which it contains. ISTM that either a) XS should use
> "unsigned int" too or b) they should be domid_t in upstream too.
> 
> I expect s/unsigned int/domid_t/ == an ABI change so I think only
"a)"
> is actually an option, although maybe "b)" is an option if you
are
> careful/clever with the padding.
I think I''d be happy with a.  I had actually considered it but
discarded
it for some reason I can''t seem to remember now.

Let me check if ''a'' will work; if not, let me see if adding
xen/xen.h to
evtchn.h will work.

 -George

On Tue, 2012-03-13 at 17:09 +0000, George Dunlap wrote:
> On Tue, 2012-03-13 at 16:52 +0000, Ian Campbell wrote:
> > On Tue, 2012-03-13 at 16:36 +0000, George Dunlap wrote:
> > > On Tue, 2012-03-13 at 16:30 +0000, Ian Jackson wrote:
> > > > George Dunlap writes ("[Xen-devel] [PATCH] tools:
Reorder and add includes in libvchan"):
> > > > > libxenvchan.h includes xen/sys/evtchn.h, which in the
XenServer patchqueue
> > > > > may contain domid_t values.  Just re-order the order of
includes to make sure
> > > > > that Xen-defined values make sense to the compiler.
> > > > 
> > > > Surely this should be fixed by having (XenServer''s)
evtchn.h include
> > > > xen.h itself ?
> > > 
> > > Well, that''s what I thought, but it''s not what
ijc thought (IIRC).
> > 
> > I can''t remember that, but that''s probably just me
being forgetful
> > and/or I''ve not made the link between that conversation and
this patch.
> 
> Yeah, I''m having trouble reconstructing the exact conversation
too.  It
> just had to do with it being OK to assume that the #include-r had
> included other things already.
IIRC I think my point then was that both styles had proponents and
arguments for why their was the right way ;-).
> 
> > What is the related patch which adds the domid''s to evtchn.h?
Are we
> > going to see it upstream at some point too?
> 
> Unfortunately not; it''s the ioctl having to do with the qemu
privilege
> restriction stuff; not suitable for upstream (IIRC), since it requires
> Linux to know details of, and be able to enforce, the hypercall
> interaction between the tools and Xen.
Right, no problem.
> > I notice that upstream tools/include/xen-sys/Linux/evtchn.h (which I
> > presume to be the header in question) uses "unsigned int"
for the
> > various domain ids which it contains. ISTM that either a) XS should
use
> > "unsigned int" too or b) they should be domid_t in upstream
too.
> > 
> > I expect s/unsigned int/domid_t/ == an ABI change so I think only
"a)"
> > is actually an option, although maybe "b)" is an option if
you are
> > careful/clever with the padding.
> 
> I think I''d be happy with a.  I had actually considered it but
discarded
> it for some reason I can''t seem to remember now.
> 
> Let me check if ''a'' will work; if not, let me see if
adding xen/xen.h to
> evtchn.h will work.
OK, thanks!

Ian.