[PATCH] RFC: Make xen's public headers a little friendlier for C++.

[PATCH] RFC: Make xen's public headers a little friendlier for C++.

[Tim Deegan]

Shuffle some struct definitions up to file scope so that they remain
in scope in C++ when they're used again later.

Add an automatic check for similar C++ pitfalls, to be run only when
g++ is available.

RFC because it's not clear whether we want to make any commitments to
have the public headers be C++-friendly.

Explicitly _not_ addressing the use of 'private' in various fields,
since we'd previously decided not to fix that.

Reported-by: Razvan Cojocaru <rcojocaru@bitdefender.com>
Signed-off-by: Tim Deegan <tim@xen.org>
Cc: Jan Beulich <JBeulich@suse.com>
---
 .gitignore                    |  1 +
 xen/include/Makefile          | 12 +++++++++---
 xen/include/public/platform.h | 39 ++++++++++++++++++++++-----------------
 3 files changed, 32 insertions(+), 20 deletions(-)

diff --git a/.gitignore b/.gitignore
index 13ee05b..78958ea 100644
--- a/.gitignore
+++ b/.gitignore
@@ -233,6 +233,7 @@ xen/arch/*/efi/compat.c
 xen/arch/*/efi/efi.h
 xen/arch/*/efi/runtime.c
 xen/include/headers.chk
+xen/include/headers++.chk
 xen/include/asm
 xen/include/asm-*/asm-offsets.h
 xen/include/compat/*
diff --git a/xen/include/Makefile b/xen/include/Makefile
index 94112d1..c361877 100644
--- a/xen/include/Makefile
+++ b/xen/include/Makefile
@@ -87,13 +87,19 @@ compat/xlat.h: $(addprefix compat/.xlat/,$(xlat-y)) Makefile
 
 ifeq ($(XEN_TARGET_ARCH),$(XEN_COMPILE_ARCH))
 
-all: headers.chk
+all: headers.chk headers++.chk
 
-headers.chk: $(filter-out public/arch-% public/%ctl.h public/xsm/% public/%hvm/save.h, $(wildcard public/*.h public/*/*.h) $(public-y)) Makefile
+PUBLIC_HEADERS_TO_CHECK := $(filter-out public/arch-% public/%ctl.h public/xsm/% public/%hvm/save.h, $(wildcard public/*.h public/*/*.h) $(public-y))
+
+headers.chk: $(PUBLIC_HEADERS_TO_CHECK) Makefile
 	for i in $(filter %.h,$^); do $(CC) -ansi -include stdint.h -Wall -W -Werror -S -o /dev/null -x c $$i || exit 1; echo $$i; done >$@.new
 	mv $@.new $@
 
+headers++.chk: $(PUBLIC_HEADERS_TO_CHECK) Makefile
+	if g++ -v >/dev/null; then for i in $(filter %.h,$^); do g++ -ansi -include stdint.h -Wall -W -Werror -S -o /dev/null -x c++ -Dprivate=private_is_a_keyword_in_c_plus_plus $$i || exit 1; echo $$i; done ; fi >$@.new
+	mv $@.new $@
+
 endif
 
 clean::
-	rm -rf compat headers.chk
+	rm -rf compat headers.chk headers++.chk
diff --git a/xen/include/public/platform.h b/xen/include/public/platform.h
index 3e340b4..dd03447 100644
--- a/xen/include/public/platform.h
+++ b/xen/include/public/platform.h
@@ -126,6 +126,26 @@ DEFINE_XEN_GUEST_HANDLE(xenpf_platform_quirk_t);
 #define XEN_EFI_query_variable_info           9
 #define XEN_EFI_query_capsule_capabilities   10
 #define XEN_EFI_update_capsule               11
+
+struct xenpf_efi_guid {
+    uint32_t data1;
+    uint16_t data2;
+    uint16_t data3;
+    uint8_t data4[8];
+};
+
+struct xenpf_efi_time {
+    uint16_t year;
+    uint8_t month;
+    uint8_t day;
+    uint8_t hour;
+    uint8_t min;
+    uint8_t sec;
+    uint32_t ns;
+    int16_t tz;
+    uint8_t daylight;
+};
+
 struct xenpf_efi_runtime_call {
     uint32_t function;
     /*
@@ -138,17 +158,7 @@ struct xenpf_efi_runtime_call {
     union {
 #define XEN_EFI_GET_TIME_SET_CLEARS_NS 0x00000001
         struct {
-            struct xenpf_efi_time {
-                uint16_t year;
-                uint8_t month;
-                uint8_t day;
-                uint8_t hour;
-                uint8_t min;
-                uint8_t sec;
-                uint32_t ns;
-                int16_t tz;
-                uint8_t daylight;
-            } time;
+            struct xenpf_efi_time time;
             uint32_t resolution;
             uint32_t accuracy;
         } get_time;
@@ -170,12 +180,7 @@ struct xenpf_efi_runtime_call {
             XEN_GUEST_HANDLE(void) name;  /* UCS-2/UTF-16 string */
             xen_ulong_t size;
             XEN_GUEST_HANDLE(void) data;
-            struct xenpf_efi_guid {
-                uint32_t data1;
-                uint16_t data2;
-                uint16_t data3;
-                uint8_t data4[8];
-            } vendor_guid;
+            struct xenpf_efi_guid vendor_guid;
         } get_variable, set_variable;
 
         struct {
-- 
2.1.4

Re: [PATCH] RFC: Make xen's public headers a little friendlier for C++.

[Jan Beulich]

>>> On 26.02.15 at 14:11, <tim@xen.org> wrote:
> Shuffle some struct definitions up to file scope so that they remain
> in scope in C++ when they're used again later.
> 
> Add an automatic check for similar C++ pitfalls, to be run only when
> g++ is available.
> 
> RFC because it's not clear whether we want to make any commitments to
> have the public headers be C++-friendly.

I like this, and it looks it was easier to do than I thought. Albeit ...

> --- a/xen/include/Makefile
> +++ b/xen/include/Makefile
> @@ -87,13 +87,19 @@ compat/xlat.h: $(addprefix compat/.xlat/,$(xlat-y)) Makefile
>  
>  ifeq ($(XEN_TARGET_ARCH),$(XEN_COMPILE_ARCH))
>  
> -all: headers.chk
> +all: headers.chk headers++.chk
>  
> -headers.chk: $(filter-out public/arch-% public/%ctl.h public/xsm/% public/%hvm/save.h, $(wildcard public/*.h public/*/*.h) $(public-y)) Makefile
> +PUBLIC_HEADERS_TO_CHECK := $(filter-out public/arch-% public/%ctl.h public/xsm/% public/%hvm/save.h, $(wildcard public/*.h public/*/*.h) $(public-y))
> +
> +headers.chk: $(PUBLIC_HEADERS_TO_CHECK) Makefile
>  	for i in $(filter %.h,$^); do $(CC) -ansi -include stdint.h -Wall -W -Werror -S -o /dev/null -x c $$i || exit 1; echo $$i; done >$@.new
>  	mv $@.new $@
>  
> +headers++.chk: $(PUBLIC_HEADERS_TO_CHECK) Makefile

... I don't think limiting this to a subset of the headers is the right
thing here: C++ consumers are (most likely) going to be user space,
i.e. tools, and those would want to be able to use those excluded
headers.

> +	if g++ -v >/dev/null; then for i in $(filter %.h,$^); do g++ -ansi -include stdint.h -Wall -W -Werror -S -o /dev/null -x c++ -Dprivate=private_is_a_keyword_in_c_plus_plus $$i || exit 1; echo $$i; done ; fi >$@.new

You may want to define __XEN_TOOLS__ (and un-define __XEN__)
here. Also g++ ought to by abstracted to $(CXX), and I don't see
how this step is being avoided when there's no C++ compiler there.

> --- a/xen/include/public/platform.h
> +++ b/xen/include/public/platform.h

These changes went in a few minutes ago.

Jan

Re: [PATCH] RFC: Make xen's public headers a little friendlier for C++.

[Tim Deegan]

At 14:09 +0000 on 26 Feb (1424956161), Jan Beulich wrote:
> >>> On 26.02.15 at 14:11, <tim@xen.org> wrote:
> > -headers.chk: $(filter-out public/arch-% public/%ctl.h public/xsm/% public/%hvm/save.h, $(wildcard public/*.h public/*/*.h) $(public-y)) Makefile
> > +PUBLIC_HEADERS_TO_CHECK := $(filter-out public/arch-% public/%ctl.h public/xsm/% public/%hvm/save.h, $(wildcard public/*.h public/*/*.h) $(public-y))
> > +
> > +headers.chk: $(PUBLIC_HEADERS_TO_CHECK) Makefile
> >  	for i in $(filter %.h,$^); do $(CC) -ansi -include stdint.h -Wall -W -Werror -S -o /dev/null -x c $$i || exit 1; echo $$i; done >$@.new
> >  	mv $@.new $@
> >  
> > +headers++.chk: $(PUBLIC_HEADERS_TO_CHECK) Makefile
> 
> ... I don't think limiting this to a subset of the headers is the right
> thing here: C++ consumers are (most likely) going to be user space,
> i.e. tools, and those would want to be able to use those excluded
> headers.

OK.  I'll have a look through that list.  I presume I'll still want to
exclude e.g. the arch/ headers on the grounds that users shouldn't
be including them directly (and we won't want cross-arch includes)?

If I'm extending this to cover more headers than the ANSI-C check
does, should I also relax the '-ansi' requirement to, e.g. '-std=gnu++98'?

> > +	if g++ -v >/dev/null; then for i in $(filter %.h,$^); do g++ -ansi -include stdint.h -Wall -W -Werror -S -o /dev/null -x c++ -Dprivate=private_is_a_keyword_in_c_plus_plus $$i || exit 1; echo $$i; done ; fi >$@.new
> 
> You may want to define __XEN_TOOLS__ (and un-define __XEN__)
> here.

OK.  I wonder how many more things will break when I do that. :)

> Also g++ ought to by abstracted to $(CXX)

Sure, I'll define up $(CXX) in StdGnu.mk.  

> and I don't see
> how this step is being avoided when there's no C++ compiler there.

if g++ isn't on the path, 'g++ -v' fails and we end up with an empty
headers++.chk file.  It doesn't deal with a present-but-broken g++ but
that way lies autoconf.

> > --- a/xen/include/public/platform.h
> > +++ b/xen/include/public/platform.h
> 
> These changes went in a few minutes ago.

Good-oh; I'll drop them from a v2.

Tim.

Re: [PATCH] RFC: Make xen's public headers a little friendlier for C++.

[Jan Beulich]

>>> On 26.02.15 at 16:22, <tim@xen.org> wrote:
> At 14:09 +0000 on 26 Feb (1424956161), Jan Beulich wrote:
>> >>> On 26.02.15 at 14:11, <tim@xen.org> wrote:
>> > +headers++.chk: $(PUBLIC_HEADERS_TO_CHECK) Makefile
>> 
>> ... I don't think limiting this to a subset of the headers is the right
>> thing here: C++ consumers are (most likely) going to be user space,
>> i.e. tools, and those would want to be able to use those excluded
>> headers.
> 
> OK.  I'll have a look through that list.  I presume I'll still want to
> exclude e.g. the arch/ headers on the grounds that users shouldn't
> be including them directly (and we won't want cross-arch includes)?

Aren't even our tools doing cross-arch includes. I've been trying to
remember why they're excluded from the C check, but can't.

> If I'm extending this to cover more headers than the ANSI-C check
> does, should I also relax the '-ansi' requirement to, e.g. '-std=gnu++98'?

I think that would be reasonable nowadays. Anyone wanting
compatibility farther back could contribute a patch...

>> and I don't see
>> how this step is being avoided when there's no C++ compiler there.
> 
> if g++ isn't on the path, 'g++ -v' fails and we end up with an empty
> headers++.chk file.

The line is so long that I overlooked that "if g++ -v" at the
beginning (having expected some different mechanism, like
suppressing the dependency in that case). Sorry for the noise.

Jan

[PATCH v2] RFC: Automatically check xen's public headers for C++ pitfalls.

[Tim Deegan]

Add a check, like the existing check for non-ANSI C in the public
headers, that runs the public headers through a C++ compiler to
flag non-C++-friendly constructs.

Unlike the ANSI C check, we accept GCC-isms (gnu++98), and we also
check various tools-only headers.

Explicitly _not_ addressing the use of 'private' in various fields,
since we'd previously decided not to fix that.

Also tidy up the runes for these checks to be a bit more readable.

Reported-by: Razvan Cojocaru <rcojocaru@bitdefender.com>
Signed-off-by: Tim Deegan <tim@xen.org>
Cc: Jan Beulich <JBeulich@suse.com>

---

v2: test more headers;
    define __XEN_TOOLS__;
    use g++98 rather than ansi;
    tidy the makefile for readability;
    add a missing include to flask_op.h, which uses evtchn_port_t.
---
 .gitignore                        |  1 +
 config/StdGNU.mk                  |  2 ++
 config/SunOS.mk                   |  1 +
 xen/include/Makefile              | 28 ++++++++++++++++++++++++----
 xen/include/public/platform.h     | 39 ++++++++++++++++++++++-----------------
 xen/include/public/xsm/flask_op.h |  2 ++
 6 files changed, 52 insertions(+), 21 deletions(-)

diff --git a/.gitignore b/.gitignore
index 13ee05b..78958ea 100644
--- a/.gitignore
+++ b/.gitignore
@@ -233,6 +233,7 @@ xen/arch/*/efi/compat.c
 xen/arch/*/efi/efi.h
 xen/arch/*/efi/runtime.c
 xen/include/headers.chk
+xen/include/headers++.chk
 xen/include/asm
 xen/include/asm-*/asm-offsets.h
 xen/include/compat/*
diff --git a/config/StdGNU.mk b/config/StdGNU.mk
index 4efebe3..e10ed39 100644
--- a/config/StdGNU.mk
+++ b/config/StdGNU.mk
@@ -2,9 +2,11 @@ AS         = $(CROSS_COMPILE)as
 LD         = $(CROSS_COMPILE)ld
 ifeq ($(clang),y)
 CC         = $(CROSS_COMPILE)clang
+CXX        = $(CROSS_COMPILE)clang++
 LD_LTO     = $(CROSS_COMPILE)llvm-ld
 else
 CC         = $(CROSS_COMPILE)gcc
+CXX        = $(CROSS_COMPILE)g++
 LD_LTO     = $(CROSS_COMPILE)ld
 endif
 CPP        = $(CC) -E
diff --git a/config/SunOS.mk b/config/SunOS.mk
index 3316280..c2be37d 100644
--- a/config/SunOS.mk
+++ b/config/SunOS.mk
@@ -2,6 +2,7 @@ AS         = $(CROSS_COMPILE)gas
 LD         = $(CROSS_COMPILE)gld
 CC         = $(CROSS_COMPILE)gcc
 CPP        = $(CROSS_COMPILE)gcc -E
+CXX        = $(CROSS_COMPILE)g++
 AR         = $(CROSS_COMPILE)gar
 RANLIB     = $(CROSS_COMPILE)granlib
 NM         = $(CROSS_COMPILE)gnm
diff --git a/xen/include/Makefile b/xen/include/Makefile
index 94112d1..d48a642 100644
--- a/xen/include/Makefile
+++ b/xen/include/Makefile
@@ -87,13 +87,33 @@ compat/xlat.h: $(addprefix compat/.xlat/,$(xlat-y)) Makefile
 
 ifeq ($(XEN_TARGET_ARCH),$(XEN_COMPILE_ARCH))
 
-all: headers.chk
+all: headers.chk headers++.chk
 
-headers.chk: $(filter-out public/arch-% public/%ctl.h public/xsm/% public/%hvm/save.h, $(wildcard public/*.h public/*/*.h) $(public-y)) Makefile
-	for i in $(filter %.h,$^); do $(CC) -ansi -include stdint.h -Wall -W -Werror -S -o /dev/null -x c $$i || exit 1; echo $$i; done >$@.new
+PUBLIC_HEADERS := $(filter-out public/arch-% public/dom0_ops.h, $(wildcard public/*.h public/*/*.h) $(public-y))
+
+PUBLIC_ANSI_HEADERS := $(filter-out public/%ctl.h public/xsm/% public/%hvm/save.h, $(PUBLIC_HEADERS))
+
+headers.chk: $(PUBLIC_ANSI_HEADERS) Makefile
+	for i in $(filter %.h,$^); do \
+	    $(CC) -x c -ansi -Wall -Werror -include stdint.h \
+	          -S -o /dev/null $$i || exit 1; \
+	    echo $$i; \
+	done >$@.new
+	mv $@.new $@
+
+headers++.chk: $(PUBLIC_HEADERS) Makefile
+	if $(CXX) -v >/dev/null 2>&1; then \
+	    for i in $(filter %.h,$^); do \
+	        $(CXX) -x c++ -std=gnu++98 -Wall -Werror \
+	               -D__XEN_TOOLS__ -Dprivate=private_is_a_keyword_in_cpp \
+	               -include stdint.h -include public/xen.h \
+	               -S -o /dev/null $$i || exit 1; \
+	        echo $$i; \
+	    done ; \
+	fi >$@.new
 	mv $@.new $@
 
 endif
 
 clean::
-	rm -rf compat headers.chk
+	rm -rf compat headers.chk headers++.chk
diff --git a/xen/include/public/platform.h b/xen/include/public/platform.h
index 3e340b4..dd03447 100644
--- a/xen/include/public/platform.h
+++ b/xen/include/public/platform.h
@@ -126,6 +126,26 @@ DEFINE_XEN_GUEST_HANDLE(xenpf_platform_quirk_t);
 #define XEN_EFI_query_variable_info           9
 #define XEN_EFI_query_capsule_capabilities   10
 #define XEN_EFI_update_capsule               11
+
+struct xenpf_efi_guid {
+    uint32_t data1;
+    uint16_t data2;
+    uint16_t data3;
+    uint8_t data4[8];
+};
+
+struct xenpf_efi_time {
+    uint16_t year;
+    uint8_t month;
+    uint8_t day;
+    uint8_t hour;
+    uint8_t min;
+    uint8_t sec;
+    uint32_t ns;
+    int16_t tz;
+    uint8_t daylight;
+};
+
 struct xenpf_efi_runtime_call {
     uint32_t function;
     /*
@@ -138,17 +158,7 @@ struct xenpf_efi_runtime_call {
     union {
 #define XEN_EFI_GET_TIME_SET_CLEARS_NS 0x00000001
         struct {
-            struct xenpf_efi_time {
-                uint16_t year;
-                uint8_t month;
-                uint8_t day;
-                uint8_t hour;
-                uint8_t min;
-                uint8_t sec;
-                uint32_t ns;
-                int16_t tz;
-                uint8_t daylight;
-            } time;
+            struct xenpf_efi_time time;
             uint32_t resolution;
             uint32_t accuracy;
         } get_time;
@@ -170,12 +180,7 @@ struct xenpf_efi_runtime_call {
             XEN_GUEST_HANDLE(void) name;  /* UCS-2/UTF-16 string */
             xen_ulong_t size;
             XEN_GUEST_HANDLE(void) data;
-            struct xenpf_efi_guid {
-                uint32_t data1;
-                uint16_t data2;
-                uint16_t data3;
-                uint8_t data4[8];
-            } vendor_guid;
+            struct xenpf_efi_guid vendor_guid;
         } get_variable, set_variable;
 
         struct {
diff --git a/xen/include/public/xsm/flask_op.h b/xen/include/public/xsm/flask_op.h
index 233de81..f874589 100644
--- a/xen/include/public/xsm/flask_op.h
+++ b/xen/include/public/xsm/flask_op.h
@@ -25,6 +25,8 @@
 #ifndef __FLASK_OP_H__
 #define __FLASK_OP_H__
 
+#include "../event_channel.h"
+
 #define XEN_FLASK_INTERFACE_VERSION 1
 
 struct xen_flask_load {
-- 
2.1.4

Re: [PATCH v2] RFC: Automatically check xen's public headers for C++ pitfalls.

[Tim Deegan]

At 16:11 +0000 on 26 Feb (1424963496), Tim Deegan wrote:
> Add a check, like the existing check for non-ANSI C in the public
> headers, that runs the public headers through a C++ compiler to
> flag non-C++-friendly constructs.

Oops, this still has the EFI changes in it.  v3, rebased, is on its way.

> Unlike the ANSI C check, we accept GCC-isms (gnu++98), and we also
> check various tools-only headers.
> 
> Explicitly _not_ addressing the use of 'private' in various fields,
> since we'd previously decided not to fix that.

BTW, ring.h is the only instance of that, so the extra diff to clear
that up too is pretty small (see below).

Not sure what people think about that though - it might be
quite a PITA for downstream users of it, though they ought really to
be using local copies so they can update in a controlled way.

diff --git a/xen/include/Makefile b/xen/include/Makefile
index d48a642..c7a1d52 100644
--- a/xen/include/Makefile
+++ b/xen/include/Makefile
@@ -104,8 +104,7 @@ headers.chk: $(PUBLIC_ANSI_HEADERS) Makefile
 headers++.chk: $(PUBLIC_HEADERS) Makefile
 	if $(CXX) -v >/dev/null 2>&1; then \
 	    for i in $(filter %.h,$^); do \
-	        $(CXX) -x c++ -std=gnu++98 -Wall -Werror \
-	               -D__XEN_TOOLS__ -Dprivate=private_is_a_keyword_in_cpp \
+	        $(CXX) -x c++ -std=gnu++98 -Wall -Werror -D__XEN_TOOLS__ \
 	               -include stdint.h -include public/xen.h \
 	               -S -o /dev/null $$i || exit 1; \
 	        echo $$i; \
diff --git a/xen/include/public/io/ring.h b/xen/include/public/io/ring.h
index 73e13d7..bb13494 100644
--- a/xen/include/public/io/ring.h
+++ b/xen/include/public/io/ring.h
@@ -111,7 +111,7 @@ struct __name##_sring {                                                 \
             uint8_t msg;                                                \
         } tapif_user;                                                   \
         uint8_t pvt_pad[4];                                             \
-    } private;                                                          \
+    } local;                                                            \
     uint8_t __pad[44];                                                  \
     union __name##_sring_entry ring[1]; /* variable-length */           \
 };                                                                      \
@@ -156,7 +156,7 @@ typedef struct __name##_back_ring __name##_back_ring_t
 #define SHARED_RING_INIT(_s) do {                                       \
     (_s)->req_prod  = (_s)->rsp_prod  = 0;                              \
     (_s)->req_event = (_s)->rsp_event = 1;                              \
-    (void)memset((_s)->private.pvt_pad, 0, sizeof((_s)->private.pvt_pad)); \
+    (void)memset((_s)->local.pvt_pad, 0, sizeof((_s)->local.pvt_pad));  \
     (void)memset((_s)->__pad, 0, sizeof((_s)->__pad));                  \
 } while(0)

Re: [PATCH v2] RFC: Automatically check xen's public headers for C++ pitfalls.

[Andrew Cooper]

On 26/02/15 16:28, Tim Deegan wrote:
> At 16:11 +0000 on 26 Feb (1424963496), Tim Deegan wrote:
>> Add a check, like the existing check for non-ANSI C in the public
>> headers, that runs the public headers through a C++ compiler to
>> flag non-C++-friendly constructs.
> Oops, this still has the EFI changes in it.  v3, rebased, is on its way.
>
>> Unlike the ANSI C check, we accept GCC-isms (gnu++98), and we also
>> check various tools-only headers.
>>
>> Explicitly _not_ addressing the use of 'private' in various fields,
>> since we'd previously decided not to fix that.
> BTW, ring.h is the only instance of that, so the extra diff to clear
> that up too is pretty small (see below).
>
> Not sure what people think about that though - it might be
> quite a PITA for downstream users of it, though they ought really to
> be using local copies so they can update in a controlled way.

It is basically no effort, wont (directly) break consumers, and will
make the headers fully friendly (other than extern C, which can be dealt
with using the C++ #include <c$FOO> pattern).

+1 throw this in and be done with the incompatibilities for good.

~Andrew

>
> diff --git a/xen/include/Makefile b/xen/include/Makefile
> index d48a642..c7a1d52 100644
> --- a/xen/include/Makefile
> +++ b/xen/include/Makefile
> @@ -104,8 +104,7 @@ headers.chk: $(PUBLIC_ANSI_HEADERS) Makefile
>  headers++.chk: $(PUBLIC_HEADERS) Makefile
>  	if $(CXX) -v >/dev/null 2>&1; then \
>  	    for i in $(filter %.h,$^); do \
> -	        $(CXX) -x c++ -std=gnu++98 -Wall -Werror \
> -	               -D__XEN_TOOLS__ -Dprivate=private_is_a_keyword_in_cpp \
> +	        $(CXX) -x c++ -std=gnu++98 -Wall -Werror -D__XEN_TOOLS__ \
>  	               -include stdint.h -include public/xen.h \
>  	               -S -o /dev/null $$i || exit 1; \
>  	        echo $$i; \
> diff --git a/xen/include/public/io/ring.h b/xen/include/public/io/ring.h
> index 73e13d7..bb13494 100644
> --- a/xen/include/public/io/ring.h
> +++ b/xen/include/public/io/ring.h
> @@ -111,7 +111,7 @@ struct __name##_sring {                                                 \
>              uint8_t msg;                                                \
>          } tapif_user;                                                   \
>          uint8_t pvt_pad[4];                                             \
> -    } private;                                                          \
> +    } local;                                                            \
>      uint8_t __pad[44];                                                  \
>      union __name##_sring_entry ring[1]; /* variable-length */           \
>  };                                                                      \
> @@ -156,7 +156,7 @@ typedef struct __name##_back_ring __name##_back_ring_t
>  #define SHARED_RING_INIT(_s) do {                                       \
>      (_s)->req_prod  = (_s)->rsp_prod  = 0;                              \
>      (_s)->req_event = (_s)->rsp_event = 1;                              \
> -    (void)memset((_s)->private.pvt_pad, 0, sizeof((_s)->private.pvt_pad)); \
> +    (void)memset((_s)->local.pvt_pad, 0, sizeof((_s)->local.pvt_pad));  \
>      (void)memset((_s)->__pad, 0, sizeof((_s)->__pad));                  \
>  } while(0)
>  
>
>
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel

Re: [PATCH v2] RFC: Automatically check xen's public headers for C++ pitfalls.

[Jan Beulich]

>>> On 26.02.15 at 17:28, <tim@xen.org> wrote:
> At 16:11 +0000 on 26 Feb (1424963496), Tim Deegan wrote:
>> Explicitly _not_ addressing the use of 'private' in various fields,
>> since we'd previously decided not to fix that.
> 
> BTW, ring.h is the only instance of that, so the extra diff to clear
> that up too is pretty small (see below).
> 
> Not sure what people think about that though - it might be
> quite a PITA for downstream users of it, though they ought really to
> be using local copies so they can update in a controlled way.

linux-2.6.18-xen.hg always having consumed them (almost)
verbatim, I don't think we should break users not massaging
the headers. I.e. at least make the field name conditional upon
using C vs C++.

Jan

Re: [PATCH v2] RFC: Automatically check xen's public headers for C++ pitfalls.

[Tim Deegan]

At 16:47 +0000 on 26 Feb (1424965651), Jan Beulich wrote:
> >>> On 26.02.15 at 17:28, <tim@xen.org> wrote:
> > At 16:11 +0000 on 26 Feb (1424963496), Tim Deegan wrote:
> >> Explicitly _not_ addressing the use of 'private' in various fields,
> >> since we'd previously decided not to fix that.
> > 
> > BTW, ring.h is the only instance of that, so the extra diff to clear
> > that up too is pretty small (see below).
> > 
> > Not sure what people think about that though - it might be
> > quite a PITA for downstream users of it, though they ought really to
> > be using local copies so they can update in a controlled way.
> 
> linux-2.6.18-xen.hg always having consumed them (almost)
> verbatim, I don't think we should break users not massaging
> the headers. I.e. at least make the field name conditional upon
> using C vs C++.

Something like this?  This is the kind of uglification that I would
like to avoid, though (and I don't like '#define private pvt' much
either).

Tim.

diff --git a/xen/include/Makefile b/xen/include/Makefile
index d48a642..c7a1d52 100644
--- a/xen/include/Makefile
+++ b/xen/include/Makefile
@@ -104,8 +104,7 @@ headers.chk: $(PUBLIC_ANSI_HEADERS) Makefile
 headers++.chk: $(PUBLIC_HEADERS) Makefile
 	if $(CXX) -v >/dev/null 2>&1; then \
 	    for i in $(filter %.h,$^); do \
-	        $(CXX) -x c++ -std=gnu++98 -Wall -Werror \
-	               -D__XEN_TOOLS__ -Dprivate=private_is_a_keyword_in_cpp \
+	        $(CXX) -x c++ -std=gnu++98 -Wall -Werror -D__XEN_TOOLS__ \
 	               -include stdint.h -include public/xen.h \
 	               -S -o /dev/null $$i || exit 1; \
 	        echo $$i; \
diff --git a/xen/include/public/io/ring.h b/xen/include/public/io/ring.h
index 73e13d7..86fb991 100644
--- a/xen/include/public/io/ring.h
+++ b/xen/include/public/io/ring.h
@@ -35,6 +35,15 @@
 #define xen_wmb() wmb()
 #endif
 
+#ifdef __cplusplus
+/* 'private' is a keyword in C++, so we have to use a different name for
+ * private state there.  Leaving the C name alone to avoid unnecessary
+ * pain for the existing users. */
+#define XEN_RING_PRIVATE pvt
+#else
+#define XEN_RING_PRIVATE private
+#endif
+
 typedef unsigned int RING_IDX;
 
 /* Round a 32-bit unsigned constant down to the nearest power of two. */
@@ -111,7 +120,7 @@ struct __name##_sring {                                                 \
             uint8_t msg;                                                \
         } tapif_user;                                                   \
         uint8_t pvt_pad[4];                                             \
-    } private;                                                          \
+    } XEN_RING_PRIVATE;                                                 \
     uint8_t __pad[44];                                                  \
     union __name##_sring_entry ring[1]; /* variable-length */           \
 };                                                                      \
@@ -156,7 +165,8 @@ typedef struct __name##_back_ring __name##_back_ring_t
 #define SHARED_RING_INIT(_s) do {                                       \
     (_s)->req_prod  = (_s)->rsp_prod  = 0;                              \
     (_s)->req_event = (_s)->rsp_event = 1;                              \
-    (void)memset((_s)->private.pvt_pad, 0, sizeof((_s)->private.pvt_pad)); \
+    (void)memset((_s)->XEN_RING_PRIVATE.pvt_pad, 0,                     \
+                 sizeof((_s)->XEN_RING_PRIVATE.pvt_pad));               \
     (void)memset((_s)->__pad, 0, sizeof((_s)->__pad));                  \
 } while(0)

Re: [PATCH v2] RFC: Automatically check xen's public headers for C++ pitfalls.

[Razvan Cojocaru]

On 02/26/2015 07:01 PM, Tim Deegan wrote:
> +#ifdef __cplusplus
> +/* 'private' is a keyword in C++, so we have to use a different name for
> + * private state there.  Leaving the C name alone to avoid unnecessary
> + * pain for the existing users. */
> +#define XEN_RING_PRIVATE pvt
> +#else
> +#define XEN_RING_PRIVATE private
> +#endif

Are there likely to be many users outside of the ones using that code
with mem_event? Because if there aren't, there are much more drastic
changes happening in Tamas' pending series, so perhaps seen that way the
change becomes more acceptable.


Thanks,
Razvan

Re: [PATCH v2] RFC: Automatically check xen's public headers for C++ pitfalls.

[Tim Deegan]

At 19:49 +0200 on 26 Feb (1424976562), Razvan Cojocaru wrote:
> On 02/26/2015 07:01 PM, Tim Deegan wrote:
> > +#ifdef __cplusplus
> > +/* 'private' is a keyword in C++, so we have to use a different name for
> > + * private state there.  Leaving the C name alone to avoid unnecessary
> > + * pain for the existing users. */
> > +#define XEN_RING_PRIVATE pvt
> > +#else
> > +#define XEN_RING_PRIVATE private
> > +#endif
> 
> Are there likely to be many users outside of the ones using that code
> with mem_event?

Yes, lots.  It's used to implement split drivers for net, block, etc.
Most users will have taken copies of this header into their own trees,
though, and so won't face build breakage, and this isn't an ABI change.

So far, I've seen David and Andrew in favour of just changing the
field's name and letting out-of-tree users update their copies when/if
they want to.  Jan would prefer to avoid changing the field name for C
users.  I'm not delighted with any of these options but I think this
ifdeffery is worse than the others. :)

Let's see what anyone else has to say.

Cheers,

Tim.

Re: [PATCH v2] RFC: Automatically check xen's public headers for C++ pitfalls.

[Don Slutz]

On 02/26/15 14:22, Tim Deegan wrote:
> At 19:49 +0200 on 26 Feb (1424976562), Razvan Cojocaru wrote:
>> On 02/26/2015 07:01 PM, Tim Deegan wrote:
>>> +#ifdef __cplusplus
>>> +/* 'private' is a keyword in C++, so we have to use a different name for
>>> + * private state there.  Leaving the C name alone to avoid unnecessary
>>> + * pain for the existing users. */
>>> +#define XEN_RING_PRIVATE pvt
>>> +#else
>>> +#define XEN_RING_PRIVATE private
>>> +#endif
>>
>> Are there likely to be many users outside of the ones using that code
>> with mem_event?
> 
> Yes, lots.  It's used to implement split drivers for net, block, etc.
> Most users will have taken copies of this header into their own trees,
> though, and so won't face build breakage, and this isn't an ABI change.
> 
> So far, I've seen David and Andrew in favour of just changing the
> field's name and letting out-of-tree users update their copies when/if
> they want to.  Jan would prefer to avoid changing the field name for C
> users.  I'm not delighted with any of these options but I think this
> ifdeffery is worse than the others. :)
> 
> Let's see what anyone else has to say.
> 

Since I am one of the user of C++ and Xen headers, I like this a lot.
I do not like the ifdeffery above.  I am in favour of just changing the
the field's name.

    -Don Slutz

> Cheers,
> 
> Tim.
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel
> 

Re: [PATCH v2] RFC: Automatically check xen's public headers for C++ pitfalls.

[Jan Beulich]

>>> On 26.02.15 at 18:01, <tim@xen.org> wrote:
> At 16:47 +0000 on 26 Feb (1424965651), Jan Beulich wrote:
>> >>> On 26.02.15 at 17:28, <tim@xen.org> wrote:
>> > At 16:11 +0000 on 26 Feb (1424963496), Tim Deegan wrote:
>> >> Explicitly _not_ addressing the use of 'private' in various fields,
>> >> since we'd previously decided not to fix that.
>> > 
>> > BTW, ring.h is the only instance of that, so the extra diff to clear
>> > that up too is pretty small (see below).
>> > 
>> > Not sure what people think about that though - it might be
>> > quite a PITA for downstream users of it, though they ought really to
>> > be using local copies so they can update in a controlled way.
>> 
>> linux-2.6.18-xen.hg always having consumed them (almost)
>> verbatim, I don't think we should break users not massaging
>> the headers. I.e. at least make the field name conditional upon
>> using C vs C++.
> 
> Something like this?  This is the kind of uglification that I would
> like to avoid, though (and I don't like '#define private pvt' much
> either).

Yes, and perhaps the definition part put into xen-compat.h
instead of io/ring.h (e.g. as XEN_PRIVATE, or - leaving room in
case C++ grows more keywords - a more generic XEN_CXX()).
I don't view this as all that ugly.

Jan

Re: [PATCH v2] RFC: Automatically check xen's public headers for C++ pitfalls.

[David Vrabel]

On 26/02/15 16:28, Tim Deegan wrote:
> 
> BTW, ring.h is the only instance of that, so the extra diff to clear
> that up too is pretty small (see below).
> 
> Not sure what people think about that though - it might be
> quite a PITA for downstream users of it, though they ought really to
> be using local copies so they can update in a controlled way.

With my linux maintainer hat on, this is fine by me.

David

Re: [PATCH v2] RFC: Automatically check xen's public headers for C++ pitfalls.

[Tim Deegan]

At 17:28 +0100 on 26 Feb (1424968122), Tim Deegan wrote:
> BTW, ring.h is the only instance of that, so the extra diff to clear
> that up too is pretty small (see below).
> 
> Not sure what people think about that though - it might be
> quite a PITA for downstream users of it, though they ought really to
> be using local copies so they can update in a controlled way.

So I've seen four responses in favour of just renaming the field
(Andrew Cooper, Razvan Cojocaru, Don Slutz and David Vrabel) and one
in favour of #ifdeffing it so it's only renamed in C++ (Jan Beulich).
I really don't like adding more #ifdefs to an already hard-to-read
file; I'd rather just rename the field, or else leaving it alone and
letting C++ users carry the fixup in their own code.

CC'ing the other "THE REST" maintainers for their opinions.

Tim.

> diff --git a/xen/include/Makefile b/xen/include/Makefile
> index d48a642..c7a1d52 100644
> --- a/xen/include/Makefile
> +++ b/xen/include/Makefile
> @@ -104,8 +104,7 @@ headers.chk: $(PUBLIC_ANSI_HEADERS) Makefile
>  headers++.chk: $(PUBLIC_HEADERS) Makefile
>  	if $(CXX) -v >/dev/null 2>&1; then \
>  	    for i in $(filter %.h,$^); do \
> -	        $(CXX) -x c++ -std=gnu++98 -Wall -Werror \
> -	               -D__XEN_TOOLS__ -Dprivate=private_is_a_keyword_in_cpp \
> +	        $(CXX) -x c++ -std=gnu++98 -Wall -Werror -D__XEN_TOOLS__ \
>  	               -include stdint.h -include public/xen.h \
>  	               -S -o /dev/null $$i || exit 1; \
>  	        echo $$i; \
> diff --git a/xen/include/public/io/ring.h b/xen/include/public/io/ring.h
> index 73e13d7..bb13494 100644
> --- a/xen/include/public/io/ring.h
> +++ b/xen/include/public/io/ring.h
> @@ -111,7 +111,7 @@ struct __name##_sring {                                                 \
>              uint8_t msg;                                                \
>          } tapif_user;                                                   \
>          uint8_t pvt_pad[4];                                             \
> -    } private;                                                          \
> +    } local;                                                            \
>      uint8_t __pad[44];                                                  \
>      union __name##_sring_entry ring[1]; /* variable-length */           \
>  };                                                                      \
> @@ -156,7 +156,7 @@ typedef struct __name##_back_ring __name##_back_ring_t
>  #define SHARED_RING_INIT(_s) do {                                       \
>      (_s)->req_prod  = (_s)->rsp_prod  = 0;                              \
>      (_s)->req_event = (_s)->rsp_event = 1;                              \
> -    (void)memset((_s)->private.pvt_pad, 0, sizeof((_s)->private.pvt_pad)); \
> +    (void)memset((_s)->local.pvt_pad, 0, sizeof((_s)->local.pvt_pad));  \
>      (void)memset((_s)->__pad, 0, sizeof((_s)->__pad));                  \
>  } while(0)
>  
> 
> 

Re: [PATCH v2] RFC: Automatically check xen's public headers for C++ pitfalls.

[Ian Campbell]

On Thu, 2015-03-05 at 12:25 +0100, Tim Deegan wrote:
> At 17:28 +0100 on 26 Feb (1424968122), Tim Deegan wrote:
> > BTW, ring.h is the only instance of that, so the extra diff to clear
> > that up too is pretty small (see below).
> > 
> > Not sure what people think about that though - it might be
> > quite a PITA for downstream users of it, though they ought really to
> > be using local copies so they can update in a controlled way.
> 
> So I've seen four responses in favour of just renaming the field
> (Andrew Cooper, Razvan Cojocaru, Don Slutz and David Vrabel) and one
> in favour of #ifdeffing it so it's only renamed in C++ (Jan Beulich).
> I really don't like adding more #ifdefs to an already hard-to-read
> file; I'd rather just rename the field, or else leaving it alone and
> letting C++ users carry the fixup in their own code.
> 
> CC'ing the other "THE REST" maintainers for their opinions.

Rather than ifdefs for C++, don't we need them based on
__XEN_INTERFACE_VERSION__?

I don't much like that (I'd rather just change the name) but I think
that's what we are supposed to do here.

Re: [PATCH v2] RFC: Automatically check xen's public headers for C++ pitfalls.

[Jan Beulich]

>>> On 05.03.15 at 12:35, <ian.campbell@citrix.com> wrote:
> On Thu, 2015-03-05 at 12:25 +0100, Tim Deegan wrote:
>> At 17:28 +0100 on 26 Feb (1424968122), Tim Deegan wrote:
>> > BTW, ring.h is the only instance of that, so the extra diff to clear
>> > that up too is pretty small (see below).
>> > 
>> > Not sure what people think about that though - it might be
>> > quite a PITA for downstream users of it, though they ought really to
>> > be using local copies so they can update in a controlled way.
>> 
>> So I've seen four responses in favour of just renaming the field
>> (Andrew Cooper, Razvan Cojocaru, Don Slutz and David Vrabel) and one
>> in favour of #ifdeffing it so it's only renamed in C++ (Jan Beulich).
>> I really don't like adding more #ifdefs to an already hard-to-read
>> file; I'd rather just rename the field, or else leaving it alone and
>> letting C++ users carry the fixup in their own code.
>> 
>> CC'ing the other "THE REST" maintainers for their opinions.
> 
> Rather than ifdefs for C++, don't we need them based on
> __XEN_INTERFACE_VERSION__?

That's not applicable to the stuff under public/io/.

Jan

Re: [PATCH v2] RFC: Automatically check xen's public headers for C++ pitfalls.

[Ian Campbell]

On Thu, 2015-03-05 at 11:41 +0000, Jan Beulich wrote:
> >>> On 05.03.15 at 12:35, <ian.campbell@citrix.com> wrote:
> > On Thu, 2015-03-05 at 12:25 +0100, Tim Deegan wrote:
> >> At 17:28 +0100 on 26 Feb (1424968122), Tim Deegan wrote:
> >> > BTW, ring.h is the only instance of that, so the extra diff to clear
> >> > that up too is pretty small (see below).
> >> > 
> >> > Not sure what people think about that though - it might be
> >> > quite a PITA for downstream users of it, though they ought really to
> >> > be using local copies so they can update in a controlled way.
> >> 
> >> So I've seen four responses in favour of just renaming the field
> >> (Andrew Cooper, Razvan Cojocaru, Don Slutz and David Vrabel) and one
> >> in favour of #ifdeffing it so it's only renamed in C++ (Jan Beulich).
> >> I really don't like adding more #ifdefs to an already hard-to-read
> >> file; I'd rather just rename the field, or else leaving it alone and
> >> letting C++ users carry the fixup in their own code.
> >> 
> >> CC'ing the other "THE REST" maintainers for their opinions.
> > 
> > Rather than ifdefs for C++, don't we need them based on
> > __XEN_INTERFACE_VERSION__?
> 
> That's not applicable to the stuff under public/io/.

In which case I'd certainly prefer just changing the name and getting it
over with.

mini-os would need checking, since that's (AFAIK) the only intree user
of these headers. (Probably now that it is split out it ought to do as
everything else now does and take a copy)

Ian.

Re: [PATCH v2] RFC: Automatically check xen's public headers for C++ pitfalls.

[Wei Liu]

On Thu, Mar 05, 2015 at 11:55:55AM +0000, Ian Campbell wrote:
> On Thu, 2015-03-05 at 11:41 +0000, Jan Beulich wrote:
> > >>> On 05.03.15 at 12:35, <ian.campbell@citrix.com> wrote:
> > > On Thu, 2015-03-05 at 12:25 +0100, Tim Deegan wrote:
> > >> At 17:28 +0100 on 26 Feb (1424968122), Tim Deegan wrote:
> > >> > BTW, ring.h is the only instance of that, so the extra diff to clear
> > >> > that up too is pretty small (see below).
> > >> > 
> > >> > Not sure what people think about that though - it might be
> > >> > quite a PITA for downstream users of it, though they ought really to
> > >> > be using local copies so they can update in a controlled way.
> > >> 
> > >> So I've seen four responses in favour of just renaming the field
> > >> (Andrew Cooper, Razvan Cojocaru, Don Slutz and David Vrabel) and one
> > >> in favour of #ifdeffing it so it's only renamed in C++ (Jan Beulich).
> > >> I really don't like adding more #ifdefs to an already hard-to-read
> > >> file; I'd rather just rename the field, or else leaving it alone and
> > >> letting C++ users carry the fixup in their own code.
> > >> 
> > >> CC'ing the other "THE REST" maintainers for their opinions.
> > > 
> > > Rather than ifdefs for C++, don't we need them based on
> > > __XEN_INTERFACE_VERSION__?
> > 
> > That's not applicable to the stuff under public/io/.
> 
> In which case I'd certainly prefer just changing the name and getting it
> over with.
> 
> mini-os would need checking, since that's (AFAIK) the only intree user
> of these headers. (Probably now that it is split out it ought to do as
> everything else now does and take a copy)
> 

Yes, mini-os now is just like any other Xen guests, which carries a copy
of all Xen public headers. Taking a new copy is OK.

Wei.

> Ian.

Re: [PATCH v2] RFC: Automatically check xen's public headers for C++ pitfalls.

[Ian Campbell]

On Thu, 2015-03-05 at 12:13 +0000, Wei Liu wrote:
> Yes, mini-os now is just like any other Xen guests, which carries a copy
> of all Xen public headers. Taking a new copy is OK.

Ah, I forgot you'd already done this. Super!

Re: [PATCH v2] RFC: Automatically check xen's public headers for C++ pitfalls.

[Tim Deegan]

At 11:55 +0000 on 05 Mar (1425552955), Ian Campbell wrote:
> On Thu, 2015-03-05 at 11:41 +0000, Jan Beulich wrote:
> > >>> On 05.03.15 at 12:35, <ian.campbell@citrix.com> wrote:
> > > On Thu, 2015-03-05 at 12:25 +0100, Tim Deegan wrote:
> > >> So I've seen four responses in favour of just renaming the field
> > >> (Andrew Cooper, Razvan Cojocaru, Don Slutz and David Vrabel) and one
> > >> in favour of #ifdeffing it so it's only renamed in C++ (Jan Beulich).
> > >> I really don't like adding more #ifdefs to an already hard-to-read
> > >> file; I'd rather just rename the field, or else leaving it alone and
> > >> letting C++ users carry the fixup in their own code.
> > >> 
> > >> CC'ing the other "THE REST" maintainers for their opinions.
> > > 
> > > Rather than ifdefs for C++, don't we need them based on
> > > __XEN_INTERFACE_VERSION__?
> > 
> > That's not applicable to the stuff under public/io/.
> 
> In which case I'd certainly prefer just changing the name and getting it
> over with.
> 
> mini-os would need checking, since that's (AFAIK) the only intree user
> of these headers.

mini-os doesn't in fact use this field; it's a blktap2-ism.
Here's a (non-RFC) patch to rename it and update blktap2:

>From a97715b97a02c3182914cee90b363d2939c5d416 Mon Sep 17 00:00:00 2001
From: Tim Deegan <tim@xen.org>
Date: Thu, 5 Mar 2015 12:11:25 +0000
Subject: [PATCH] xen: don't use C++ keyword 'private' in public headers.

The 'private' field in io/ring.h (for blktap2, see 1b9cecdb)
is the last thing in the Xen public headers that a C++ compiler
will object to.

Rename it to pvt, update the only in-tree user, and remove the
workaround in the C++ compatibility check.

Signed-off-by: Tim Deegan <tim@xen.org>
---
 tools/blktap2/drivers/tapdisk-vbd.c | 2 +-
 xen/include/Makefile                | 3 +--
 xen/include/public/io/ring.h        | 4 ++--
 3 files changed, 4 insertions(+), 5 deletions(-)

diff --git a/tools/blktap2/drivers/tapdisk-vbd.c b/tools/blktap2/drivers/tapdisk-vbd.c
index c665f27..6d1d94a 100644
--- a/tools/blktap2/drivers/tapdisk-vbd.c
+++ b/tools/blktap2/drivers/tapdisk-vbd.c
@@ -1684,7 +1684,7 @@ tapdisk_vbd_check_ring_message(td_vbd_t *vbd)
 	if (!vbd->ring.sring)
 		return -EINVAL;
 
-	switch (vbd->ring.sring->private.tapif_user.msg) {
+	switch (vbd->ring.sring->pvt.tapif_user.msg) {
 	case 0:
 		return 0;
 
diff --git a/xen/include/Makefile b/xen/include/Makefile
index d48a642..c7a1d52 100644
--- a/xen/include/Makefile
+++ b/xen/include/Makefile
@@ -104,8 +104,7 @@ headers.chk: $(PUBLIC_ANSI_HEADERS) Makefile
 headers++.chk: $(PUBLIC_HEADERS) Makefile
 	if $(CXX) -v >/dev/null 2>&1; then \
 	    for i in $(filter %.h,$^); do \
-	        $(CXX) -x c++ -std=gnu++98 -Wall -Werror \
-	               -D__XEN_TOOLS__ -Dprivate=private_is_a_keyword_in_cpp \
+	        $(CXX) -x c++ -std=gnu++98 -Wall -Werror -D__XEN_TOOLS__ \
 	               -include stdint.h -include public/xen.h \
 	               -S -o /dev/null $$i || exit 1; \
 	        echo $$i; \
diff --git a/xen/include/public/io/ring.h b/xen/include/public/io/ring.h
index 73e13d7..ba9401b 100644
--- a/xen/include/public/io/ring.h
+++ b/xen/include/public/io/ring.h
@@ -111,7 +111,7 @@ struct __name##_sring {                                                 \
             uint8_t msg;                                                \
         } tapif_user;                                                   \
         uint8_t pvt_pad[4];                                             \
-    } private;                                                          \
+    } pvt;                                                              \
     uint8_t __pad[44];                                                  \
     union __name##_sring_entry ring[1]; /* variable-length */           \
 };                                                                      \
@@ -156,7 +156,7 @@ typedef struct __name##_back_ring __name##_back_ring_t
 #define SHARED_RING_INIT(_s) do {                                       \
     (_s)->req_prod  = (_s)->rsp_prod  = 0;                              \
     (_s)->req_event = (_s)->rsp_event = 1;                              \
-    (void)memset((_s)->private.pvt_pad, 0, sizeof((_s)->private.pvt_pad)); \
+    (void)memset((_s)->pvt.pvt_pad, 0, sizeof((_s)->pvt.pvt_pad));      \
     (void)memset((_s)->__pad, 0, sizeof((_s)->__pad));                  \
 } while(0)
 
-- 
2.1.4

Re: [PATCH v2] RFC: Automatically check xen's public headers for C++ pitfalls.

[Tim Deegan]

At 13:16 +0100 on 05 Mar (1425557777), Tim Deegan wrote:
> At 11:55 +0000 on 05 Mar (1425552955), Ian Campbell wrote:
> > On Thu, 2015-03-05 at 11:41 +0000, Jan Beulich wrote:
> > > >>> On 05.03.15 at 12:35, <ian.campbell@citrix.com> wrote:
> > > > On Thu, 2015-03-05 at 12:25 +0100, Tim Deegan wrote:
> > > >> So I've seen four responses in favour of just renaming the field
> > > >> (Andrew Cooper, Razvan Cojocaru, Don Slutz and David Vrabel) and one
> > > >> in favour of #ifdeffing it so it's only renamed in C++ (Jan Beulich).
> > > >> I really don't like adding more #ifdefs to an already hard-to-read
> > > >> file; I'd rather just rename the field, or else leaving it alone and
> > > >> letting C++ users carry the fixup in their own code.
> > > >> 
> > > >> CC'ing the other "THE REST" maintainers for their opinions.
> > > > 
> > > > Rather than ifdefs for C++, don't we need them based on
> > > > __XEN_INTERFACE_VERSION__?
> > > 
> > > That's not applicable to the stuff under public/io/.
> > 
> > In which case I'd certainly prefer just changing the name and getting it
> > over with.
> > 
> > mini-os would need checking, since that's (AFAIK) the only intree user
> > of these headers.
> 
> mini-os doesn't in fact use this field; it's a blktap2-ism.
> Here's a (non-RFC) patch to rename it and update blktap2:

Ian, Jan, can I get an Ack or Nack on this so I can clear it off my
plate? :)

Tim.

> From a97715b97a02c3182914cee90b363d2939c5d416 Mon Sep 17 00:00:00 2001
> From: Tim Deegan <tim@xen.org>
> Date: Thu, 5 Mar 2015 12:11:25 +0000
> Subject: [PATCH] xen: don't use C++ keyword 'private' in public headers.
> 
> The 'private' field in io/ring.h (for blktap2, see 1b9cecdb)
> is the last thing in the Xen public headers that a C++ compiler
> will object to.
> 
> Rename it to pvt, update the only in-tree user, and remove the
> workaround in the C++ compatibility check.
> 
> Signed-off-by: Tim Deegan <tim@xen.org>
> ---
>  tools/blktap2/drivers/tapdisk-vbd.c | 2 +-
>  xen/include/Makefile                | 3 +--
>  xen/include/public/io/ring.h        | 4 ++--
>  3 files changed, 4 insertions(+), 5 deletions(-)
> 
> diff --git a/tools/blktap2/drivers/tapdisk-vbd.c b/tools/blktap2/drivers/tapdisk-vbd.c
> index c665f27..6d1d94a 100644
> --- a/tools/blktap2/drivers/tapdisk-vbd.c
> +++ b/tools/blktap2/drivers/tapdisk-vbd.c
> @@ -1684,7 +1684,7 @@ tapdisk_vbd_check_ring_message(td_vbd_t *vbd)
>  	if (!vbd->ring.sring)
>  		return -EINVAL;
>  
> -	switch (vbd->ring.sring->private.tapif_user.msg) {
> +	switch (vbd->ring.sring->pvt.tapif_user.msg) {
>  	case 0:
>  		return 0;
>  
> diff --git a/xen/include/Makefile b/xen/include/Makefile
> index d48a642..c7a1d52 100644
> --- a/xen/include/Makefile
> +++ b/xen/include/Makefile
> @@ -104,8 +104,7 @@ headers.chk: $(PUBLIC_ANSI_HEADERS) Makefile
>  headers++.chk: $(PUBLIC_HEADERS) Makefile
>  	if $(CXX) -v >/dev/null 2>&1; then \
>  	    for i in $(filter %.h,$^); do \
> -	        $(CXX) -x c++ -std=gnu++98 -Wall -Werror \
> -	               -D__XEN_TOOLS__ -Dprivate=private_is_a_keyword_in_cpp \
> +	        $(CXX) -x c++ -std=gnu++98 -Wall -Werror -D__XEN_TOOLS__ \
>  	               -include stdint.h -include public/xen.h \
>  	               -S -o /dev/null $$i || exit 1; \
>  	        echo $$i; \
> diff --git a/xen/include/public/io/ring.h b/xen/include/public/io/ring.h
> index 73e13d7..ba9401b 100644
> --- a/xen/include/public/io/ring.h
> +++ b/xen/include/public/io/ring.h
> @@ -111,7 +111,7 @@ struct __name##_sring {                                                 \
>              uint8_t msg;                                                \
>          } tapif_user;                                                   \
>          uint8_t pvt_pad[4];                                             \
> -    } private;                                                          \
> +    } pvt;                                                              \
>      uint8_t __pad[44];                                                  \
>      union __name##_sring_entry ring[1]; /* variable-length */           \
>  };                                                                      \
> @@ -156,7 +156,7 @@ typedef struct __name##_back_ring __name##_back_ring_t
>  #define SHARED_RING_INIT(_s) do {                                       \
>      (_s)->req_prod  = (_s)->rsp_prod  = 0;                              \
>      (_s)->req_event = (_s)->rsp_event = 1;                              \
> -    (void)memset((_s)->private.pvt_pad, 0, sizeof((_s)->private.pvt_pad)); \
> +    (void)memset((_s)->pvt.pvt_pad, 0, sizeof((_s)->pvt.pvt_pad));      \
>      (void)memset((_s)->__pad, 0, sizeof((_s)->__pad));                  \
>  } while(0)
>  
> -- 
> 2.1.4
> 

Re: [PATCH v2] RFC: Automatically check xen's public headers for C++ pitfalls.

[Ian Campbell]

On Thu, 2015-03-12 at 11:03 +0100, Tim Deegan wrote:
> At 13:16 +0100 on 05 Mar (1425557777), Tim Deegan wrote:
> > At 11:55 +0000 on 05 Mar (1425552955), Ian Campbell wrote:
> > > On Thu, 2015-03-05 at 11:41 +0000, Jan Beulich wrote:
> > > > >>> On 05.03.15 at 12:35, <ian.campbell@citrix.com> wrote:
> > > > > On Thu, 2015-03-05 at 12:25 +0100, Tim Deegan wrote:
> > > > >> So I've seen four responses in favour of just renaming the field
> > > > >> (Andrew Cooper, Razvan Cojocaru, Don Slutz and David Vrabel) and one
> > > > >> in favour of #ifdeffing it so it's only renamed in C++ (Jan Beulich).
> > > > >> I really don't like adding more #ifdefs to an already hard-to-read
> > > > >> file; I'd rather just rename the field, or else leaving it alone and
> > > > >> letting C++ users carry the fixup in their own code.
> > > > >> 
> > > > >> CC'ing the other "THE REST" maintainers for their opinions.
> > > > > 
> > > > > Rather than ifdefs for C++, don't we need them based on
> > > > > __XEN_INTERFACE_VERSION__?
> > > > 
> > > > That's not applicable to the stuff under public/io/.
> > > 
> > > In which case I'd certainly prefer just changing the name and getting it
> > > over with.
> > > 
> > > mini-os would need checking, since that's (AFAIK) the only intree user
> > > of these headers.
> > 
> > mini-os doesn't in fact use this field; it's a blktap2-ism.
> > Here's a (non-RFC) patch to rename it and update blktap2:
> 
> Ian, Jan, can I get an Ack or Nack on this so I can clear it off my
> plate? :)

Acked-by: Ian Campbell <ian.campbell@citrix.com>

Re: [PATCH v2] RFC: Automatically check xen's public headers for C++ pitfalls.

[Tim Deegan]

At 10:14 +0000 on 12 Mar (1426151689), Ian Campbell wrote:
> On Thu, 2015-03-12 at 11:03 +0100, Tim Deegan wrote:
> > At 13:16 +0100 on 05 Mar (1425557777), Tim Deegan wrote:
> > > At 11:55 +0000 on 05 Mar (1425552955), Ian Campbell wrote:
> > > > On Thu, 2015-03-05 at 11:41 +0000, Jan Beulich wrote:
> > > > > >>> On 05.03.15 at 12:35, <ian.campbell@citrix.com> wrote:
> > > > > > On Thu, 2015-03-05 at 12:25 +0100, Tim Deegan wrote:
> > > > > >> So I've seen four responses in favour of just renaming the field
> > > > > >> (Andrew Cooper, Razvan Cojocaru, Don Slutz and David Vrabel) and one
> > > > > >> in favour of #ifdeffing it so it's only renamed in C++ (Jan Beulich).
> > > > > >> I really don't like adding more #ifdefs to an already hard-to-read
> > > > > >> file; I'd rather just rename the field, or else leaving it alone and
> > > > > >> letting C++ users carry the fixup in their own code.
> > > > > >> 
> > > > > >> CC'ing the other "THE REST" maintainers for their opinions.
> > > > > > 
> > > > > > Rather than ifdefs for C++, don't we need them based on
> > > > > > __XEN_INTERFACE_VERSION__?
> > > > > 
> > > > > That's not applicable to the stuff under public/io/.
> > > > 
> > > > In which case I'd certainly prefer just changing the name and getting it
> > > > over with.
> > > > 
> > > > mini-os would need checking, since that's (AFAIK) the only intree user
> > > > of these headers.
> > > 
> > > mini-os doesn't in fact use this field; it's a blktap2-ism.
> > > Here's a (non-RFC) patch to rename it and update blktap2:
> > 
> > Ian, Jan, can I get an Ack or Nack on this so I can clear it off my
> > plate? :)
> 
> Acked-by: Ian Campbell <ian.campbell@citrix.com>

And pushed, thanks.

Tim.

Re: [PATCH v2] RFC: Automatically check xen's public headers for C++ pitfalls.

[Jan Beulich]

>>> On 12.03.15 at 11:03, <tim@xen.org> wrote:
> At 13:16 +0100 on 05 Mar (1425557777), Tim Deegan wrote:
>> At 11:55 +0000 on 05 Mar (1425552955), Ian Campbell wrote:
>> > On Thu, 2015-03-05 at 11:41 +0000, Jan Beulich wrote:
>> > > >>> On 05.03.15 at 12:35, <ian.campbell@citrix.com> wrote:
>> > > > On Thu, 2015-03-05 at 12:25 +0100, Tim Deegan wrote:
>> > > >> So I've seen four responses in favour of just renaming the field
>> > > >> (Andrew Cooper, Razvan Cojocaru, Don Slutz and David Vrabel) and one
>> > > >> in favour of #ifdeffing it so it's only renamed in C++ (Jan Beulich).
>> > > >> I really don't like adding more #ifdefs to an already hard-to-read
>> > > >> file; I'd rather just rename the field, or else leaving it alone and
>> > > >> letting C++ users carry the fixup in their own code.
>> > > >> 
>> > > >> CC'ing the other "THE REST" maintainers for their opinions.
>> > > > 
>> > > > Rather than ifdefs for C++, don't we need them based on
>> > > > __XEN_INTERFACE_VERSION__?
>> > > 
>> > > That's not applicable to the stuff under public/io/.
>> > 
>> > In which case I'd certainly prefer just changing the name and getting it
>> > over with.
>> > 
>> > mini-os would need checking, since that's (AFAIK) the only intree user
>> > of these headers.
>> 
>> mini-os doesn't in fact use this field; it's a blktap2-ism.
>> Here's a (non-RFC) patch to rename it and update blktap2:
> 
> Ian, Jan, can I get an Ack or Nack on this so I can clear it off my
> plate? :)

In fact I'm not enough against the change to NAK it, but I'm also not
really in favor of it, so wouldn't want to ack it. I.e. - Ian?

Jan

[PATCH v3] RFC: Automatically check xen's public headers for C++ pitfalls.

[Tim Deegan]

Add a check, like the existing check for non-ANSI C in the public
headers, that runs the public headers through a C++ compiler to
flag non-C++-friendly constructs.

Unlike the ANSI C check, we accept GCC-isms (gnu++98), and we also
check various tools-only headers.

Explicitly _not_ addressing the use of 'private' in various fields,
since we'd previously decided not to fix that.

Also tidy up the runes for these checks to be a bit more readable.

Reported-by: Razvan Cojocaru <rcojocaru@bitdefender.com>
Signed-off-by: Tim Deegan <tim@xen.org>
Cc: Jan Beulich <JBeulich@suse.com>

---

v3: rebase on staging.

v2: test more headers;
    define __XEN_TOOLS__;
    use g++98 rather than ansi;
    tidy the makefile for readability;
    add a missing include to flask_op.h, which uses evtchn_port_t.
---
 .gitignore                        |  1 +
 config/StdGNU.mk                  |  2 ++
 config/SunOS.mk                   |  1 +
 xen/include/Makefile              | 28 ++++++++++++++++++++++++----
 xen/include/public/xsm/flask_op.h |  2 ++
 5 files changed, 30 insertions(+), 4 deletions(-)

diff --git a/.gitignore b/.gitignore
index 13ee05b..78958ea 100644
--- a/.gitignore
+++ b/.gitignore
@@ -233,6 +233,7 @@ xen/arch/*/efi/compat.c
 xen/arch/*/efi/efi.h
 xen/arch/*/efi/runtime.c
 xen/include/headers.chk
+xen/include/headers++.chk
 xen/include/asm
 xen/include/asm-*/asm-offsets.h
 xen/include/compat/*
diff --git a/config/StdGNU.mk b/config/StdGNU.mk
index 4efebe3..e10ed39 100644
--- a/config/StdGNU.mk
+++ b/config/StdGNU.mk
@@ -2,9 +2,11 @@ AS         = $(CROSS_COMPILE)as
 LD         = $(CROSS_COMPILE)ld
 ifeq ($(clang),y)
 CC         = $(CROSS_COMPILE)clang
+CXX        = $(CROSS_COMPILE)clang++
 LD_LTO     = $(CROSS_COMPILE)llvm-ld
 else
 CC         = $(CROSS_COMPILE)gcc
+CXX        = $(CROSS_COMPILE)g++
 LD_LTO     = $(CROSS_COMPILE)ld
 endif
 CPP        = $(CC) -E
diff --git a/config/SunOS.mk b/config/SunOS.mk
index 3316280..c2be37d 100644
--- a/config/SunOS.mk
+++ b/config/SunOS.mk
@@ -2,6 +2,7 @@ AS         = $(CROSS_COMPILE)gas
 LD         = $(CROSS_COMPILE)gld
 CC         = $(CROSS_COMPILE)gcc
 CPP        = $(CROSS_COMPILE)gcc -E
+CXX        = $(CROSS_COMPILE)g++
 AR         = $(CROSS_COMPILE)gar
 RANLIB     = $(CROSS_COMPILE)granlib
 NM         = $(CROSS_COMPILE)gnm
diff --git a/xen/include/Makefile b/xen/include/Makefile
index 94112d1..d48a642 100644
--- a/xen/include/Makefile
+++ b/xen/include/Makefile
@@ -87,13 +87,33 @@ compat/xlat.h: $(addprefix compat/.xlat/,$(xlat-y)) Makefile
 
 ifeq ($(XEN_TARGET_ARCH),$(XEN_COMPILE_ARCH))
 
-all: headers.chk
+all: headers.chk headers++.chk
 
-headers.chk: $(filter-out public/arch-% public/%ctl.h public/xsm/% public/%hvm/save.h, $(wildcard public/*.h public/*/*.h) $(public-y)) Makefile
-	for i in $(filter %.h,$^); do $(CC) -ansi -include stdint.h -Wall -W -Werror -S -o /dev/null -x c $$i || exit 1; echo $$i; done >$@.new
+PUBLIC_HEADERS := $(filter-out public/arch-% public/dom0_ops.h, $(wildcard public/*.h public/*/*.h) $(public-y))
+
+PUBLIC_ANSI_HEADERS := $(filter-out public/%ctl.h public/xsm/% public/%hvm/save.h, $(PUBLIC_HEADERS))
+
+headers.chk: $(PUBLIC_ANSI_HEADERS) Makefile
+	for i in $(filter %.h,$^); do \
+	    $(CC) -x c -ansi -Wall -Werror -include stdint.h \
+	          -S -o /dev/null $$i || exit 1; \
+	    echo $$i; \
+	done >$@.new
+	mv $@.new $@
+
+headers++.chk: $(PUBLIC_HEADERS) Makefile
+	if $(CXX) -v >/dev/null 2>&1; then \
+	    for i in $(filter %.h,$^); do \
+	        $(CXX) -x c++ -std=gnu++98 -Wall -Werror \
+	               -D__XEN_TOOLS__ -Dprivate=private_is_a_keyword_in_cpp \
+	               -include stdint.h -include public/xen.h \
+	               -S -o /dev/null $$i || exit 1; \
+	        echo $$i; \
+	    done ; \
+	fi >$@.new
 	mv $@.new $@
 
 endif
 
 clean::
-	rm -rf compat headers.chk
+	rm -rf compat headers.chk headers++.chk
diff --git a/xen/include/public/xsm/flask_op.h b/xen/include/public/xsm/flask_op.h
index 233de81..f874589 100644
--- a/xen/include/public/xsm/flask_op.h
+++ b/xen/include/public/xsm/flask_op.h
@@ -25,6 +25,8 @@
 #ifndef __FLASK_OP_H__
 #define __FLASK_OP_H__
 
+#include "../event_channel.h"
+
 #define XEN_FLASK_INTERFACE_VERSION 1
 
 struct xen_flask_load {
-- 
2.1.4

Re: [PATCH v3] RFC: Automatically check xen's public headers for C++ pitfalls.

[Don Slutz]

On 02/26/15 11:24, Tim Deegan wrote:
> Add a check, like the existing check for non-ANSI C in the public
> headers, that runs the public headers through a C++ compiler to
> flag non-C++-friendly constructs.
> 
> Unlike the ANSI C check, we accept GCC-isms (gnu++98), and we also
> check various tools-only headers.
> 
> Explicitly _not_ addressing the use of 'private' in various fields,
> since we'd previously decided not to fix that.

This sentence and the "-Dprivate=private_is_a_keyword_in_cpp" below
appear to be at odds.  Maybe add something like:

The check ignores the use of 'private'.

> 
> Also tidy up the runes for these checks to be a bit more readable.
> 
> Reported-by: Razvan Cojocaru <rcojocaru@bitdefender.com>
> Signed-off-by: Tim Deegan <tim@xen.org>
> Cc: Jan Beulich <JBeulich@suse.com>
> 
> ---
> 

You can add my

Tested-by: Don Slutz <dslutz@verizon.com>

   -Don Slutz

> v3: rebase on staging.
> 
> v2: test more headers;
>     define __XEN_TOOLS__;
>     use g++98 rather than ansi;
>     tidy the makefile for readability;
>     add a missing include to flask_op.h, which uses evtchn_port_t.
> ---
>  .gitignore                        |  1 +
>  config/StdGNU.mk                  |  2 ++
>  config/SunOS.mk                   |  1 +
>  xen/include/Makefile              | 28 ++++++++++++++++++++++++----
>  xen/include/public/xsm/flask_op.h |  2 ++
>  5 files changed, 30 insertions(+), 4 deletions(-)
> 
> diff --git a/.gitignore b/.gitignore
> index 13ee05b..78958ea 100644
> --- a/.gitignore
> +++ b/.gitignore
> @@ -233,6 +233,7 @@ xen/arch/*/efi/compat.c
>  xen/arch/*/efi/efi.h
>  xen/arch/*/efi/runtime.c
>  xen/include/headers.chk
> +xen/include/headers++.chk
>  xen/include/asm
>  xen/include/asm-*/asm-offsets.h
>  xen/include/compat/*
> diff --git a/config/StdGNU.mk b/config/StdGNU.mk
> index 4efebe3..e10ed39 100644
> --- a/config/StdGNU.mk
> +++ b/config/StdGNU.mk
> @@ -2,9 +2,11 @@ AS         = $(CROSS_COMPILE)as
>  LD         = $(CROSS_COMPILE)ld
>  ifeq ($(clang),y)
>  CC         = $(CROSS_COMPILE)clang
> +CXX        = $(CROSS_COMPILE)clang++
>  LD_LTO     = $(CROSS_COMPILE)llvm-ld
>  else
>  CC         = $(CROSS_COMPILE)gcc
> +CXX        = $(CROSS_COMPILE)g++
>  LD_LTO     = $(CROSS_COMPILE)ld
>  endif
>  CPP        = $(CC) -E
> diff --git a/config/SunOS.mk b/config/SunOS.mk
> index 3316280..c2be37d 100644
> --- a/config/SunOS.mk
> +++ b/config/SunOS.mk
> @@ -2,6 +2,7 @@ AS         = $(CROSS_COMPILE)gas
>  LD         = $(CROSS_COMPILE)gld
>  CC         = $(CROSS_COMPILE)gcc
>  CPP        = $(CROSS_COMPILE)gcc -E
> +CXX        = $(CROSS_COMPILE)g++
>  AR         = $(CROSS_COMPILE)gar
>  RANLIB     = $(CROSS_COMPILE)granlib
>  NM         = $(CROSS_COMPILE)gnm
> diff --git a/xen/include/Makefile b/xen/include/Makefile
> index 94112d1..d48a642 100644
> --- a/xen/include/Makefile
> +++ b/xen/include/Makefile
> @@ -87,13 +87,33 @@ compat/xlat.h: $(addprefix compat/.xlat/,$(xlat-y)) Makefile
>  
>  ifeq ($(XEN_TARGET_ARCH),$(XEN_COMPILE_ARCH))
>  
> -all: headers.chk
> +all: headers.chk headers++.chk
>  
> -headers.chk: $(filter-out public/arch-% public/%ctl.h public/xsm/% public/%hvm/save.h, $(wildcard public/*.h public/*/*.h) $(public-y)) Makefile
> -	for i in $(filter %.h,$^); do $(CC) -ansi -include stdint.h -Wall -W -Werror -S -o /dev/null -x c $$i || exit 1; echo $$i; done >$@.new
> +PUBLIC_HEADERS := $(filter-out public/arch-% public/dom0_ops.h, $(wildcard public/*.h public/*/*.h) $(public-y))
> +
> +PUBLIC_ANSI_HEADERS := $(filter-out public/%ctl.h public/xsm/% public/%hvm/save.h, $(PUBLIC_HEADERS))
> +
> +headers.chk: $(PUBLIC_ANSI_HEADERS) Makefile
> +	for i in $(filter %.h,$^); do \
> +	    $(CC) -x c -ansi -Wall -Werror -include stdint.h \
> +	          -S -o /dev/null $$i || exit 1; \
> +	    echo $$i; \
> +	done >$@.new
> +	mv $@.new $@
> +
> +headers++.chk: $(PUBLIC_HEADERS) Makefile
> +	if $(CXX) -v >/dev/null 2>&1; then \
> +	    for i in $(filter %.h,$^); do \
> +	        $(CXX) -x c++ -std=gnu++98 -Wall -Werror \
> +	               -D__XEN_TOOLS__ -Dprivate=private_is_a_keyword_in_cpp \
> +	               -include stdint.h -include public/xen.h \
> +	               -S -o /dev/null $$i || exit 1; \
> +	        echo $$i; \
> +	    done ; \
> +	fi >$@.new
>  	mv $@.new $@
>  
>  endif
>  
>  clean::
> -	rm -rf compat headers.chk
> +	rm -rf compat headers.chk headers++.chk
> diff --git a/xen/include/public/xsm/flask_op.h b/xen/include/public/xsm/flask_op.h
> index 233de81..f874589 100644
> --- a/xen/include/public/xsm/flask_op.h
> +++ b/xen/include/public/xsm/flask_op.h
> @@ -25,6 +25,8 @@
>  #ifndef __FLASK_OP_H__
>  #define __FLASK_OP_H__
>  
> +#include "../event_channel.h"
> +
>  #define XEN_FLASK_INTERFACE_VERSION 1
>  
>  struct xen_flask_load {
> 

Re: [PATCH v3] RFC: Automatically check xen's public headers for C++ pitfalls.

[Tim Deegan]

At 15:28 -0500 on 26 Feb (1424960919), Don Slutz wrote:
> On 02/26/15 11:24, Tim Deegan wrote:
> > Explicitly _not_ addressing the use of 'private' in various fields,
> > since we'd previously decided not to fix that.
> 
> This sentence and the "-Dprivate=private_is_a_keyword_in_cpp" below
> appear to be at odds.

Yes, that's not very clear; will reword as I apply.

> You can add my
> 
> Tested-by: Don Slutz <dslutz@verizon.com>

Thanks.

Tim.

Re: [PATCH v3] RFC: Automatically check xen's public headers for C++ pitfalls.

[Jan Beulich]

>>> On 26.02.15 at 17:24, <tim@xen.org> wrote:
> +PUBLIC_ANSI_HEADERS := $(filter-out public/%ctl.h public/xsm/% public/%hvm/save.h, $(PUBLIC_HEADERS))
> +
> +headers.chk: $(PUBLIC_ANSI_HEADERS) Makefile
> +	for i in $(filter %.h,$^); do \
> +	    $(CC) -x c -ansi -Wall -Werror -include stdint.h \
> +	          -S -o /dev/null $$i || exit 1; \
> +	    echo $$i; \
> +	done >$@.new
> +	mv $@.new $@
> +
> +headers++.chk: $(PUBLIC_HEADERS) Makefile
> +	if $(CXX) -v >/dev/null 2>&1; then \
> +	    for i in $(filter %.h,$^); do \
> +	        $(CXX) -x c++ -std=gnu++98 -Wall -Werror \
> +	               -D__XEN_TOOLS__ -Dprivate=private_is_a_keyword_in_cpp \

With -D__XEN_TOOLS__ added, did you check that domctl.h and
sysctl.h still actually need to be excluded from this test?

Jan

Re: [PATCH v3] RFC: Automatically check xen's public headers for C++ pitfalls.

[Tim Deegan]

At 08:36 +0000 on 27 Feb (1425022578), Jan Beulich wrote:
> >>> On 26.02.15 at 17:24, <tim@xen.org> wrote:
> > +PUBLIC_ANSI_HEADERS := $(filter-out public/%ctl.h public/xsm/% public/%hvm/save.h, $(PUBLIC_HEADERS))
> > +
> > +headers.chk: $(PUBLIC_ANSI_HEADERS) Makefile
> > +	for i in $(filter %.h,$^); do \
> > +	    $(CC) -x c -ansi -Wall -Werror -include stdint.h \
> > +	          -S -o /dev/null $$i || exit 1; \
> > +	    echo $$i; \
> > +	done >$@.new
> > +	mv $@.new $@
> > +
> > +headers++.chk: $(PUBLIC_HEADERS) Makefile
> > +	if $(CXX) -v >/dev/null 2>&1; then \
> > +	    for i in $(filter %.h,$^); do \
> > +	        $(CXX) -x c++ -std=gnu++98 -Wall -Werror \
> > +	               -D__XEN_TOOLS__ -Dprivate=private_is_a_keyword_in_cpp \
> 
> With -D__XEN_TOOLS__ added, did you check that domctl.h and
> sysctl.h still actually need to be excluded from this test?

The C++ check includes those headers and defines __XEN_TOOLS__; the
ANSI C check does neither (as before).  Would you like to change that too?

Tim.

Re: [PATCH v3] RFC: Automatically check xen's public headers for C++ pitfalls.

[Jan Beulich]

>>> On 27.02.15 at 10:22, <tim@xen.org> wrote:
> At 08:36 +0000 on 27 Feb (1425022578), Jan Beulich wrote:
>> >>> On 26.02.15 at 17:24, <tim@xen.org> wrote:
>> > +PUBLIC_ANSI_HEADERS := $(filter-out public/%ctl.h public/xsm/% 
> public/%hvm/save.h, $(PUBLIC_HEADERS))
>> > +
>> > +headers.chk: $(PUBLIC_ANSI_HEADERS) Makefile
>> > +	for i in $(filter %.h,$^); do \
>> > +	    $(CC) -x c -ansi -Wall -Werror -include stdint.h \
>> > +	          -S -o /dev/null $$i || exit 1; \
>> > +	    echo $$i; \
>> > +	done >$@.new
>> > +	mv $@.new $@
>> > +
>> > +headers++.chk: $(PUBLIC_HEADERS) Makefile
>> > +	if $(CXX) -v >/dev/null 2>&1; then \
>> > +	    for i in $(filter %.h,$^); do \
>> > +	        $(CXX) -x c++ -std=gnu++98 -Wall -Werror \
>> > +	               -D__XEN_TOOLS__ -Dprivate=private_is_a_keyword_in_cpp \
>> 
>> With -D__XEN_TOOLS__ added, did you check that domctl.h and
>> sysctl.h still actually need to be excluded from this test?
> 
> The C++ check includes those headers and defines __XEN_TOOLS__; the
> ANSI C check does neither (as before). 

Argh - I again didn't look closely enough; I'm sorry.

> Would you like to change that too?

No.

Ack on v3 then.

Jan