From: Arnd Bergmann <[email protected]>
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 <[email protected]>
---
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
On 9/28/21 4:55 PM, Arnd Bergmann wrote:
> From: Arnd Bergmann <[email protected]>
>
> 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 <[email protected]>
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
>
On Mon, Oct 18, 2021 at 4:20 PM Coly Li <[email protected]> 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
On 10/18/21 10:39 PM, Arnd Bergmann wrote:
> On Mon, Oct 18, 2021 at 4:20 PM Coly Li <[email protected]> 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
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.
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.
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