[PATCH] libxc, libxenstore: make the headers C++-friendly

[PATCH] libxc, libxenstore: make the headers C++-friendly

[Razvan Cojocaru]

Made #including xenctrl.h, xenstore.h and a few other
frequently used headers safe to use with C++: added
extern "C" statements, renamed variables named with
C++ keywords ('new', 'private'), updated dependendt
code.

Signed-off-by: Razvan Cojocaru <rzvncj@gmail.com>

diff -r 4b476378fc35 -r 93e5f6cf98d2 tools/blktap2/drivers/tapdisk-vbd.c
--- a/tools/blktap2/drivers/tapdisk-vbd.c	Mon Jan 21 17:03:10 2013 +0000
+++ b/tools/blktap2/drivers/tapdisk-vbd.c	Tue Jan 22 18:43:45 2013 +0200
@@ -1684,7 +1684,7 @@ tapdisk_vbd_check_ring_message(td_vbd_t 
 	if (!vbd->ring.sring)
 		return -EINVAL;
 
-	switch (vbd->ring.sring->private.tapif_user.msg) {
+	switch (vbd->ring.sring->rprivate.tapif_user.msg) {
 	case 0:
 		return 0;
 
diff -r 4b476378fc35 -r 93e5f6cf98d2 tools/libxc/xenctrl.h
--- a/tools/libxc/xenctrl.h	Mon Jan 21 17:03:10 2013 +0000
+++ b/tools/libxc/xenctrl.h	Tue Jan 22 18:43:45 2013 +0200
@@ -26,6 +26,10 @@
 #ifndef XENCTRL_H
 #define XENCTRL_H
 
+#ifdef __cplusplus
+extern "C" {
+#endif
+
 /* Tell the Xen public headers we are a user-space tools build. */
 #ifndef __XEN_TOOLS__
 #define __XEN_TOOLS__ 1
@@ -114,6 +118,15 @@ typedef struct xc_interface_core xc_inte
 typedef struct xc_interface_core xc_evtchn;
 typedef struct xc_interface_core xc_gnttab;
 typedef struct xc_interface_core xc_gntshr;
+
+enum xc_error_code {
+  XC_ERROR_NONE = 0,
+  XC_INTERNAL_ERROR = 1,
+  XC_INVALID_KERNEL = 2,
+  XC_INVALID_PARAM = 3,
+  XC_OUT_OF_MEMORY = 4,
+  /* new codes need to be added to xc_error_level_to_desc too */
+};
 typedef enum xc_error_code xc_error_code;
 
 
@@ -1618,16 +1631,6 @@ int xc_hvm_inject_trap(
  *  LOGGING AND ERROR REPORTING
  */
 
-
-enum xc_error_code {
-  XC_ERROR_NONE = 0,
-  XC_INTERNAL_ERROR = 1,
-  XC_INVALID_KERNEL = 2,
-  XC_INVALID_PARAM = 3,
-  XC_OUT_OF_MEMORY = 4,
-  /* new codes need to be added to xc_error_level_to_desc too */
-};
-
 #define XC_MAX_ERROR_MSG_LEN 1024
 typedef struct xc_error {
   enum xc_error_code code;
@@ -2236,4 +2239,8 @@ int xc_compression_uncompress_page(xc_in
 				   unsigned long compbuf_size,
 				   unsigned long *compbuf_pos, char *dest);
 
+#ifdef __cplusplus
+}
+#endif 
+
 #endif /* XENCTRL_H */
diff -r 4b476378fc35 -r 93e5f6cf98d2 tools/xenstore/xenstore.h
--- a/tools/xenstore/xenstore.h	Mon Jan 21 17:03:10 2013 +0000
+++ b/tools/xenstore/xenstore.h	Tue Jan 22 18:43:45 2013 +0200
@@ -20,6 +20,10 @@
 #ifndef XENSTORE_H
 #define XENSTORE_H
 
+#ifdef __cplusplus
+extern "C" {
+#endif
+
 #include <xenstore_lib.h>
 
 #define XBT_NULL 0
@@ -244,6 +248,11 @@ char *xs_debug_command(struct xs_handle 
 		       void *data, unsigned int len);
 
 int xs_suspend_evtchn_port(int domid);
+
+#ifdef __cplusplus
+}
+#endif
+
 #endif /* XENSTORE_H */
 
 /*
diff -r 4b476378fc35 -r 93e5f6cf98d2 xen/include/public/arch-x86/hvm/save.h
--- a/xen/include/public/arch-x86/hvm/save.h	Mon Jan 21 17:03:10 2013 +0000
+++ b/xen/include/public/arch-x86/hvm/save.h	Tue Jan 22 18:43:45 2013 +0200
@@ -269,15 +269,15 @@ struct hvm_hw_cpu_compat {
 };
 
 static inline int _hvm_hw_fix_cpu(void *h) {
-    struct hvm_hw_cpu *new=h;
-    struct hvm_hw_cpu_compat *old=h;
+    struct hvm_hw_cpu *newcpu=(struct hvm_hw_cpu *)h;
+    struct hvm_hw_cpu_compat *old=(struct hvm_hw_cpu_compat *)h;
 
     /* If we copy from the end backwards, we should
      * be able to do the modification in-place */
-    new->error_code=old->error_code;
-    new->pending_event=old->pending_event;
-    new->tsc=old->tsc;
-    new->msr_tsc_aux=0;
+    newcpu->error_code=old->error_code;
+    newcpu->pending_event=old->pending_event;
+    newcpu->tsc=old->tsc;
+    newcpu->msr_tsc_aux=0;
 
     return 0;
 }
diff -r 4b476378fc35 -r 93e5f6cf98d2 xen/include/public/hvm/save.h
--- a/xen/include/public/hvm/save.h	Mon Jan 21 17:03:10 2013 +0000
+++ b/xen/include/public/hvm/save.h	Tue Jan 22 18:43:45 2013 +0200
@@ -28,6 +28,10 @@
 #ifndef __XEN_PUBLIC_HVM_SAVE_H__
 #define __XEN_PUBLIC_HVM_SAVE_H__
 
+#ifdef __cplusplus
+extern "C" {
+#endif
+
 /*
  * Structures in this header *must* have the same layout in 32bit 
  * and 64bit environments: this means that all fields must be explicitly 
@@ -108,4 +112,8 @@ DECLARE_HVM_SAVE_TYPE(END, 0, struct hvm
 #error "unsupported architecture"
 #endif
 
+#ifdef __cplusplus
+}
+#endif
+
 #endif /* __XEN_PUBLIC_HVM_SAVE_H__ */
diff -r 4b476378fc35 -r 93e5f6cf98d2 xen/include/public/io/ring.h
--- a/xen/include/public/io/ring.h	Mon Jan 21 17:03:10 2013 +0000
+++ b/xen/include/public/io/ring.h	Tue Jan 22 18:43:45 2013 +0200
@@ -111,7 +111,7 @@ struct __name##_sring {                 
             uint8_t msg;                                                \
         } tapif_user;                                                   \
         uint8_t pvt_pad[4];                                             \
-    } private;                                                          \
+    } rprivate;                                                          \
     uint8_t __pad[44];                                                  \
     union __name##_sring_entry ring[1]; /* variable-length */           \
 };                                                                      \
@@ -156,7 +156,7 @@ typedef struct __name##_back_ring __name
 #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)->rprivate.pvt_pad, 0, sizeof((_s)->rprivate.pvt_pad)); \
     (void)memset((_s)->__pad, 0, sizeof((_s)->__pad));                  \
 } while(0)
 
diff -r 4b476378fc35 -r 93e5f6cf98d2 xen/include/public/mem_event.h
--- a/xen/include/public/mem_event.h	Mon Jan 21 17:03:10 2013 +0000
+++ b/xen/include/public/mem_event.h	Tue Jan 22 18:43:45 2013 +0200
@@ -27,6 +27,10 @@
 #ifndef _XEN_PUBLIC_MEM_EVENT_H
 #define _XEN_PUBLIC_MEM_EVENT_H
 
+#ifdef __cplusplus
+extern "C" {
+#endif
+
 #include "xen.h"
 #include "io/ring.h"
 
@@ -69,6 +73,10 @@ typedef struct mem_event_st {
 
 DEFINE_RING_TYPES(mem_event, mem_event_request_t, mem_event_response_t);
 
+#ifdef __cplusplus
+}
+#endif
+
 #endif
 
 /*

[PATCH] libxc, libxenstore: make the headers C++-friendly

[Jan Beulich]

>>> On 22.01.13 at 17:44, Razvan Cojocaru <rzvncj@gmail.com> wrote:
> --- a/xen/include/public/arch-x86/hvm/save.h	Mon Jan 21 17:03:10 2013 +0000
> +++ b/xen/include/public/arch-x86/hvm/save.h	Tue Jan 22 18:43:45 2013 +0200
> @@ -269,15 +269,15 @@ struct hvm_hw_cpu_compat {
>  };
>  
>  static inline int _hvm_hw_fix_cpu(void *h) {
> -    struct hvm_hw_cpu *new=h;
> -    struct hvm_hw_cpu_compat *old=h;
> +    struct hvm_hw_cpu *newcpu=(struct hvm_hw_cpu *)h;
> +    struct hvm_hw_cpu_compat *old=(struct hvm_hw_cpu_compat *)h;

That's not really C++. But yes, I recognize that the alternative
would be an even uglier #ifdef.

>  
>      /* If we copy from the end backwards, we should
>       * be able to do the modification in-place */
> -    new->error_code=old->error_code;
> -    new->pending_event=old->pending_event;
> -    new->tsc=old->tsc;
> -    new->msr_tsc_aux=0;
> +    newcpu->error_code=old->error_code;
> +    newcpu->pending_event=old->pending_event;
> +    newcpu->tsc=old->tsc;
> +    newcpu->msr_tsc_aux=0;

Here and above - if you already touch those, could you add
spaces around the = operators?

> --- a/xen/include/public/io/ring.h	Mon Jan 21 17:03:10 2013 +0000
> +++ b/xen/include/public/io/ring.h	Tue Jan 22 18:43:45 2013 +0200
> @@ -111,7 +111,7 @@ struct __name##_sring {                 
>              uint8_t msg;                                                \
>          } tapif_user;                                                   \
>          uint8_t pvt_pad[4];                                             \
> -    } private;                                                          \
> +    } rprivate;                                                          \

This is a no-go: In a public header, you can't change names like
this. Since the stuff under io/ isn't really tied to
__XEN_INTERFACE_VERSION__, I'm also not immediately seeing
how else you could adjust this.

Jan

Re: [PATCH] libxc, libxenstore: make the headers C++-friendly

[Ian Campbell]

On Tue, 2013-01-22 at 16:53 +0000, Jan Beulich wrote:
> >>> On 22.01.13 at 17:44, Razvan Cojocaru <rzvncj@gmail.com> wrote:
> > --- a/xen/include/public/arch-x86/hvm/save.h	Mon Jan 21 17:03:10 2013 +0000
> > +++ b/xen/include/public/arch-x86/hvm/save.h	Tue Jan 22 18:43:45 2013 +0200
> > @@ -269,15 +269,15 @@ struct hvm_hw_cpu_compat {
> >  };
> >  
> >  static inline int _hvm_hw_fix_cpu(void *h) {
> > -    struct hvm_hw_cpu *new=h;
> > -    struct hvm_hw_cpu_compat *old=h;
> > +    struct hvm_hw_cpu *newcpu=(struct hvm_hw_cpu *)h;
> > +    struct hvm_hw_cpu_compat *old=(struct hvm_hw_cpu_compat *)h;
> 
> That's not really C++. But yes, I recognize that the alternative
> would be an even uglier #ifdef.

Or out of lining this entire sucker?

> > --- a/xen/include/public/io/ring.h	Mon Jan 21 17:03:10 2013 +0000
> > +++ b/xen/include/public/io/ring.h	Tue Jan 22 18:43:45 2013 +0200
> > @@ -111,7 +111,7 @@ struct __name##_sring {                 
> >              uint8_t msg;                                                \
> >          } tapif_user;                                                   \
> >          uint8_t pvt_pad[4];                                             \
> > -    } private;                                                          \
> > +    } rprivate;                                                          \
> 
> This is a no-go: In a public header, you can't change names like
> this. Since the stuff under io/ isn't really tied to
> __XEN_INTERFACE_VERSION__, I'm also not immediately seeing
> how else you could adjust this.

Me neither. Perhaps everyone uses the macros and we get away with this
change?

Was this change build tested, including stubdoms and other tools which
may rely on these headers? (qemu?)

Ian.

Re: [PATCH] libxc, libxenstore: make the headers C++-friendly

[Razvan Cojocaru]

> Me neither. Perhaps everyone uses the macros and we get away with this
> change?

Not everyone. You'll notice that in the patch I had to modify 
tools/blktap2/drivers/tapdisk-vbd.c which used 'private' directly.

> Was this change build tested, including stubdoms and other tools which
> may rely on these headers? (qemu?)

I did run 'make' and everything built with no errors on my machine.

Thanks,
Razvan Cojocaru

Re: [PATCH] libxc, libxenstore: make the headers C++-friendly

[Jan Beulich]

>>> On 22.01.13 at 18:02, Ian Campbell <Ian.Campbell@citrix.com> wrote:
> On Tue, 2013-01-22 at 16:53 +0000, Jan Beulich wrote:
>> >>> On 22.01.13 at 17:44, Razvan Cojocaru <rzvncj@gmail.com> wrote:
>> > --- a/xen/include/public/io/ring.h	Mon Jan 21 17:03:10 2013 +0000
>> > +++ b/xen/include/public/io/ring.h	Tue Jan 22 18:43:45 2013 +0200
>> > @@ -111,7 +111,7 @@ struct __name##_sring {                 
>> >              uint8_t msg;                                                \
>> >          } tapif_user;                                                   \
>> >          uint8_t pvt_pad[4];                                             \
>> > -    } private;                                                          \
>> > +    } rprivate;                                                          \
>> 
>> This is a no-go: In a public header, you can't change names like
>> this. Since the stuff under io/ isn't really tied to
>> __XEN_INTERFACE_VERSION__, I'm also not immediately seeing
>> how else you could adjust this.
> 
> Me neither. Perhaps everyone uses the macros and we get away with this
> change?

No, we won't - netif has "smartpoll_active" defined inside that
sub-structure (not sure where that's being used though), and
blktap2 has a "msg" field there.

> Was this change build tested, including stubdoms and other tools which
> may rely on these headers? (qemu?)

Obviously not, at least not tools/blktap2/drivers/tapdisk-vbd.c
and (with a suitably synchronized header) linux-2.6.18-xen.hg.

Jan

Re: [PATCH] libxc, libxenstore: make the headers C++-friendly

[Razvan Cojocaru]

>> -    struct hvm_hw_cpu *new=h;
>> -    struct hvm_hw_cpu_compat *old=h;
>> +    struct hvm_hw_cpu *newcpu=(struct hvm_hw_cpu *)h;
>> +    struct hvm_hw_cpu_compat *old=(struct hvm_hw_cpu_compat *)h;
>
> That's not really C++. But yes, I recognize that the alternative
> would be an even uglier #ifdef.

Is this about reinterpret_cast<type>(h)? I'm not trying to turn a C 
header into a C++ header, I'm trying to make a C header usable with C++, 
obviously.

>> +    newcpu->error_code=old->error_code;
>> +    newcpu->pending_event=old->pending_event;
>> +    newcpu->tsc=old->tsc;
>> +    newcpu->msr_tsc_aux=0;
>
> Here and above - if you already touch those, could you add
> spaces around the = operators?

Of course.

>> -    } private;                                                          \
>> +    } rprivate;                                                          \
>
> This is a no-go: In a public header, you can't change names like
> this. Since the stuff under io/ isn't really tied to
> __XEN_INTERFACE_VERSION__, I'm also not immediately seeing
> how else you could adjust this.

That ammounts to not being able to use libxc with C++ if you ever use 
ring.h's DEFINE_RING_TYPES() macro (that is, if you ever use mem_events).

Cheers,
Razvan Cojocaru

Re: [PATCH] libxc, libxenstore: make the headers C++-friendly

[Jan Beulich]

>>> On 22.01.13 at 18:07, Razvan Cojocaru <rzvncj@gmail.com> wrote:
>> > -    struct hvm_hw_cpu *new=h;
>>> -    struct hvm_hw_cpu_compat *old=h;
>>> +    struct hvm_hw_cpu *newcpu=(struct hvm_hw_cpu *)h;
>>> +    struct hvm_hw_cpu_compat *old=(struct hvm_hw_cpu_compat *)h;
>>
>> That's not really C++. But yes, I recognize that the alternative
>> would be an even uglier #ifdef.
> 
> Is this about reinterpret_cast<type>(h)? I'm not trying to turn a C 
> header into a C++ header, I'm trying to make a C header usable with C++, 
> obviously.

I understand that. Yet I'm opposed to casts in C where they
aren't needed.

>>> -    } private;                                                          \
>>> +    } rprivate;                                                          \
>>
>> This is a no-go: In a public header, you can't change names like
>> this. Since the stuff under io/ isn't really tied to
>> __XEN_INTERFACE_VERSION__, I'm also not immediately seeing
>> how else you could adjust this.
> 
> That ammounts to not being able to use libxc with C++ if you ever use 
> ring.h's DEFINE_RING_TYPES() macro (that is, if you ever use mem_events).

Not exactly - with all of this being macros, the _consumer_ of
DEFINE_RING_TYPES() could define private to rprivate (or
whatever is being liked best), with the respective uses of the
other macros using that field suitably placed inside the scope
of that #define.

Jan

Re: [PATCH] libxc, libxenstore: make the headers C++-friendly

[Razvan Cojocaru]

> Not exactly - with all of this being macros, the _consumer_ of
> DEFINE_RING_TYPES() could define private to rprivate (or
> whatever is being liked best), with the respective uses of the
> other macros using that field suitably placed inside the scope
> of that #define.

All I have to do is #include <mem_event.h> and I get this build error 
(with g++):

/usr/include/xen/mem_event.h:71:1: error: expected ‘;’ after union
definition
/usr/include/xen/mem_event.h:71:1: error: expected ‘:’ before ‘;’ token

It goes away after renaming 'private' to something else (that's not a 
C++ keyword). Not much room to maneuver around it from C++ code.

Cheers,
Razvan Cojocaru

Re: [PATCH] libxc, libxenstore: make the headers C++-friendly

[Ian Campbell]

On Tue, 2013-01-22 at 17:21 +0000, Razvan Cojocaru wrote:
> > Not exactly - with all of this being macros, the _consumer_ of
> > DEFINE_RING_TYPES() could define private to rprivate (or
> > whatever is being liked best), with the respective uses of the
> > other macros using that field suitably placed inside the scope
> > of that #define.
> 
> All I have to do is #include <mem_event.h> and I get this build error 
> (with g++):

What about if you do
#define private pprivate
#include <mem_event.h>
?

Re: [PATCH] libxc, libxenstore: make the headers C++-friendly

[Razvan Cojocaru]

> What about if you do
> #define private pprivate
> #include <mem_event.h>
> ?

Then when I write my classes, and like a good C++ citizen, try to hide 
as much implementation detail as possible, my class becomes from this:

class XenHandle {
// ...
private:
      xc_interface *xci_;
};

this:


class XenHandle {
// ...
pprivate:
      xc_interface *xci_;
};

Cheers,
Razvan Cojocaru

Re: [PATCH] libxc, libxenstore: make the headers C++-friendly

[Andrew Cooper]

On 22/01/13 17:29, Razvan Cojocaru wrote:
>> What about if you do
>> #define private pprivate
>> #include <mem_event.h>
>> ?

The correct way is:

#define private pprivate
#include <foo>
#undef private

...

Given these complications, would it perhaps be better to define some
specific "C++ headers" for libxc etc which correctly wrap the C ones ? 
I dont think it is unreasonable to say "C++ consumers should include
<foo.hpp> instead of foo.h"

~Andrew

> Then when I write my classes, and like a good C++ citizen, try to hide 
> as much implementation detail as possible, my class becomes from this:
>
> class XenHandle {
> // ...
> private:
>       xc_interface *xci_;
> };
>
> this:
>
>
> class XenHandle {
> // ...
> pprivate:
>       xc_interface *xci_;
> };
>
> Cheers,
> Razvan Cojocaru
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel

Re: [PATCH] libxc, libxenstore: make the headers C++-friendly

[Jan Beulich]

>>> On 22.01.13 at 18:21, Razvan Cojocaru <rzvncj@gmail.com> wrote:
>>  Not exactly - with all of this being macros, the _consumer_ of
>> DEFINE_RING_TYPES() could define private to rprivate (or
>> whatever is being liked best), with the respective uses of the
>> other macros using that field suitably placed inside the scope
>> of that #define.
> 
> All I have to do is #include <mem_event.h> and I get this build error 
> (with g++):
> 
> /usr/include/xen/mem_event.h:71:1: error: expected ‘;’ after union
> definition
> /usr/include/xen/mem_event.h:71:1: error: expected ‘:’ before ‘;’ token
> 
> It goes away after renaming 'private' to something else (that's not a 
> C++ keyword). Not much room to maneuver around it from C++ code.

As said - a #define around the consumer of DEFINE_RING_TYPES()
would help; in the given case that's your inclusion of mem_event.h.

Jan

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [PATCH] libxc, libxenstore: make the headers C++-friendly

[Razvan Cojocaru]

> Not exactly - with all of this being macros, the _consumer_ of
> DEFINE_RING_TYPES() could define private to rprivate (or
> whatever is being liked best), with the respective uses of the
> other macros using that field suitably placed inside the scope
> of that #define.

More to the point, #defining private to anything in C++ code is asking 
for (big) trouble.

Cheers,
Razvan Cojocaru

Re: [PATCH] libxc, libxenstore: make the headers C++-friendly

[Tim Deegan]

At 19:26 +0200 on 22 Jan (1358882774), Razvan Cojocaru wrote:
> >Not exactly - with all of this being macros, the _consumer_ of
> >DEFINE_RING_TYPES() could define private to rprivate (or
> >whatever is being liked best), with the respective uses of the
> >other macros using that field suitably placed inside the scope
> >of that #define.
> 
> More to the point, #defining private to anything in C++ code is asking 
> for (big) trouble.

Well, obviously you shouldn't leave it lying around.  Can you do
something like

extern "C" {
#define private mumblywurzle
#include <xen/ring.h>
#undef private
}

Once again, this isn't C++.  If there has to be an ugly workaround, put
it in the C++ code.

Re: [PATCH] libxc, libxenstore: make the headers C++-friendly

[Razvan Cojocaru]

> Well, obviously you shouldn't leave it lying around.  Can you do
> something like
>
> extern "C" {
> #define private mumblywurzle
> #include <xen/ring.h>
> #undef private
> }

There are other headers that indirectly include ring.h. My code never 
includes it directly, and, as you have seen, the errors were far from 
specific. Trying to figure out who brought what header in and what the 
error means, and fighting to be able to use C++ properly on top of that 
is surely not something any of us prefers.

> Once again, this isn't C++.  If there has to be an ugly workaround, put
> it in the C++ code.

There should be no ugly workaround. I'll probably leave all the headers 
alone, implement my own C wrapper that exposes extern "C" functions and 
hides all the macro stuff, and build my C++ application on top of that 
intermediate layer.

I just asked because I thought there might be some interest in being 
able to use the Xen code directly from C++ projects; if there isn't, 
that's fair enough.

Thank you for all your comments!

Cheers,
Razvan Cojocaru

Re: [PATCH] libxc, libxenstore: make the headers C++-friendly

[Jan Beulich]

>>> On 22.01.13 at 18:26, Razvan Cojocaru <rzvncj@gmail.com> wrote:
>>  Not exactly - with all of this being macros, the _consumer_ of
>> DEFINE_RING_TYPES() could define private to rprivate (or
>> whatever is being liked best), with the respective uses of the
>> other macros using that field suitably placed inside the scope
>> of that #define.
> 
> More to the point, #defining private to anything in C++ code is asking 
> for (big) trouble.

If done carelessly, sure. Which is why I said to wrap any users
of respective macros in inline functions or helper classes, and
have only the #include and these helpers in the scope where
private is re-#define-d (i.e. I thought it goes without saying that
you want to #undef private after those helpers).

Jan

Re: [PATCH] libxc, libxenstore: make the headers C++-friendly

[Tim Deegan]

At 18:44 +0200 on 22 Jan (1358880251), Razvan Cojocaru wrote:
> diff -r 4b476378fc35 -r 93e5f6cf98d2 xen/include/public/hvm/save.h
> --- a/xen/include/public/hvm/save.h	Mon Jan 21 17:03:10 2013 +0000
> +++ b/xen/include/public/hvm/save.h	Tue Jan 22 18:43:45 2013 +0200
> @@ -28,6 +28,10 @@
>  #ifndef __XEN_PUBLIC_HVM_SAVE_H__
>  #define __XEN_PUBLIC_HVM_SAVE_H__
>  
> +#ifdef __cplusplus
> +extern "C" {
> +#endif
> +

The Xen public headers are in C, not C++. Doesn't this kind of
boilerplace belong in the C++ file that's trying to include a C header?

Tim.

>  /*
>   * Structures in this header *must* have the same layout in 32bit 
>   * and 64bit environments: this means that all fields must be explicitly 
> @@ -108,4 +112,8 @@ DECLARE_HVM_SAVE_TYPE(END, 0, struct hvm
>  #error "unsupported architecture"
>  #endif
>  
> +#ifdef __cplusplus
> +}
> +#endif
> +
>  #endif /* __XEN_PUBLIC_HVM_SAVE_H__ */
> diff -r 4b476378fc35 -r 93e5f6cf98d2 xen/include/public/io/ring.h
> --- a/xen/include/public/io/ring.h	Mon Jan 21 17:03:10 2013 +0000
> +++ b/xen/include/public/io/ring.h	Tue Jan 22 18:43:45 2013 +0200
> @@ -111,7 +111,7 @@ struct __name##_sring {                 
>              uint8_t msg;                                                \
>          } tapif_user;                                                   \
>          uint8_t pvt_pad[4];                                             \
> -    } private;                                                          \
> +    } rprivate;                                                          \
>      uint8_t __pad[44];                                                  \
>      union __name##_sring_entry ring[1]; /* variable-length */           \
>  };                                                                      \
> @@ -156,7 +156,7 @@ typedef struct __name##_back_ring __name
>  #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)->rprivate.pvt_pad, 0, sizeof((_s)->rprivate.pvt_pad)); \
>      (void)memset((_s)->__pad, 0, sizeof((_s)->__pad));                  \
>  } while(0)
>  
> diff -r 4b476378fc35 -r 93e5f6cf98d2 xen/include/public/mem_event.h
> --- a/xen/include/public/mem_event.h	Mon Jan 21 17:03:10 2013 +0000
> +++ b/xen/include/public/mem_event.h	Tue Jan 22 18:43:45 2013 +0200
> @@ -27,6 +27,10 @@
>  #ifndef _XEN_PUBLIC_MEM_EVENT_H
>  #define _XEN_PUBLIC_MEM_EVENT_H
>  
> +#ifdef __cplusplus
> +extern "C" {
> +#endif
> +
>  #include "xen.h"
>  #include "io/ring.h"
>  
> @@ -69,6 +73,10 @@ typedef struct mem_event_st {
>  
>  DEFINE_RING_TYPES(mem_event, mem_event_request_t, mem_event_response_t);
>  
> +#ifdef __cplusplus
> +}
> +#endif
> +
>  #endif
>  
>  /*
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel

Re: [PATCH] libxc, libxenstore: make the headers C++-friendly

[Razvan Cojocaru]

>> +#ifdef __cplusplus
>> +extern "C" {
>> +#endif
>> +
>
> The Xen public headers are in C, not C++. Doesn't this kind of
> boilerplace belong in the C++ file that's trying to include a C header?

That's the whole point, if they were in C++ there would have been no 
need for the extern "C". It is, of course, possible to to this from your 
C++ code:

extern "C" {
#include <C_header.h>
}

However, most well-behaved C libraries do that for all their headers.
A few random examples on my Arch Linux system:

$ grep __cplusplus zlib.h
#ifdef __cplusplus
#ifdef __cplusplus

$ grep __cplusplus xvid.h
#ifdef __cplusplus
#ifdef __cplusplus

$ grep __cplusplus lzx.h
#ifdef __cplusplus
#ifdef __cplusplus

$ grep __cplusplus curses.h
#if defined(__cplusplus)        /* __cplusplus, etc. */
#endif /* !__cplusplus, etc. */
#ifdef __cplusplus
#ifdef __cplusplus

Pretty much all of them.

Cheers,
Razvan Cojocaru

Re: [PATCH] libxc, libxenstore: make the headers C++-friendly

[Tim Deegan]

At 19:12 +0200 on 22 Jan (1358881935), Razvan Cojocaru wrote:
> >>+#ifdef __cplusplus
> >>+extern "C" {
> >>+#endif
> >>+
> >
> >The Xen public headers are in C, not C++. Doesn't this kind of
> >boilerplace belong in the C++ file that's trying to include a C header?
> 
> However, most well-behaved C libraries do that for all their headers.

xen/include/public is is not a library, it's the C API (and ABI) of the
hypervisor.  By all means make libxc/libxg/libxl C++-friendly if you
like, but please leave the hypervisor interface alone.

Tim.

Re: [PATCH] libxc, libxenstore: make the headers C++-friendly

[Razvan Cojocaru]

>> However, most well-behaved C libraries do that for all their headers.
>
> xen/include/public is is not a library, it's the C API (and ABI) of the
> hypervisor.  By all means make libxc/libxg/libxl C++-friendly if you
> like, but please leave the hypervisor interface alone.

I understand your position, however I should point out that, if done 
properly, making even all the headers in Xen C++-friendly would have no 
impact whatsoever on anything in Xen.

If you compile them with a C compiler (i.e. gcc instead of g++), the 
'extern "C"' statements simply don't exist (because then __cplusplus is 
not #defined). As for the rest, all that's required is to not use C++ 
keywords as variable names, and to be careful to define something before 
typedef-ing it. That's absolutely all there is to it.

Thank you for your time and comments,
Razvan Cojocaru