Re: [PATCH 7/7] tune2fs: Update dir checksums when clearing dir_index feature

[Andreas Dilger <adilger@dilger.ca>]

[-- Attachment #1: Type: text/plain, Size: 1992 bytes --]

On Feb 13, 2020, at 3:16 AM, Jan Kara <jack@suse.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@suse.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






[-- Attachment #2: Message signed with OpenPGP --]
[-- Type: application/pgp-signature, Size: 873 bytes --]