Re: [PATCH 0/3] actually detect bad file modes in fsck

From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date: Aug 11, 2022, 09:39 UTC
Message-ID: <220811.86r11nqfi2.gmgdl@evledraar.gmail.com>
In-Reply-To: <YvQcNpizy9uOZiAz@coredump.intra.peff.net>

On Wed, Aug 10 2022, Jeff King wrote:

Show 18 quoted lines

> On Wed, Aug 10, 2022 at 04:04:17PM -0400, Jeff King wrote:
>
>> > Maybe downgrade to info or ignore by default then? It might still be
>> > an issue for people who wilfully upgraded the diagnostic to error
>> > hoping to catch the, but hopefully if they did that they'd rather get
>> > the notice later than never?
>> 
>> Yeah, that may be a sensible resolution. All things being equal I think
>> "warning" is the right level, but out of caution and the historical
>> precedent, maybe downgrading it to "info" is justified.
>> 
>> It should be easy to work that into the patch I showed earlier.
>
> OK, so here are cleaned-up patches to do that.
>
>   [1/3]: tree-walk: add a mechanism for getting non-canonicalized modes
>   [2/3]: fsck: actually detect bad file modes in trees
>   [3/3]: fsck: downgrade tree badFilemode to "info"

This LGTM.

I noticed/reported this issue more than a year ago, but the series I had for fixing it ended up being dropped, here's the report/analysis at the time: https://lore.kernel.org/git/20210308150650.18626-31-avarab@gmail.com/

Basically I was taking a much longer way around to avoid...

Show 8 quoted lines

>  	/* counts the number of bytes left in the `buffer`. */
>  	unsigned int size;
> +
> +	/* option flags passed via init_tree_desc_gently() */
> +	enum tree_desc_flags {
> +		TREE_DESC_RAW_MODES = (1 << 0),
> +	} flags;
>  };

...this from 1/3 here, i.e. now we're paying the cost of an extra entry in every "struct tree_desc" user (which includes some hot codepaths), and just for this one fsck caller.

I wonder if we couldn't introduce a init_tree_desc_gently_flags() for this instead. You note in 1/3 that you're (rightly) avoiding the churn of existing callers, but they could just use a "static inline" wrapper function that would set those flags to 0, we'd pass the flags down, and not put them into the "tree_desc" struct.

Arguably it doesn't belong there at all, since this new thing is really an "opts" struct, but "tree_desc" is for "the state of the walk".

It might/would make sense as a "raw_mode" in "struct name_entry" perhaps, but then we're gettin closer to the larger scope of my initial larger series, oh well ... :)