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: <7BA5024A-9600-4D2E-8D23-7A0F900BFE7F@dilger.ca>
Date:   Tue, 18 Feb 2020 13:50:33 -0700
From:   Andreas Dilger <adilger@...ger.ca>
To:     Jan Kara <jack@...e.cz>
Cc:     Ted Tso <tytso@....edu>, linux-ext4@...r.kernel.org
Subject: Re: [PATCH 7/7] tune2fs: Update dir checksums when clearing dir_index
 feature

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.

Cheers, Andreas

Download attachment "signature.asc" of type "application/pgp-signature" (874 bytes)