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

[Tim Deegan <tim@xxxxxxx>]

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@xxxxxxxxxx> 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@xxxxxxx>
> 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@xxxxxxx>
> ---
>  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
> 

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