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?
