On Tue 18-02-20 13:50:33, Andreas Dilger wrote:
> On Feb 13, 2020, at 3:16 AM, Jan Kara <jack@xxxxxxx> 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@xxxxxxx>
> > ---
> >
> > +#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@xxxxxxxx>
SUSE Labs, CR
