| Message ID | 20220911033137.4010427-1-zhangshida@kylinos.cn (mailing list archive) |
|---|---|
| State | Deferred, archived |
| Headers | show |
| Series | xfs: fix up the comment in xfs_dir2_isleaf | expand |
On Sun, Sep 11, 2022 at 11:31:37AM +0800, Stephen Zhang wrote: > xfs_dir2_isleaf is used to see if the directory is a single-leaf > form directory, as commented right above the function. > > Signed-off-by: Shida Zhang <zhangshida@kylinos.cn> > --- > fs/xfs/libxfs/xfs_dir2.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/fs/xfs/libxfs/xfs_dir2.c b/fs/xfs/libxfs/xfs_dir2.c > index 76eedc2756b3..1485f53fecf4 100644 > --- a/fs/xfs/libxfs/xfs_dir2.c > +++ b/fs/xfs/libxfs/xfs_dir2.c > @@ -632,7 +632,7 @@ xfs_dir2_isblock( > int > xfs_dir2_isleaf( > struct xfs_da_args *args, > - int *vp) /* out: 1 is block, 0 is not block */ > + int *vp) /* out: 1 is leaf, 0 is not leaf */ > { > xfs_fileoff_t last; /* last file offset */ > int rval; If you are going to touch this code to fix a broken comment, then please clean it up properly. The "*vp" parameter should be a "bool *isleaf", in which case the return value is obvious and the comment can be removed. Then the logic in the function can be cleaned up to be obvious instead of relying on easy to mistake conditional logic in assignemnts... IOWs, don't fix the comments - fix the code to be self documenting to remove the need for comments... Cheers, Dave.
Dave Chinner <david@fromorbit.com> 于2022年9月12日周一 06:20写道： > > The "*vp" parameter should be a "bool *isleaf", in which case the > return value is obvious and the comment can be removed. Then the > logic in the function can be cleaned up to be obvious instead of > relying on easy to mistake conditional logic in assignemnts... Thanks for the suggestion.In order to make sure we are at the same page, so this change will be shown like: ==== xfs_dir2_isblock( struct xfs_da_args *args, - int *vp) /* out: 1 is block, 0 is not block */ + bool *isblock) { xfs_fileoff_t last; /* last file offset */ int rval; if ((rval = xfs_bmap_last_offset(args->dp, &last, XFS_DATA_FORK))) return rval; - rval = XFS_FSB_TO_B(args->dp->i_mount, last) == args->geo->blksize; + *isblock = XFS_FSB_TO_B(args->dp->i_mount, last) == args->geo->blksize; if (XFS_IS_CORRUPT(args->dp->i_mount, - rval != 0 && + *isblock && args->dp->i_disk_size != args->geo->blksize)) return -EFSCORRUPTED; - *vp = rval; return 0; } ==== Thanks, Stephen.
On Mon, Sep 12, 2022 at 11:14:51AM +0800, Stephen Zhang wrote: > Dave Chinner <david@fromorbit.com> 于2022年9月12日周一 06:20写道： > > > > The "*vp" parameter should be a "bool *isleaf", in which case the > > return value is obvious and the comment can be removed. Then the > > logic in the function can be cleaned up to be obvious instead of > > relying on easy to mistake conditional logic in assignemnts... > > Thanks for the suggestion.In order to make sure we are at the same page, > so this change will be shown like: That's not what I was thinking. Cleanup involves converting everything over to standard formatting and conventions. It also means rethinking the logic to make the code more correct, easier to read and understand, and so involves more than just changing the name of a variable. > ==== > xfs_dir2_isblock( > struct xfs_da_args *args, > - int *vp) /* out: 1 is block, 0 is not block */ > + bool *isblock) > { > xfs_fileoff_t last; /* last file offset */ > int rval; > > if ((rval = xfs_bmap_last_offset(args->dp, &last, XFS_DATA_FORK))) > return rval; We don't put assingments in if statements anymore, so this needs to be rewritten in the form: error = foo(); if (error) { .... return error; } > - rval = XFS_FSB_TO_B(args->dp->i_mount, last) == args->geo->blksize; > + *isblock = XFS_FSB_TO_B(args->dp->i_mount, last) == args->geo->blksize; Similarly, we don't elide if() statements in this way anymore, because it's easy to mistake this code as a multiple assignment rather than a combination of assignment and logic. if() is much clearer. > if (XFS_IS_CORRUPT(args->dp->i_mount, > - rval != 0 && > + *isblock && > args->dp->i_disk_size != args->geo->blksize)) > return -EFSCORRUPTED; And this only ever evaluates as true if *isblock is true, so why run this logic check when *isblock is false? IOWs, we can rearrange the logic so that it's made up of simple, individual single comparisons that are obviously self documenting: int xfs_dir2_isblock( struct xfs_da_args *args, bool *isblock) { struct xfs_mount *mp = args->dp->i_mount; xfs_fileoff_t eof; int error; error = xfs_bmap_last_offset(args->dp, &eof, XFS_DATA_FORK); if (error) return error; *isblock = false; if (XFS_FSB_TO_B(mp, eof) != args->geo->blksize) return 0; *isblock = true; if (XFS_IS_CORRUPT(mp, args->dp->i_disk_size != args->geo->blksize)) return -EFSCORRUPTED; return 0; }
diff --git a/fs/xfs/libxfs/xfs_dir2.c b/fs/xfs/libxfs/xfs_dir2.c index 76eedc2756b3..1485f53fecf4 100644 --- a/fs/xfs/libxfs/xfs_dir2.c +++ b/fs/xfs/libxfs/xfs_dir2.c @@ -632,7 +632,7 @@ xfs_dir2_isblock( int xfs_dir2_isleaf( struct xfs_da_args *args, - int *vp) /* out: 1 is block, 0 is not block */ + int *vp) /* out: 1 is leaf, 0 is not leaf */ { xfs_fileoff_t last; /* last file offset */ int rval;
xfs_dir2_isleaf is used to see if the directory is a single-leaf form directory, as commented right above the function. Signed-off-by: Shida Zhang <zhangshida@kylinos.cn> --- fs/xfs/libxfs/xfs_dir2.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)