linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] [v2] bcache: hide variable-sized types from uapi header check
@ 2021-09-28  8:55 Arnd Bergmann
  2021-10-18 14:20 ` Coly Li
  0 siblings, 1 reply; 7+ messages in thread
From: Arnd Bergmann @ 2021-09-28  8:55 UTC (permalink / raw)
  To: Kent Overstreet; +Cc: Arnd Bergmann, Jens Axboe, Coly Li, linux-kernel

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


^ permalink raw reply related	[flat|nested] 7+ messages in thread

* Re: [PATCH] [v2] bcache: hide variable-sized types from uapi header check
  2021-09-28  8:55 [PATCH] [v2] bcache: hide variable-sized types from uapi header check Arnd Bergmann
@ 2021-10-18 14:20 ` Coly Li
  2021-10-18 14:39   ` Arnd Bergmann
  0 siblings, 1 reply; 7+ messages in thread
From: Coly Li @ 2021-10-18 14:20 UTC (permalink / raw)
  To: Arnd Bergmann; +Cc: Arnd Bergmann, Jens Axboe, Kent Overstreet, linux-kernel

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
>   


^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] [v2] bcache: hide variable-sized types from uapi header check
  2021-10-18 14:20 ` Coly Li
@ 2021-10-18 14:39   ` Arnd Bergmann
  2021-10-18 14:42     ` Coly Li
  0 siblings, 1 reply; 7+ messages in thread
From: Arnd Bergmann @ 2021-10-18 14:39 UTC (permalink / raw)
  To: Coly Li
  Cc: Arnd Bergmann, Jens Axboe, Kent Overstreet, Linux Kernel Mailing List

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

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] [v2] bcache: hide variable-sized types from uapi header check
  2021-10-18 14:39   ` Arnd Bergmann
@ 2021-10-18 14:42     ` Coly Li
  2021-10-18 15:18       ` Christoph Hellwig
  2021-10-18 15:22       ` Christoph Hellwig
  0 siblings, 2 replies; 7+ messages in thread
From: Coly Li @ 2021-10-18 14:42 UTC (permalink / raw)
  To: Arnd Bergmann
  Cc: Arnd Bergmann, Jens Axboe, Kent Overstreet,
	Linux Kernel Mailing List, Christoph Hellwig

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

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] [v2] bcache: hide variable-sized types from uapi header check
  2021-10-18 14:42     ` Coly Li
@ 2021-10-18 15:18       ` Christoph Hellwig
  2021-10-18 15:22       ` Christoph Hellwig
  1 sibling, 0 replies; 7+ messages in thread
From: Christoph Hellwig @ 2021-10-18 15:18 UTC (permalink / raw)
  To: Coly Li
  Cc: Arnd Bergmann, Arnd Bergmann, Jens Axboe, Kent Overstreet,
	Linux Kernel Mailing List, 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.

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] [v2] bcache: hide variable-sized types from uapi header check
  2021-10-18 14:42     ` Coly Li
  2021-10-18 15:18       ` Christoph Hellwig
@ 2021-10-18 15:22       ` Christoph Hellwig
  2021-10-18 15:26         ` Coly Li
  1 sibling, 1 reply; 7+ messages in thread
From: Christoph Hellwig @ 2021-10-18 15:22 UTC (permalink / raw)
  To: Coly Li
  Cc: Arnd Bergmann, Arnd Bergmann, Jens Axboe, Kent Overstreet,
	Linux Kernel Mailing List, 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.

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] [v2] bcache: hide variable-sized types from uapi header check
  2021-10-18 15:22       ` Christoph Hellwig
@ 2021-10-18 15:26         ` Coly Li
  0 siblings, 0 replies; 7+ messages in thread
From: Coly Li @ 2021-10-18 15:26 UTC (permalink / raw)
  To: Christoph Hellwig, Arnd Bergmann
  Cc: Arnd Bergmann, Jens Axboe, Kent Overstreet, Linux Kernel Mailing List

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

^ permalink raw reply	[flat|nested] 7+ messages in thread

end of thread, other threads:[~2021-10-18 15:26 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-09-28  8:55 [PATCH] [v2] bcache: hide variable-sized types from uapi header check Arnd Bergmann
2021-10-18 14:20 ` Coly Li
2021-10-18 14:39   ` Arnd Bergmann
2021-10-18 14:42     ` Coly Li
2021-10-18 15:18       ` Christoph Hellwig
2021-10-18 15:22       ` Christoph Hellwig
2021-10-18 15:26         ` Coly Li

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).