Theodore Tso wrote:
On Wed, Sep 02, 2009 at 05:28:11PM -0600, Andreas Dilger wrote:
On Sep 02, 2009 16:43 -0500, Eric Sandeen wrote:
ext2fs_bg_flags_clear shouldn't take an unused bg_flags argument
if its purpose is to clear -all- flags. That just makes people
like me call it for the wrong purpose ;)
I'd pointed this out when the code was originally submitted.
That said, I'd prefer a function which allows clearing
individual flags, rather than all of them. It is possible to
call it with ~0 to clear all of the flags.
ext2fs_bg_flag_clear(fs, group, flag) does just that...
TBH "ext2fs_bg_clear_flags(fs, group, ~0)" seems a bit tortured to me.
Agreed; if we want to have a function which clears all of the flags,
it should be named something like ext2fs_bg_flags_zap(), or some such.
It would be confusing for ext2fs_bg_flags_clear() and
ext2fs_bg_flags_set() not to be symmetric.
- Ted
But they are symmetric; just a bit odd in usage, esp. since the argument
is ignored.
Ok should have looked closer; we have these which overwrite bg_flags:
void ext2fs_bg_flags_set(ext2_filsys fs, dgrp_t group, __u16 bg_flags);
void ext2fs_bg_flags_clear(ext2_filsys fs, dgrp_t group,__u16 bg_flags);
(_set sets exactly bg_flags; _clear clears all and ignores bg_flags)
and these, which can twiddle individual bits in bg_flags:
void ext2fs_bg_flag_set(ext2_filsys fs, dgrp_t group, __u16 bg_flag);
void ext2fs_bg_flag_clear(ext2_filsys fs, dgrp_t group, __u16 bg_flag);
hrmph. Ok, not the most obvious. I still don't think that an unused
variable to "ext2fs_bg_flags_clear" is helpful, regardless of the name.
Oh, and ext2fs_bg_flags_set is unused it seems.
What about perhaps just these 3:
ext2fs_bg_flags_zero(fs, group) /* zeros bg_flags */
ext2fs_bg_flags_set(fs, group, flags) /* adds flags to bg_flags */
ext2fs_bg_flags_clear(fs, group, flags) /* clears flags in bg_flags */
and remove the original ext2fs_bg_flags_set / ext2fs_bg_flags_clear.
-Eric
--
To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
