| Message ID | 4A9EE6E4.2000307@redhat.com |
|---|---|
| State | Accepted, archived |
| Headers | show |
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. Cheers, Andreas -- Andreas Dilger Sr. Staff Engineer, Lustre Group Sun Microsystems of Canada, Inc. -- To unsubscribe from this list: send the line "unsubscribe linux-ext4" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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. 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 -- To unsubscribe from this list: send the line "unsubscribe linux-ext4" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Sep 02, 2009 22:00 -0500, Eric Sandeen wrote: > 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. Yes, this is exactly what I would expect from this kind of interface. Cheers, Andreas -- Andreas Dilger Sr. Staff Engineer, Lustre Group Sun Microsystems of Canada, Inc. -- To unsubscribe from this list: send the line "unsubscribe linux-ext4" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Andreas Dilger wrote: > On Sep 02, 2009 22:00 -0500, Eric Sandeen wrote: >> 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. > > Yes, this is exactly what I would expect from this kind of interface. Ok, I'll try to get that sent out tomorrow, and any other similar interfaces. Thanks, -Eric -- To unsubscribe from this list: send the line "unsubscribe linux-ext4" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Index: e2fsprogs/lib/ext2fs/blknum.c =================================================================== --- e2fsprogs.orig/lib/ext2fs/blknum.c +++ e2fsprogs/lib/ext2fs/blknum.c @@ -435,9 +435,9 @@ void ext2fs_bg_flags_set(ext2_filsys fs, } /* - * Clear the flags for this block group + * Clear all flags for this block group */ -void ext2fs_bg_flags_clear(ext2_filsys fs, dgrp_t group, __u16 bg_flags) +void ext2fs_bg_flags_clear(ext2_filsys fs, dgrp_t group) { if (fs->super->s_desc_size >= EXT2_MIN_DESC_SIZE_64BIT) { struct ext4_group_desc *gdp; Index: e2fsprogs/lib/ext2fs/csum.c =================================================================== --- e2fsprogs.orig/lib/ext2fs/csum.c +++ e2fsprogs/lib/ext2fs/csum.c @@ -204,7 +204,7 @@ int main(int argc, char **argv) fs->group_desc[i].bg_free_blocks_count = 31119; fs->group_desc[i].bg_free_inodes_count = 15701; fs->group_desc[i].bg_used_dirs_count = 2; - ext2fs_bg_flags_clear(fs, i, 0); + ext2fs_bg_flags_clear(fs, i); }; csum1 = ext2fs_group_desc_csum(fs, 0); Index: e2fsprogs/resize/resize2fs.c =================================================================== --- e2fsprogs.orig/resize/resize2fs.c +++ e2fsprogs/resize/resize2fs.c @@ -495,7 +495,7 @@ retry: sizeof(struct ext2_group_desc)); adjblocks = 0; - ext2fs_bg_flags_clear(fs, i, 0); + ext2fs_bg_flags_clear(fs, i); if (csum_flag) ext2fs_bg_flag_set(fs, i, EXT2_BG_INODE_UNINIT | EXT2_BG_INODE_ZEROED) ; Index: e2fsprogs/lib/ext2fs/ext2fs.h =================================================================== --- e2fsprogs.orig/lib/ext2fs/ext2fs.h +++ e2fsprogs/lib/ext2fs/ext2fs.h @@ -777,8 +777,7 @@ extern void ext2fs_bg_itable_unused_set( blk64_t blk); extern __u16 ext2fs_bg_flags(ext2_filsys fs, dgrp_t group); extern void ext2fs_bg_flags_set(ext2_filsys fs, dgrp_t group, __u16 bg_flags); -extern void ext2fs_bg_flags_clear(ext2_filsys fs, dgrp_t group, - __u16 bg_flags); +extern void ext2fs_bg_flags_clear(ext2_filsys fs, dgrp_t group); extern int ext2fs_bg_flag_test(ext2_filsys fs, dgrp_t group, __u16 bg_flag); extern void ext2fs_bg_flag_set(ext2_filsys fs, dgrp_t group, __u16 bg_flag); extern void ext2fs_bg_flag_clear(ext2_filsys fs, dgrp_t group, __u16 bg_flag);
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 ;) Signed-off-by: Eric Sandeen <sandeen@redhat.com> --- -- To unsubscribe from this list: send the line "unsubscribe linux-ext4" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html