[PATCH] xen: Minor modifications to platform.h to make it C++-friendly

[PATCH] xen: Minor modifications to platform.h to make it C++-friendly

[Razvan Cojocaru]

Moved the definition of struct xenpf_efi_guid up, and rearranged
struct xenpf_efi_time in the containing union to avoid compilation
errors with C++ (structs defined inside unnamed structs become
unavailable outside their scope with C++). The change allows C++
applications to use platform.h with no consequences for current
C clients.

Signed-off-by: Razvan Cojocaru <rcojocaru@bitdefender.com>
---
 xen/include/public/platform.h |   40 +++++++++++++++++++++-------------------
 1 file changed, 21 insertions(+), 19 deletions(-)

diff --git a/xen/include/public/platform.h b/xen/include/public/platform.h
index e4cf65f..2eca375 100644
--- a/xen/include/public/platform.h
+++ b/xen/include/public/platform.h
@@ -141,6 +141,13 @@ 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_runtime_call {
     uint32_t function;
     /*
@@ -152,24 +159,24 @@ struct xenpf_efi_runtime_call {
     xen_ulong_t status;
     union {
 #define XEN_EFI_GET_TIME_SET_CLEARS_NS 0x00000001
+        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;
+        } set_time;
+
         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;
 
-        struct xenpf_efi_time set_time;
-
 #define XEN_EFI_GET_WAKEUP_TIME_ENABLED 0x00000001
 #define XEN_EFI_GET_WAKEUP_TIME_PENDING 0x00000002
         struct xenpf_efi_time get_wakeup_time;
@@ -185,12 +192,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 {
-- 
1.7.9.5

Re: [PATCH] xen: Minor modifications to platform.h to make it C++-friendly

[Jan Beulich]

>>> On 23.02.15 at 10:34, <rcojocaru@bitdefender.com> wrote:
> Moved the definition of struct xenpf_efi_guid up, and rearranged
> struct xenpf_efi_time in the containing union to avoid compilation
> errors with C++ (structs defined inside unnamed structs become
> unavailable outside their scope with C++). The change allows C++
> applications to use platform.h with no consequences for current
> C clients.

What's wrong with simply naming the union/struct, making for a
much smaller change?

> @@ -152,24 +159,24 @@ struct xenpf_efi_runtime_call {
>      xen_ulong_t status;
>      union {
>  #define XEN_EFI_GET_TIME_SET_CLEARS_NS 0x00000001
> +        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;
> +        } set_time;
> +

If we were to go this proposed route, the insertion would need
to happen above the #define.

Jan

Re: [PATCH] xen: Minor modifications to platform.h to make it C++-friendly

[Razvan Cojocaru]

On 02/23/2015 12:16 PM, Jan Beulich wrote:
>>>> On 23.02.15 at 10:34, <rcojocaru@bitdefender.com> wrote:
>> Moved the definition of struct xenpf_efi_guid up, and rearranged
>> struct xenpf_efi_time in the containing union to avoid compilation
>> errors with C++ (structs defined inside unnamed structs become
>> unavailable outside their scope with C++). The change allows C++
>> applications to use platform.h with no consequences for current
>> C clients.
> 
> What's wrong with simply naming the union/struct, making for a
> much smaller change?

The bigger issue is not that the containing struct is unnamed, it is
that from C++'s standpoint the inner struct needs to be explictly
qualified, as if it were in a namespace. The fact that the outer struct
is unnamed makes it impossible to refer to the inner struct, but even if
it were possible to refer to it (by naming the outer struct), it would
still not be valid C code (and the change would arguably not be much
smaller anyway):

154 #define XEN_EFI_GET_TIME_SET_CLEARS_NS 0x00000001
155         struct get_time_t {
156             struct xenpf_efi_time {
157                 uint16_t year;
158                 uint8_t month;
159                 uint8_t day;
160                 uint8_t hour;
161                 uint8_t min;
162                 uint8_t sec;
163                 uint32_t ns;
164                 int16_t tz;
165                 uint8_t daylight;
166             } time;
167             uint32_t resolution;
168             uint32_t accuracy;
169         } get_time;
170
171         struct get_time_t::xenpf_efi_time set_time;
172
173 #define XEN_EFI_GET_WAKEUP_TIME_ENABLED 0x00000001
174 #define XEN_EFI_GET_WAKEUP_TIME_PENDING 0x00000002
175         struct get_time_t::xenpf_efi_time get_wakeup_time;
176
177 #define XEN_EFI_SET_WAKEUP_TIME_ENABLE      0x00000001
178 #define XEN_EFI_SET_WAKEUP_TIME_ENABLE_ONLY 0x00000002
179         struct get_time_t::xenpf_efi_time set_wakeup_time;

This is why I think the proposed solution works best for everyone.

>> @@ -152,24 +159,24 @@ struct xenpf_efi_runtime_call {
>>      xen_ulong_t status;
>>      union {
>>  #define XEN_EFI_GET_TIME_SET_CLEARS_NS 0x00000001
>> +        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;
>> +        } set_time;
>> +
> 
> If we were to go this proposed route, the insertion would need
> to happen above the #define.

Of course, if there are no other objections I'll move it above the
#define and submit V2.


Thanks,
Razvan

Re: [PATCH] xen: Minor modifications to platform.h to make it C++-friendly

[Jan Beulich]

>>> On 23.02.15 at 11:31, <rcojocaru@bitdefender.com> wrote:
> On 02/23/2015 12:16 PM, Jan Beulich wrote:
>>>>> On 23.02.15 at 10:34, <rcojocaru@bitdefender.com> wrote:
>>> Moved the definition of struct xenpf_efi_guid up, and rearranged
>>> struct xenpf_efi_time in the containing union to avoid compilation
>>> errors with C++ (structs defined inside unnamed structs become
>>> unavailable outside their scope with C++). The change allows C++
>>> applications to use platform.h with no consequences for current
>>> C clients.
>> 
>> What's wrong with simply naming the union/struct, making for a
>> much smaller change?
> 
> The bigger issue is not that the containing struct is unnamed, it is
> that from C++'s standpoint the inner struct needs to be explictly
> qualified, as if it were in a namespace. The fact that the outer struct
> is unnamed makes it impossible to refer to the inner struct, but even if
> it were possible to refer to it (by naming the outer struct), it would
> still not be valid C code (and the change would arguably not be much
> smaller anyway):
> 
> 154 #define XEN_EFI_GET_TIME_SET_CLEARS_NS 0x00000001
> 155         struct get_time_t {
> 156             struct xenpf_efi_time {
> 157                 uint16_t year;
> 158                 uint8_t month;
> 159                 uint8_t day;
> 160                 uint8_t hour;
> 161                 uint8_t min;
> 162                 uint8_t sec;
> 163                 uint32_t ns;
> 164                 int16_t tz;
> 165                 uint8_t daylight;
> 166             } time;
> 167             uint32_t resolution;
> 168             uint32_t accuracy;
> 169         } get_time;
> 170
> 171         struct get_time_t::xenpf_efi_time set_time;
> 172
> 173 #define XEN_EFI_GET_WAKEUP_TIME_ENABLED 0x00000001
> 174 #define XEN_EFI_GET_WAKEUP_TIME_PENDING 0x00000002
> 175         struct get_time_t::xenpf_efi_time get_wakeup_time;
> 176
> 177 #define XEN_EFI_SET_WAKEUP_TIME_ENABLE      0x00000001
> 178 #define XEN_EFI_SET_WAKEUP_TIME_ENABLE_ONLY 0x00000002
> 179         struct get_time_t::xenpf_efi_time set_wakeup_time;
> 
> This is why I think the proposed solution works best for everyone.

Ah, right. But then ...

>>> @@ -152,24 +159,24 @@ struct xenpf_efi_runtime_call {
>>>      xen_ulong_t status;
>>>      union {
>>>  #define XEN_EFI_GET_TIME_SET_CLEARS_NS 0x00000001
>>> +        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;
>>> +        } set_time;
>>> +

... this should be moved out to file scope too, both for consistency
and to avoid an eventual further adjustment going forward. Otoh
I'm not convinced we need the headers to be C++ ready (nor am
I convinced that there aren't any other obstacles preventing their
unmodified use in C++). Co-maintainers, what do you think?

Jan

Re: [PATCH] xen: Minor modifications to platform.h to make it C++-friendly

[Razvan Cojocaru]

On 02/23/2015 12:54 PM, Jan Beulich wrote:
>>>> @@ -152,24 +159,24 @@ struct xenpf_efi_runtime_call {
>>>>      xen_ulong_t status;
>>>>      union {
>>>>  #define XEN_EFI_GET_TIME_SET_CLEARS_NS 0x00000001
>>>> +        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;
>>>> +        } set_time;
>>>> +
> 
> ... this should be moved out to file scope too, both for consistency
> and to avoid an eventual further adjustment going forward. Otoh
> I'm not convinced we need the headers to be C++ ready (nor am
> I convinced that there aren't any other obstacles preventing their
> unmodified use in C++). Co-maintainers, what do you think?

You're right, it's always better to be consistent. Will do.

As for the headers being C++ ready, there's already the precedent of at
least my previous patch "xenctrl: Make the headers C++ friendly":

http://www.gossamer-threads.com/lists/xen/devel/337788

where it turned out that there's at least one other serious user of Xen
with C++, Don Slutz.

I do understand and respect the fact that C++ is not a xen-devel target,
but I also believe that there are a very small number of changes that
could enlarge the Xen user base to include C++ developers, and that it
can be done in completely painless manner for all involved.


Thanks,
Razvan

Re: [PATCH] xen: Minor modifications to platform.h to make it C++-friendly

[Jan Beulich]

>>> On 23.02.15 at 12:05, <rcojocaru@bitdefender.com> wrote:
> As for the headers being C++ ready, there's already the precedent of at
> least my previous patch "xenctrl: Make the headers C++ friendly":
> 
> http://www.gossamer-threads.com/lists/xen/devel/337788 
> 
> where it turned out that there's at least one other serious user of Xen
> with C++, Don Slutz.

That's a different story - we're talking about the canonical public
headers here.

> I do understand and respect the fact that C++ is not a xen-devel target,
> but I also believe that there are a very small number of changes that
> could enlarge the Xen user base to include C++ developers, and that it
> can be done in completely painless manner for all involved.

No, if you want to do just that, it's going to get broken again. If we
want the headers to be C++ ready, we ought to introduce a build
time check similar to the one checking that they're plain ANSI C
conforming. Which in turn, as a prerequisite, would need you to
make sure they _all_ are usable in C++. (Such a build time check,
in case you want to propose an extended series, should of course
be conditional upon there being a C++ compiler available - we
clearly don't want to enforce its installation onto everyone
consuming Xen, even if we expect developers to have it installed
in order for them to make sure their patches don't introduce build
breakage).

Jan

Re: [PATCH] xen: Minor modifications to platform.h to make it C++-friendly

[Tim Deegan]

At 10:54 +0000 on 23 Feb (1424685241), Jan Beulich wrote:
> ... this should be moved out to file scope too, both for consistency
> and to avoid an eventual further adjustment going forward. Otoh
> I'm not convinced we need the headers to be C++ ready (nor am
> I convinced that there aren't any other obstacles preventing their
> unmodified use in C++). Co-maintainers, what do you think?

In general I'm against making C code uglier to keep C++ compilers
happy (e.g. ISTR vetoing the idea of putting 'extern C' runes in our
header files a while back).  And we certainly don't _support_ the
header files as C++.

OTOH, I think that just defining struct xenpf_efi_guid and struct
xenpf_efi_time at file scope would be fine here -- if anything it's
nicer to have these reused structs defined before first use.
And if it happens to solve a problem for C++, too, that's great. :)

Cheers,

Tim.

Re: [PATCH] xen: Minor modifications to platform.h to make it C++-friendly

[Jan Beulich]

>>> On 23.02.15 at 12:09, <tim@xen.org> wrote:
> At 10:54 +0000 on 23 Feb (1424685241), Jan Beulich wrote:
>> ... this should be moved out to file scope too, both for consistency
>> and to avoid an eventual further adjustment going forward. Otoh
>> I'm not convinced we need the headers to be C++ ready (nor am
>> I convinced that there aren't any other obstacles preventing their
>> unmodified use in C++). Co-maintainers, what do you think?
> 
> In general I'm against making C code uglier to keep C++ compilers
> happy (e.g. ISTR vetoing the idea of putting 'extern C' runes in our
> header files a while back).  And we certainly don't _support_ the
> header files as C++.
> 
> OTOH, I think that just defining struct xenpf_efi_guid and struct
> xenpf_efi_time at file scope would be fine here -- if anything it's
> nicer to have these reused structs defined before first use.
> And if it happens to solve a problem for C++, too, that's great. :)

But then I'd ask for consistency again - there are a couple of
other similar structure definitions as it seems. And other than full
C++ correctness I can't see how such a rule could be auto-
enforced (to avoid introducing similar future issues - personally
I'm in favor of defining things upon first use where possible, i.e.
I'd be likely to introduce such "nested" structure definitions again
unless prevented from doing so by some automatic check).

Jan

Re: [PATCH] xen: Minor modifications to platform.h to make it C++-friendly

[Razvan Cojocaru]

On 02/23/2015 12:54 PM, Jan Beulich wrote:
> I'm not convinced we need the headers to be C++ ready (nor am
> I convinced that there aren't any other obstacles preventing their
> unmodified use in C++).

Sorry, I meant to answer the last part too. I'm using libxc and xenstore
headers, as well as the public Xen headers with C++, and so far the only
trouble I've had was what this patch is addressing, and there's
something called "private" (a macro or a variable, I don't remember
exactly) which is a keyword in C++. I've done the following to work
around that:

extern "C" {
#include <xenstore.h>
#define private rprivate /* private is a C++ keyword */
#include <xen/mem_event.h>
#undef private
}

I've understood Tim's message about "extern "C"" so I've fixed it at
#include-time in my code. :)


HTH,
Razvan