[RFC] UAPI: Check headers by compiling all together as C++

[RFC] UAPI: Check headers by compiling all together as C++

[David Howells]

Here's a set of patches that inserts a step into the build process to make
sure that the UAPI headers can all be built together with C++ (if the
compiler being used supports C++).  All but the final patch perform fixups,
including:

 (1) Fix member names that conflict with C++ reserved words by providing
     alternates that can be used anywhere.  An anonymous union is used so
     that that the conflicting name is still available outside of C++.

 (2) Fix the use of flexible arrays in structs that get embedded (which is
     illegal in C++).

 (3) Remove the use of internal kernel structs in UAPI structures.

 (4) Fix symbol collisions.

 (5) Replace usage of u32 and co. with __u32 and co.

 (6) Fix use of sparsely initialised arrays (which g++ doesn't implement).

 (7) Remove some use of PAGE_SIZE since this isn't valid outside of the
     kernel.

And lastly:

 (8) Compile all of the UAPI headers (with a few exceptions) together as
     C++ to catch new errors occurring as part of the regular build
     process.

The patches can also be found here:

	http://git.kernel.org/cgit/linux/kernel/git/dhowells/linux-fs.git/log/?h=uapi-check

Thanks,
David
---
David Howells (11):
      UAPI: drm: Fix use of C++ keywords as structural members
      UAPI: keys: Fix use of C++ keywords as structural members
      UAPI: virtio_net: Fix use of C++ keywords as structural members
      UAPI: bcache: Fix use of embedded flexible array
      UAPI: coda: Don't use internal kernel structs in UAPI
      UAPI: netfilter: Fix symbol collision issues
      UAPI: nilfs2: Fix use of undefined byteswapping functions
      UAPI: sound: Fix use of u32 and co. in UAPI headers
      UAPI: ndctl: Fix g++-unsupported initialisation in headers
      UAPI: ndctl: Remove use of PAGE_SIZE
      UAPI: Check headers build for C++


 Makefile                                          |    1 
 include/linux/ndctl.h                             |   22 ++++
 include/uapi/drm/i810_drm.h                       |    7 +
 include/uapi/drm/msm_drm.h                        |    7 +
 include/uapi/linux/bcache.h                       |    2 
 include/uapi/linux/coda_psdev.h                   |    4 +
 include/uapi/linux/keyctl.h                       |    7 +
 include/uapi/linux/ndctl.h                        |   20 ++-
 include/uapi/linux/netfilter/nfnetlink_cthelper.h |    2 
 include/uapi/linux/netfilter_ipv4/ipt_ECN.h       |    9 --
 include/uapi/linux/nilfs2_ondisk.h                |   21 ++--
 include/uapi/linux/virtio_net.h                   |    7 +
 include/uapi/sound/skl-tplg-interface.h           |  106 +++++++++---------
 scripts/headers-c++.sh                            |  124 +++++++++++++++++++++
 14 files changed, 255 insertions(+), 84 deletions(-)
 create mode 100644 include/linux/ndctl.h
 create mode 100755 scripts/headers-c++.sh

[PATCH 05/11] UAPI: coda: Don't use internal kernel structs in UAPI

[David Howells]

The size and layout of internal kernel structures may not be relied upon
outside of the kernel and may even change in a containerised environment if
a container image is frozen and shifted to another machine.

Excise these from Coda's upc_req struct.

Signed-off-by: David Howells <dhowells@redhat.com>
cc: Jan Harkes <jaharkes@cs.cmu.edu>
cc: coda@cs.cmu.edu
cc: codalist@coda.cs.cmu.edu
cc: linux-fsdevel@vger.kernel.org
---

 include/uapi/linux/coda_psdev.h |    4 ++++
 1 file changed, 4 insertions(+)

diff --git a/include/uapi/linux/coda_psdev.h b/include/uapi/linux/coda_psdev.h
index aa6623efd2dd..9c3acde393cd 100644
--- a/include/uapi/linux/coda_psdev.h
+++ b/include/uapi/linux/coda_psdev.h
@@ -10,14 +10,18 @@
 
 /* messages between coda filesystem in kernel and Venus */
 struct upc_req {
+#ifdef __KERNEL__
 	struct list_head    uc_chain;
+#endif
 	caddr_t	            uc_data;
 	u_short	            uc_flags;
 	u_short             uc_inSize;  /* Size is at most 5000 bytes */
 	u_short	            uc_outSize;
 	u_short	            uc_opcode;  /* copied from data to save lookup */
 	int		    uc_unique;
+#ifdef __KERNEL__
 	wait_queue_head_t   uc_sleep;   /* process' wait queue */
+#endif
 };
 
 #define CODA_REQ_ASYNC  0x1

[PATCH 07/11] UAPI: nilfs2: Fix use of undefined byteswapping functions

[David Howells]

nilfs2 exports a load of inline functions to userspace that call kernel
byteswapping functions that don't exist in UAPI.  Fix this by making it
#include asm/byteorder.h and use the functions declared there.

A better way is probably to remove these inline functions from the nilfs2
header since they are technically broken.

Signed-off-by: David Howells <dhowells@redhat.com>
cc: Ryusuke Konishi <konishi.ryusuke@lab.ntt.co.jp>
cc: linux-nilfs@vger.kernel.org
cc: linux-fsdevel@vger.kernel.org
---

 include/uapi/linux/nilfs2_ondisk.h |   21 +++++++++++----------
 1 file changed, 11 insertions(+), 10 deletions(-)

diff --git a/include/uapi/linux/nilfs2_ondisk.h b/include/uapi/linux/nilfs2_ondisk.h
index a7e66ab11d1d..d3bd2fe08791 100644
--- a/include/uapi/linux/nilfs2_ondisk.h
+++ b/include/uapi/linux/nilfs2_ondisk.h
@@ -29,6 +29,7 @@
 
 #include <linux/types.h>
 #include <linux/magic.h>
+#include <asm/byteorder.h>
 
 
 #define NILFS_INODE_BMAP_SIZE	7
@@ -533,19 +534,19 @@ enum {
 static inline void							\
 nilfs_checkpoint_set_##name(struct nilfs_checkpoint *cp)		\
 {									\
-	cp->cp_flags = cpu_to_le32(le32_to_cpu(cp->cp_flags) |		\
+	cp->cp_flags = __cpu_to_le32(__le32_to_cpu(cp->cp_flags) |	\
 				   (1UL << NILFS_CHECKPOINT_##flag));	\
 }									\
 static inline void							\
 nilfs_checkpoint_clear_##name(struct nilfs_checkpoint *cp)		\
 {									\
-	cp->cp_flags = cpu_to_le32(le32_to_cpu(cp->cp_flags) &		\
+	cp->cp_flags = __cpu_to_le32(__le32_to_cpu(cp->cp_flags) &	\
 				   ~(1UL << NILFS_CHECKPOINT_##flag));	\
 }									\
 static inline int							\
 nilfs_checkpoint_##name(const struct nilfs_checkpoint *cp)		\
 {									\
-	return !!(le32_to_cpu(cp->cp_flags) &				\
+	return !!(__le32_to_cpu(cp->cp_flags) &				\
 		  (1UL << NILFS_CHECKPOINT_##flag));			\
 }
 
@@ -595,20 +596,20 @@ enum {
 static inline void							\
 nilfs_segment_usage_set_##name(struct nilfs_segment_usage *su)		\
 {									\
-	su->su_flags = cpu_to_le32(le32_to_cpu(su->su_flags) |		\
+	su->su_flags = __cpu_to_le32(__le32_to_cpu(su->su_flags) |	\
 				   (1UL << NILFS_SEGMENT_USAGE_##flag));\
 }									\
 static inline void							\
 nilfs_segment_usage_clear_##name(struct nilfs_segment_usage *su)	\
 {									\
 	su->su_flags =							\
-		cpu_to_le32(le32_to_cpu(su->su_flags) &			\
+		__cpu_to_le32(__le32_to_cpu(su->su_flags) &		\
 			    ~(1UL << NILFS_SEGMENT_USAGE_##flag));      \
 }									\
 static inline int							\
 nilfs_segment_usage_##name(const struct nilfs_segment_usage *su)	\
 {									\
-	return !!(le32_to_cpu(su->su_flags) &				\
+	return !!(__le32_to_cpu(su->su_flags) &				\
 		  (1UL << NILFS_SEGMENT_USAGE_##flag));			\
 }
 
@@ -619,15 +620,15 @@ NILFS_SEGMENT_USAGE_FNS(ERROR, error)
 static inline void
 nilfs_segment_usage_set_clean(struct nilfs_segment_usage *su)
 {
-	su->su_lastmod = cpu_to_le64(0);
-	su->su_nblocks = cpu_to_le32(0);
-	su->su_flags = cpu_to_le32(0);
+	su->su_lastmod = __cpu_to_le64(0);
+	su->su_nblocks = __cpu_to_le32(0);
+	su->su_flags = __cpu_to_le32(0);
 }
 
 static inline int
 nilfs_segment_usage_clean(const struct nilfs_segment_usage *su)
 {
-	return !le32_to_cpu(su->su_flags);
+	return !__le32_to_cpu(su->su_flags);
 }
 
 /**

Re: [PATCH 05/11] UAPI: coda: Don't use internal kernel structs in UAPI

[Jan Harkes]

On Wed, Sep 05, 2018 at 04:55:10PM +0100, David Howells wrote:
> The size and layout of internal kernel structures may not be relied upon
> outside of the kernel and may even change in a containerised environment if
> a container image is frozen and shifted to another machine.
> 
> Excise these from Coda's upc_req struct.

Argh, that won't work.

I still have to look at where this structure is used exactly, but...

Either this structure is used by the messages that the kernel sends to
userspace, in which case we don't want the kernel to pack the larger
structure that includes a list_head and a wait_queue_head_t in the
message while userspace reads as if it was a smaller structure without
those.

But my gut feeling is that this is not part of the upcall request
messages and never gets to userspace and as such shouldn't be in uapi to
begin with.

Jan

Re: [RFC] UAPI: Check headers by compiling all together as C++

[Greg KH]

On Wed, Sep 05, 2018 at 04:54:27PM +0100, David Howells wrote:
> 
> Here's a set of patches that inserts a step into the build process to make
> sure that the UAPI headers can all be built together with C++ (if the
> compiler being used supports C++).  All but the final patch perform fixups,
> including:

Wait, why do we care?  What has recently changed to start to directly
import kernel uapi files into C++ code?

And if userspace wants to do this, can't they do the C namespace trick
themselves when they do the import?  That must be how they are doing it
today, right?

thanks,

greg k-h

Re: [PATCH 05/11] UAPI: coda: Don't use internal kernel structs in UAPI

[Yann Droneaud]

Le mercredi 05 septembre 2018 à 16:55 +0100, David Howells a écrit :
> The size and layout of internal kernel structures may not be relied
> upon outside of the kernel and may even change in a containerised
> environment if a container image is frozen and shifted to another
> machine.
> 
> Excise these from Coda's upc_req struct.
> 
> Signed-off-by: David Howells <dhowells@redhat.com>
> cc: Jan Harkes <jaharkes@cs.cmu.edu>
> cc: coda@cs.cmu.edu
> cc: codalist@coda.cs.cmu.edu
> cc: linux-fsdevel@vger.kernel.org
> ---
> 
>  include/uapi/linux/coda_psdev.h |    4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/include/uapi/linux/coda_psdev.h b/include/uapi/linux/coda_psdev.h
> index aa6623efd2dd..9c3acde393cd 100644
> --- a/include/uapi/linux/coda_psdev.h
> +++ b/include/uapi/linux/coda_psdev.h
> @@ -10,14 +10,18 @@
>  
>  /* messages between coda filesystem in kernel and Venus */
>  struct upc_req {
> +#ifdef __KERNEL__
>  	struct list_head    uc_chain;
> +#endif
>  	caddr_t	            uc_data;
>  	u_short	            uc_flags;
>  	u_short             uc_inSize;  /* Size is at most 5000 bytes */
>  	u_short	            uc_outSize;
>  	u_short	            uc_opcode;  /* copied from data to save lookup */
>  	int		    uc_unique;
> +#ifdef __KERNEL__
>  	wait_queue_head_t   uc_sleep;   /* process' wait queue */
> +#endif
>  };
>  

This structure should not have been exposed to userspace in the first
place: it's unusable by userspace as it is. It was incorrect to have it
outside of #ifdef __KERNEL__ before commit 607ca46e97a1b ... 

... and it's not exchanged between kernel and userspace, see
coda_psdev_write():

        struct upc_req *req = NULL;

        ...

        if (copy_from_user(req->uc_data, buf, nbytes)) {
                req->uc_flags |= CODA_REQ_ABORT;
                wake_up(&req->uc_sleep);
                retval = -EFAULT;
                goto out;
        }

Only data, a caddr_t, is read from userspace.

So the structure can be moved back to <linux/coda_psdev.h>.

>  #define CODA_REQ_ASYNC  0x1
> 

All CODA_REQ_* defines internals to kernel side and not exchanged with userspace.

Please move them back to <linux/coda_psdev.h>

Regards.

-- 
Yann Droneaud
OPTEYA

Re: [PATCH 05/11] UAPI: coda: Don't use internal kernel structs in UAPI

[David Howells]

Yann Droneaud <ydroneaud@opteya.com> wrote:

> Please move them back to <linux/coda_psdev.h>

Will do.

David

Re: [PATCH 05/11] UAPI: coda: Don't use internal kernel structs in UAPI

[Jan Harkes]

On Wed, Sep 05, 2018 at 07:12:37PM +0200, Yann Droneaud wrote:
> Le mercredi 05 septembre 2018 � 16:55 +0100, David Howells a �crit :
> > The size and layout of internal kernel structures may not be relied
> > upon outside of the kernel and may even change in a containerised
> > environment if a container image is frozen and shifted to another
> > machine.
> > 
> > Excise these from Coda's upc_req struct.
...
> 
> This structure should not have been exposed to userspace in the first
> place: it's unusable by userspace as it is. It was incorrect to have it
> outside of #ifdef __KERNEL__ before commit 607ca46e97a1b ... 
...
> So the structure can be moved back to <linux/coda_psdev.h>.

I found a year old patch that clearly fell through the cracks that
fixes this exact thing.

    https://lkml.org/lkml/2017/8/6/186

Jan

Re: [RFC] UAPI: Check headers by compiling all together as C++

[Yann Droneaud]

Hi,

Le mercredi 05 septembre 2018 à 18:55 +0200, Greg KH a écrit :
> On Wed, Sep 05, 2018 at 04:54:27PM +0100, David Howells wrote:
> > 
> > Here's a set of patches that inserts a step into the build process to make
> > sure that the UAPI headers can all be built together with C++ (if the
> > compiler being used supports C++).  All but the final patch perform fixups,
> > including:
> 
> Wait, why do we care?  What has recently changed to start to directly
> import kernel uapi files into C++ code?
> 
> And if userspace wants to do this, can't they do the C namespace trick
> themselves when they do the import?  That must be how they are doing it
> today, right?
> 

They can't.


Adding extern "C" { } doesn't magically make "class" a non keyword.
Even if it was the case, writing C++ code using whatever->class would
probably broke because class is a keyword in C++.

-- 
Yann Droneaud
OPTEYA

Re: [RFC] UAPI: Check headers by compiling all together as C++

[Michael S. Tsirkin]

On Wed, Sep 05, 2018 at 07:33:38PM +0200, Yann Droneaud wrote:
> Hi,
> 
> Le mercredi 05 septembre 2018 � 18:55 +0200, Greg KH a �crit :
> > On Wed, Sep 05, 2018 at 04:54:27PM +0100, David Howells wrote:
> > > 
> > > Here's a set of patches that inserts a step into the build process to make
> > > sure that the UAPI headers can all be built together with C++ (if the
> > > compiler being used supports C++).  All but the final patch perform fixups,
> > > including:
> > 
> > Wait, why do we care?  What has recently changed to start to directly
> > import kernel uapi files into C++ code?
> > 
> > And if userspace wants to do this, can't they do the C namespace trick
> > themselves when they do the import?  That must be how they are doing it
> > today, right?
> > 
> 
> They can't.
> 
> 
> Adding extern "C" { } doesn't magically make "class" a non keyword.
> Even if it was the case, writing C++ code using whatever->class would
> probably broke because class is a keyword in C++.

I think it's a bug in the language TBH.

> -- 
> Yann Droneaud
> OPTEYA
> 

Re: [RFC] UAPI: Check headers by compiling all together as C++

[David Howells]

Greg KH <greg@kroah.com> wrote:

> > Here's a set of patches that inserts a step into the build process to make
> > sure that the UAPI headers can all be built together with C++ (if the
> > compiler being used supports C++).  All but the final patch perform fixups,
> > including:
> 
> Wait, why do we care?  What has recently changed to start to directly
> import kernel uapi files into C++ code?

There's at least one outstanding bug due to a C++ identifier in the kernel
UAPI headers.

Are you saying you explicitly don't want people to be able to use the kernel
UAPI headers in C++?

> And if userspace wants to do this, can't they do the C namespace trick
> themselves when they do the import?  That must be how they are doing it
> today, right?

No, because there's no such trick (except with the preprocessor).

David

Re: [RFC] UAPI: Check headers by compiling all together as C++

[Jan Engelhardt]

On Wednesday 2018-09-05 18:55, Greg KH wrote:

>On Wed, Sep 05, 2018 at 04:54:27PM +0100, David Howells wrote:
>> 
>> Here's a set of patches that inserts a step into the build process to make
>> sure that the UAPI headers can all be built together with C++ (if the
>> compiler being used supports C++).  All but the final patch perform fixups,
>> including:
>
>Wait, why do we care?  What has recently changed to start to directly
>import kernel uapi files into C++ code?

With C++11, C++ has become a much nicer language to use (for userspace, anyway).

>And if userspace wants to do this, can't they do the C namespace trick
>themselves when they do the import?

The only trick is to use an extra C source file and extensively wrap things.

Re: [PATCH 07/11] UAPI: nilfs2: Fix use of undefined byteswapping functions

[Al Viro]

On Wed, Sep 05, 2018 at 04:55:23PM +0100, David Howells wrote:
>  nilfs_checkpoint_set_##name(struct nilfs_checkpoint *cp)		\
>  {									\
> -	cp->cp_flags = cpu_to_le32(le32_to_cpu(cp->cp_flags) |		\
> +	cp->cp_flags = __cpu_to_le32(__le32_to_cpu(cp->cp_flags) |	\
>  				   (1UL << NILFS_CHECKPOINT_##flag));	\

How about sanitiziung the damn thing to
	cp->cp_flags |= __cpu_to_le32(1UL << NILFS_CHECKPOINT_##flag));

while you are at it?  Or, perhaps, even
#define NILFS2_CP_FLAG(flag) __cpu_to_le32(1UL << NILFS_CHECKPOINT_##flag)

and cp->cp_flags |= NILFS2_CP_FLAG(flag) for this one,

>  }									\
>  static inline void							\
>  nilfs_checkpoint_clear_##name(struct nilfs_checkpoint *cp)		\
>  {									\
> -	cp->cp_flags = cpu_to_le32(le32_to_cpu(cp->cp_flags) &		\
> +	cp->cp_flags = __cpu_to_le32(__le32_to_cpu(cp->cp_flags) &	\
>  				   ~(1UL << NILFS_CHECKPOINT_##flag));	\
	cp->cp_flags &= ~NILFS2_CP_FLAG(flag);
here

>  }									\
>  static inline int							\
>  nilfs_checkpoint_##name(const struct nilfs_checkpoint *cp)		\
>  {									\
> -	return !!(le32_to_cpu(cp->cp_flags) &				\
> +	return !!(__le32_to_cpu(cp->cp_flags) &				\
>  		  (1UL << NILFS_CHECKPOINT_##flag));			\

and !!(cp->cp_flags & NILFS2_CP_FLAG(flag)
here?  Or maybe even make the damn thing bool and lose the !! here...

)>  }
>  

and similar for those:

> @@ -595,20 +596,20 @@ enum {
>  static inline void							\
>  nilfs_segment_usage_set_##name(struct nilfs_segment_usage *su)		\
>  {									\
> -	su->su_flags = cpu_to_le32(le32_to_cpu(su->su_flags) |		\
> +	su->su_flags = __cpu_to_le32(__le32_to_cpu(su->su_flags) |	\
>  				   (1UL << NILFS_SEGMENT_USAGE_##flag));\
>  }									\
>  static inline void							\
>  nilfs_segment_usage_clear_##name(struct nilfs_segment_usage *su)	\
>  {									\
>  	su->su_flags =							\
> -		cpu_to_le32(le32_to_cpu(su->su_flags) &			\
> +		__cpu_to_le32(__le32_to_cpu(su->su_flags) &		\
>  			    ~(1UL << NILFS_SEGMENT_USAGE_##flag));      \
>  }									\
>  static inline int							\
>  nilfs_segment_usage_##name(const struct nilfs_segment_usage *su)	\
>  {									\
> -	return !!(le32_to_cpu(su->su_flags) &				\
> +	return !!(__le32_to_cpu(su->su_flags) &				\
>  		  (1UL << NILFS_SEGMENT_USAGE_##flag));			\
>  }



> @@ -619,15 +620,15 @@ NILFS_SEGMENT_USAGE_FNS(ERROR, error)
>  static inline void
>  nilfs_segment_usage_set_clean(struct nilfs_segment_usage *su)
>  {
> -	su->su_lastmod = cpu_to_le64(0);
> -	su->su_nblocks = cpu_to_le32(0);
> -	su->su_flags = cpu_to_le32(0);
> +	su->su_lastmod = __cpu_to_le64(0);
> +	su->su_nblocks = __cpu_to_le32(0);
> +	su->su_flags = __cpu_to_le32(0);
>  }
>  
>  static inline int
>  nilfs_segment_usage_clean(const struct nilfs_segment_usage *su)
>  {
> -	return !le32_to_cpu(su->su_flags);
> +	return !__le32_to_cpu(su->su_flags);

"Check that after byteswap it becomes 0", is it?  How is that different
from return !su->su_flags; ?

Re: [RFC] UAPI: Check headers by compiling all together as C++

[Yann Droneaud]

Le mercredi 05 septembre 2018 à 19:33 +0200, Yann Droneaud a écrit :
> Le mercredi 05 septembre 2018 à 18:55 +0200, Greg KH a écrit :
> > On Wed, Sep 05, 2018 at 04:54:27PM +0100, David Howells wrote:
> > > 
> > > Here's a set of patches that inserts a step into the build
> > > process to make
> > > sure that the UAPI headers can all be built together with C++ (if
> > > the
> > > compiler being used supports C++).  All but the final patch
> > > perform fixups,
> > > including:
> > 
> > Wait, why do we care?  What has recently changed to start to
> > directly
> > import kernel uapi files into C++ code?
> > 
> > And if userspace wants to do this, can't they do the C namespace
> > trick
> > themselves when they do the import?  That must be how they are
> > doing it
> > today, right?
> > 
> 
> They can't.
> 
> 
> Adding extern "C" { } doesn't magically make "class" a non keyword.
> Even if it was the case, writing C++ code using whatever->class would
> probably broke because class is a keyword in C++.
> 

For the record, libX11 has to handle the kink pf issue with C++
keyword:


https://gitlab.freedesktop.org/xorg/lib/libx11/blob/733f64bfeb311c1d040b2f751bfdef9c9d0f89ef/include/X11/Xlib.h#L227

typedef struct {
	XExtData *ext_data;	/* hook for extension to hang data */
	VisualID visualid;	/* visual id of this visual */
#if defined(__cplusplus) || defined(c_plusplus)
	int c_class;		/* C++ class of screen (monochrome, etc.) */
#else
	int class;		/* class of screen (monochrome, etc.) */
#endif
	unsigned long red_mask, green_mask, blue_mask;	/* mask values */
	int bits_per_rgb;	/* log base 2 of distinct color values */
	int map_entries;	/* color map entries */
} Visual;


Regards.

-- 
Yann Droneaud
OPTEYA

Re: [PATCH 05/11] UAPI: coda: Don't use internal kernel structs in UAPI

[David Howells]

Yann Droneaud <ydroneaud@opteya.com> wrote:

> This structure should not have been exposed to userspace in the first
> place: it's unusable by userspace as it is. It was incorrect to have it
> outside of #ifdef __KERNEL__ before commit 607ca46e97a1b ... 
> ...
> All CODA_REQ_* defines internals to kernel side and not exchanged with
> userspace.
> 
> Please move them back to <linux/coda_psdev.h>

Is there any reason coda_psdev.h needs to be in include/linux/ rather than
fs/coda/?

David

Re: [PATCH 05/11] UAPI: coda: Don't use internal kernel structs in UAPI

[Yann Droneaud]

Hi,

Le jeudi 06 septembre 2018 à 08:13 +0100, David Howells a écrit :
> Yann Droneaud <ydroneaud@opteya.com> wrote:
> 
> > This structure should not have been exposed to userspace in the
> > first
> > place: it's unusable by userspace as it is. It was incorrect to
> > have it
> > outside of #ifdef __KERNEL__ before commit 607ca46e97a1b ... 
> > ...
> > All CODA_REQ_* defines internals to kernel side and not exchanged
> > with
> > userspace.
> > 
> > Please move them back to <linux/coda_psdev.h>
> 
> Is there any reason coda_psdev.h needs to be in include/linux/ rather
> than fs/coda/?
> 

It's a valid concern.

At first I thought the first lines (see below) could have been useful
for userspace:

  #define CODA_PSDEV_MAJOR 67
  #define MAX_CODADEVS  5    /* how many do we allow */


But the file was unsuable for a long long time so we can assume it's
usage by userspace is deprecated, then we could remove it from UAPI,
and moves its content back to include/linux.

As one could see include/linux/coda_psdev.h is not used outside of
fs/coda, moving the header here as you suggests seems to be the correct
solution.

Regards.

-- 
Yann Droneaud
OPTEYA

Re: [PATCH 05/11] UAPI: coda: Don't use internal kernel structs in UAPI

[Jan Harkes]

On Thu, Sep 06, 2018 at 01:52:29PM +0200, Yann Droneaud wrote:
> Hi,
> 
> Le jeudi 06 septembre 2018 � 08:13 +0100, David Howells a �crit :
> > Yann Droneaud <ydroneaud@opteya.com> wrote:
> > 
> > > This structure should not have been exposed to userspace in the
> > > first
> > > place: it's unusable by userspace as it is. It was incorrect to
> > > have it
> > > outside of #ifdef __KERNEL__ before commit 607ca46e97a1b ... 
> > > ...
> > > All CODA_REQ_* defines internals to kernel side and not exchanged
> > > with
> > > userspace.
> > > 
> > > Please move them back to <linux/coda_psdev.h>
> > 
> > Is there any reason coda_psdev.h needs to be in include/linux/ rather
> > than fs/coda/?
> > 
> 
> It's a valid concern.
> 
> At first I thought the first lines (see below) could have been useful
> for userspace:
> 
>   #define CODA_PSDEV_MAJOR 67
>   #define MAX_CODADEVS  5    /* how many do we allow */

Nope, userspace just tries to open /dev/cfs0, or a manually configured
alternative. We have only used linux/coda.h, and actually carry our own
copy of that file which is kept in sync manually, which is why there are
all those ifdefs for different systems in there. This all originates
from the time of the 2.1.x kernels when Coda was built externally.

> But the file was unsuable for a long long time so we can assume it's
> usage by userspace is deprecated, then we could remove it from UAPI,
> and moves its content back to include/linux.
> 
> As one could see include/linux/coda_psdev.h is not used outside of
> fs/coda, moving the header here as you suggests seems to be the correct
> solution.

Agreed.

Jan

Re: [PATCH 05/11] UAPI: coda: Don't use internal kernel structs in UAPI

[David Howells]

Yann Droneaud <ydroneaud@opteya.com> wrote:

> At first I thought the first lines (see below) could have been useful
> for userspace:
> 
>   #define CODA_PSDEV_MAJOR 67
>   #define MAX_CODADEVS  5    /* how many do we allow */

Note that I was asking about include/linux/coda_psdev.h (the internal kernel
header), not include/uapi/linux/coda_psdev.h (the UAPI header).

David

[RFC] UAPI: Check headers by compiling all together as C++

[David Howells]

Here's a set of patches that inserts a step into the build process to make
sure that the UAPI headers can all be built together with C++ (if the
compiler being used supports C++).

Note that it's based on a commit from the sound tree to fix usage of u32
and co..

Most of the patches perform fixups, including:

 (1) Fix member names that conflict with C++ reserved words by providing
     alternates that can be used anywhere.  An anonymous union is used so
     that that the conflicting name is still available outside of C++.

 (2) Fix the use of flexible arrays in structs that get embedded (which is
     illegal in C++).

 (3) Remove the use of internal kernel structs in UAPI structures.

 (4) Fix symbol collisions.

 (5) Fix use of sparsely initialised arrays (which g++ doesn't implement).

 (6) Remove some use of PAGE_SIZE since this isn't valid outside of the
     kernel.

There's also:

 (7) Move the coda_psdev.h header file to fs/coda/.

And lastly:

 (8) Compile all of the UAPI headers (with a few exceptions) together as
     C++ to catch new errors occurring as part of the regular build
     process.

Changes for v2:

 - Merge commit from sound tree to fix u32 usage issues
 - Use a switch to fix sparse array initialisation
 - Simplify nilfs2 by performing bitwise ops in LE space not CPU space
 - Handle conflicting fix to use of 'private' in keyctl.h
 - Move kernel internal coda bits to coda internal headers
 - Move coda_psdev.h header to fs/coda/.

The patches can also be found here:

	http://git.kernel.org/cgit/linux/kernel/git/dhowells/linux-fs.git/log/?h=uapi-check

Thanks,
David
---
David Howells (11):
      UAPI: drm: Fix use of C++ keywords as structural members
      UAPI: keys: Fix use of C++ keywords as structural members
      UAPI: virtio_net: Fix use of C++ keywords as structural members
      UAPI: bcache: Fix use of embedded flexible array
      UAPI: coda: Move kernel internals out of public view
      coda: Move internal defs out of include/linux/
      UAPI: netfilter: Fix symbol collision issues
      UAPI: nilfs2: Fix use of undefined byteswapping functions
      UAPI: ndctl: Fix g++-unsupported initialisation in headers
      UAPI: ndctl: Remove use of PAGE_SIZE
      UAPI: Check headers build for C++


 Makefile                                          |    1 
 fs/coda/cache.c                                   |    2 
 fs/coda/cnode.c                                   |    2 
 fs/coda/coda_linux.c                              |    2 
 fs/coda/coda_psdev.h                              |   88 +++++++++++++++
 fs/coda/dir.c                                     |    2 
 fs/coda/file.c                                    |    3 -
 fs/coda/inode.c                                   |    2 
 fs/coda/pioctl.c                                  |    3 -
 fs/coda/psdev.c                                   |    3 -
 fs/coda/symlink.c                                 |    3 -
 fs/coda/upcall.c                                  |    2 
 include/linux/coda_psdev.h                        |   72 ------------
 include/linux/ndctl.h                             |   22 ++++
 include/uapi/drm/i810_drm.h                       |    7 +
 include/uapi/drm/msm_drm.h                        |    7 +
 include/uapi/linux/bcache.h                       |    2 
 include/uapi/linux/coda_psdev.h                   |   18 ---
 include/uapi/linux/keyctl.h                       |    7 +
 include/uapi/linux/ndctl.h                        |   52 ++++-----
 include/uapi/linux/netfilter/nfnetlink_cthelper.h |    2 
 include/uapi/linux/netfilter_ipv4/ipt_ECN.h       |    9 --
 include/uapi/linux/nilfs2_ondisk.h                |   28 ++---
 include/uapi/linux/virtio_net.h                   |    7 +
 scripts/headers-c++.sh                            |  124 +++++++++++++++++++++
 25 files changed, 304 insertions(+), 166 deletions(-)
 create mode 100644 fs/coda/coda_psdev.h
 delete mode 100644 include/linux/coda_psdev.h
 create mode 100644 include/linux/ndctl.h
 create mode 100755 scripts/headers-c++.sh