linux-ext4 - Re: [PATCH 7/7] tune2fs: Update dir checksums when clearing dir

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20200219102303.GL16121@quack2.suse.cz>
Date:   Wed, 19 Feb 2020 11:23:03 +0100
From:   Jan Kara <jack@...e.cz>
To:     Andreas Dilger <adilger@...ger.ca>
Cc:     Jan Kara <jack@...e.cz>, Ted Tso <tytso@....edu>,
        linux-ext4@...r.kernel.org
Subject: Re: [PATCH 7/7] tune2fs: Update dir checksums when clearing
 dir_index feature

On Tue 18-02-20 13:50:33, Andreas Dilger wrote:
> On Feb 13, 2020, at 3:16 AM, Jan Kara <jack@...e.cz> wrote:
> > 
> > When clearing dir_index feature while metadata_csum is enabled, we have
> > to rewrite checksums of all indexed directories to update checksums of
> > internal tree nodes.
> > 
> > Signed-off-by: Jan Kara <jack@...e.cz>
> > ---
> > 
> > +#define REWRITE_EA_FL		0x01	/* Rewrite EA inodes */
> > +#define REWRITE_DIR_FL		0x02	/* Rewrite directories */
> > +#define REWRITE_NONDIR_FL	0x04	/* Rewrite other inodes */
> > +#define REWRITE_ALL (REWRITE_EA_FL | REWRITE_DIR_FL | REWRITE_NONDIR_FL)
> > +
> > +static void rewrite_inodes_pass(struct rewrite_context *ctx, unsigned int flags)
> > {
> 
> My preference these days is to put constants like the above into a named
> enum, and then use the enum as the argument to the function rather than
> a very generic "int flags" argument.  That makes it clear to the reader
> what values the flags may hold, and can immediately tag to the enum, like:
> 
> enum rewrite_inodes_flags {
> 	REWRITE_EA_FL	  = 0x01	/* Rewrite EA inodes */
> 	REWRITE_DIR_FL	  = 0x02	/* Rewrite directories */
> 	REWRITE_NONDIR_FL = 0x04	/* Rewrite other inodes */
> 	REWRITE_ALL	  = REWRITE_EA_FL | REWRITE_DIR_FL | REWRITE_NONDIR_FL
> };
> 
> static void rewrite_inodes_pass(struct rewrite_context *ctx,
> 				enum rewrite_inodes_flags rif_flags)
> static void rewrite_inodes(ext2_filsys fs, enum rewrite_inodes_flags rif_flags)
> static void rewrite_metadata_checksums(ext2_filsys fs,
> 				       enum rewrite_inodes_flags rif_flags)
> 
> Otherwise, when looking at a function that takes "int flags" as an argument,
> you have to dig through the code to see what kind of flags these are, and
> what possible values they might have.  This is often even more confusing when
> there are multiple different kinds of flags accessed within a single function
> (not the case here, but happens often enough).
> 
> I'm not _against_ the patch, just thought I'd suggest an improvement and see
> what people think about it.

Yeah, the documentation with enum type is nice. What I somewhat dislike is
that enum suggests 'enumeration' but we actually use values (like say
REWRITE_EA_FL | REWRITE_DIR_FL) which are not really enumerated in the type
definitition. So your scheme works only because enum in C is just an int
with a lipstick on it. So I'm somewhat undecided. Ted, what's your opinion
on this?

								Honza
-- 
Jan Kara <jack@...e.com>
SUSE Labs, CR