[PATCH] [v2] bcache: hide variable-sized types from uapi header check

[PATCH] [v2] bcache: hide variable-sized types from uapi header check

[Arnd Bergmann]

From: Arnd Bergmann <arnd@arndb.de>

The headers_check helper complains about a GNU extension in
one of the exported headers:

linux/bcache.h:354:2: warning: field '' with variable sized type 'union jset::(anonymous at ./usr/include/linux/bcache.h:354:2)' not at the end of a struct or class is a GNU extension [-Wgnu-variable-sized-type-not-at-end]
        BKEY_PADDED(uuid_bucket);
        ^
linux/bcache.h:134:2: note: expanded from macro 'BKEY_PADDED'
        union { struct bkey key; __u64 key ## _pad[BKEY_PAD]; }
        ^

Address this by enclosing the GNU extensions in an #ifdef: Since the
header check is done with --std=c90, this shuts up the warning and makes
it possible to include the header file C90 user space applications, but
allows applications built with --std=gnu89 or higher to use those
parts.

Another alternative would be to exclude this header from the check,
but the GNU extension check seems more sensible.

Fixes: 81ab4190ac17 ("bcache: Pull on disk data structures out into a separate header")
Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
v2: use __STRICT_ANSI__ check instead of __KERNEL__.

I think this version is better than the first, let me know if that
works for you.
---
 include/uapi/linux/bcache.h | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/include/uapi/linux/bcache.h b/include/uapi/linux/bcache.h
index cf7399f03b71..b7901e986193 100644
--- a/include/uapi/linux/bcache.h
+++ b/include/uapi/linux/bcache.h
@@ -23,9 +23,13 @@ static inline void SET_##name(type *k, __u64 v)			\
 struct bkey {
 	__u64	high;
 	__u64	low;
+#ifndef __STRICT_ANSI__
+	/* gcc extension not meant for user space */
 	__u64	ptr[];
+#endif
 };
 
+#ifndef __STRICT_ANSI__
 #define KEY_FIELD(name, field, offset, size)				\
 	BITMASK(name, struct bkey, field, offset, size)
 
@@ -127,6 +131,8 @@ static inline struct bkey *bkey_idx(const struct bkey *k, unsigned int nr_keys)
 
 	return (struct bkey *) (d + nr_keys);
 }
+#endif
+
 /* Enough for a key with 6 pointers */
 #define BKEY_PAD		8
 
-- 
2.29.2

Re: [PATCH] [v2] bcache: hide variable-sized types from uapi header check

[Coly Li]

On 9/28/21 4:55 PM, Arnd Bergmann wrote:
> From: Arnd Bergmann <arnd@arndb.de>
>
> The headers_check helper complains about a GNU extension in
> one of the exported headers:
>
> linux/bcache.h:354:2: warning: field '' with variable sized type 'union jset::(anonymous at ./usr/include/linux/bcache.h:354:2)' not at the end of a struct or class is a GNU extension [-Wgnu-variable-sized-type-not-at-end]
>          BKEY_PADDED(uuid_bucket);
>          ^
> linux/bcache.h:134:2: note: expanded from macro 'BKEY_PADDED'
>          union { struct bkey key; __u64 key ## _pad[BKEY_PAD]; }
>          ^
>
> Address this by enclosing the GNU extensions in an #ifdef: Since the
> header check is done with --std=c90, this shuts up the warning and makes
> it possible to include the header file C90 user space applications, but
> allows applications built with --std=gnu89 or higher to use those
> parts.
>
> Another alternative would be to exclude this header from the check,
> but the GNU extension check seems more sensible.
>
> Fixes: 81ab4190ac17 ("bcache: Pull on disk data structures out into a separate header")
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>

Hi Arnd,

IMHO, remove bcache related header from uapi check might be better 
solution. So far only bcache-tools uses this header with its own copy, 
no application includes the header(s) so far. It makes sense to exclude 
bcache.h from upai headers check.

Thanks.

Coly Li

> ---
> v2: use __STRICT_ANSI__ check instead of __KERNEL__.
>
> I think this version is better than the first, let me know if that
> works for you.
> ---
>   include/uapi/linux/bcache.h | 6 ++++++
>   1 file changed, 6 insertions(+)
>
> diff --git a/include/uapi/linux/bcache.h b/include/uapi/linux/bcache.h
> index cf7399f03b71..b7901e986193 100644
> --- a/include/uapi/linux/bcache.h
> +++ b/include/uapi/linux/bcache.h
> @@ -23,9 +23,13 @@ static inline void SET_##name(type *k, __u64 v)			\
>   struct bkey {
>   	__u64	high;
>   	__u64	low;
> +#ifndef __STRICT_ANSI__
> +	/* gcc extension not meant for user space */
>   	__u64	ptr[];
> +#endif
>   };
>   
> +#ifndef __STRICT_ANSI__
>   #define KEY_FIELD(name, field, offset, size)				\
>   	BITMASK(name, struct bkey, field, offset, size)
>   
> @@ -127,6 +131,8 @@ static inline struct bkey *bkey_idx(const struct bkey *k, unsigned int nr_keys)
>   
>   	return (struct bkey *) (d + nr_keys);
>   }
> +#endif
> +
>   /* Enough for a key with 6 pointers */
>   #define BKEY_PAD		8
>   

Re: [PATCH] [v2] bcache: hide variable-sized types from uapi header check

[Arnd Bergmann]

On Mon, Oct 18, 2021 at 4:20 PM Coly Li <colyli@suse.de> wrote:
>
> IMHO, remove bcache related header from uapi check might be better
> solution. So far only bcache-tools uses this header with its own copy,
> no application includes the header(s) so far. It makes sense to exclude
> bcache.h from upai headers check.

Should we just move it to include/linux/ and out of the uapi headers entirely
then? It sounds like it's not actually an ABI but just the definition of the
data layout that is not included by anything from user space.

We are a bit inconsistent here already, e.g. btrfs has all its structures
in uapi, but ext4 does not.

       Arnd

Re: [PATCH] [v2] bcache: hide variable-sized types from uapi header check

[Coly Li]

On 10/18/21 10:39 PM, Arnd Bergmann wrote:
> On Mon, Oct 18, 2021 at 4:20 PM Coly Li <colyli@suse.de> wrote:
>> IMHO, remove bcache related header from uapi check might be better
>> solution. So far only bcache-tools uses this header with its own copy,
>> no application includes the header(s) so far. It makes sense to exclude
>> bcache.h from upai headers check.
> Should we just move it to include/linux/ and out of the uapi headers entirely
> then? It sounds like it's not actually an ABI but just the definition of the
> data layout that is not included by anything from user space.
>
> We are a bit inconsistent here already, e.g. btrfs has all its structures
> in uapi, but ext4 does not.

I am quite open for this idea. It is in uapi directory before I maintain 
bcache. I just though the header fines on-media format should go into 
include/uapi/, but if this is not the restricted rule, it is fine for me 
to move this header to drivers/md/bcache/.

Coly Li

Re: [PATCH] [v2] bcache: hide variable-sized types from uapi header check

[Christoph Hellwig]

On Mon, Oct 18, 2021 at 10:42:37PM +0800, Coly Li wrote:
> I am quite open for this idea. It is in uapi directory before I maintain 
> bcache. I just though the header fines on-media format should go into 
> include/uapi/, but if this is not the restricted rule, it is fine for me to 
> move this header to drivers/md/bcache/.

Having the on-disk format in uapi seems weird.  It isn't a userspace
ABI in any way.

Re: [PATCH] [v2] bcache: hide variable-sized types from uapi header check

[Christoph Hellwig]

On Mon, Oct 18, 2021 at 10:42:37PM +0800, Coly Li wrote:
> I am quite open for this idea. It is in uapi directory before I maintain 
> bcache. I just though the header fines on-media format should go into 
> include/uapi/, but if this is not the restricted rule, it is fine for me to 
> move this header to drivers/md/bcache/.

Treating the on-disk definitions as a UAPI doesn't make much sense to
me.  I like keeping it in a separate header to make clear what is the
on-disk format and what is just in core, but it should normally live
with the rest of the code.

Re: [PATCH] [v2] bcache: hide variable-sized types from uapi header check

[Coly Li]

On 10/18/21 11:22 PM, Christoph Hellwig wrote:
> On Mon, Oct 18, 2021 at 10:42:37PM +0800, Coly Li wrote:
>> I am quite open for this idea. It is in uapi directory before I maintain
>> bcache. I just though the header fines on-media format should go into
>> include/uapi/, but if this is not the restricted rule, it is fine for me to
>> move this header to drivers/md/bcache/.
> Treating the on-disk definitions as a UAPI doesn't make much sense to
> me.  I like keeping it in a separate header to make clear what is the
> on-disk format and what is just in core, but it should normally live
> with the rest of the code.

Hi Christoph,

Thanks for the input. So it is fine for me to have the header in 
drivers/md/bcache.

Hi Arnd,

There is another bcache.h in drivers/md/bcache/, let me handle the 
header naming issue, and I will post a change to move the bcache.h from 
include/uapi/linux to driver/md/bcache later.
And I will ask you to help to review the change.

Coly Li