Re: linux-next: Tree for Sep 12 (bcachefs)

[Gustavo A. R. Silva]

On 9/14/23 13:38, Kent Overstreet wrote:
> On Wed, Sep 13, 2023 at 06:17:00PM -0700, Kees Cook wrote:
> > On Tue, Sep 12, 2023 at 03:26:45PM +1000, Stephen Rothwell wrote:
> > > New tree: bcachefs
> >
> > Thanks for going through and fixing all the fake flexible array members.
> > It looks much nicer. :)
> >
> > I have some questions about the remaining "markers", for example:
> >
> > $ git grep -A8 '\bkey_start\b' -- fs/bcachefs
> > fs/bcachefs/bcachefs_format.h:  __u8            key_start[0];
> > ...
> > fs/bcachefs/bcachefs_format.h-  __u8            pad[sizeof(struct bkey) - 3];
> > --
> > fs/bcachefs/bkey.c:     u8 *l = k->key_start;
> >
> > Why isn't this just:
> >
> > 			u8 *l = k->pad
> >
> > and you can drop the marker?
>
> In this case, it's documentation. &k->pad tells us nothing; why is pad
> significant? k->key_start documents the intent better.
>
> > And some seem entirely unused, like all of "struct bch_reflink_v".
>
> No, those aren't unused :)
>
> bcachefs does the "list of variable size items" a lot - see vstructs.h.
> start[] is the type of the item being stored, _data is what we use for
> pointer arithmetic - because we always store sizes in units of u64s, for
> alignment.
>
> > And some are going to fail at runtime, since they're still zero-sized
> > and being used as an actual array:
> >
> > struct bch_sb_field_journal_seq_blacklist {
> >          struct bch_sb_field     field;
> >
> >          struct journal_seq_blacklist_entry start[0];
> >          __u64                   _data[];
> > };
> > ...
> >                  memmove(&bl->start[i],
> >                          &bl->start[i + 1],
> >                          sizeof(bl->start[0]) * (nr - i));
> >
> > It looks like you just want a type union for the flexible array.
> > This can be done like this:
> >
> > struct bch_sb_field_journal_seq_blacklist {
> >          struct bch_sb_field     field;
> >
> > 	union {
> > 		DECLARE_FLEX_ARRAY(struct journal_seq_blacklist_entry, start);
> > 		DECLARE_FLEX_ARRAY(__u64, _data);
> > 	};
> > };
>
> Eesh, why though?
>
> Honestly, I'm not a fan of the change to get rid of zero size arrays,
> this seems to be adding a whole lot of macro layering and indirection
> for nothing.
>
> The only thing a zero size array could possibly be is a flexible array
> member or a marker, why couldn't we have just kept treating zero size
> arrays like flexible array members?

Because zero-length arrays, when used as fake flexible arrays, make
things like -Warray-bounds (we've been trying to enable this compiler
option, globally) trip; among other things like being prone to result in
undefined behavior bugs when people introduce new members that make the
array end up in the middle of its containing structure.

With C99 flexible-array members, the compiler emits a warning when the
arrays are not at the end of the structure.

The DECLARE_FLEX_ARRAY() (in a union) helper allows for multiple C99
flexible-array members together at the end of a struct.

--
Gustavo