| Message ID | 20181017100809.GA9070@pathfinder (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | fs: ufs: Remove switch statement from ufs_set_de_type function | expand |
From: Phillip Potter > Sent: 17 October 2018 11:08 > > Remove switch statement from ufs_set_de_type function in fs/ufs/util.h > header and replace with simple assignment. For each case, S_IFx >> 12 > is equal to DT_x, so in valid cases (mode & S_IFMT) >> 12 should give > us the correct file type. For invalid cases, upper layer validation > catches this anyway, so this improves readability and arguably > performance by assigning (mode & S_IFMT) >> 12 directly. > ... > - case S_IFIFO: > - de->d_u.d_44.d_type = DT_FIFO; > - break; > - default: > - de->d_u.d_44.d_type = DT_UNKNOWN; > - } > + de->d_u.d_44.d_type = (mode & S_IFMT) >> S_SHIFT; This requires that the two sets of constants are correctly aligned. If they aren't defined in terms of each other then you probably ought to add compile-time asserts that the values match. David - Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK Registration No: 1397386 (Wales)
On Wed, Oct 17, 2018 at 10:11:47AM +0000, David Laight wrote: > From: Phillip Potter > > Sent: 17 October 2018 11:08 > > > > Remove switch statement from ufs_set_de_type function in fs/ufs/util.h > > header and replace with simple assignment. For each case, S_IFx >> 12 > > is equal to DT_x, so in valid cases (mode & S_IFMT) >> 12 should give > > us the correct file type. For invalid cases, upper layer validation > > catches this anyway, so this improves readability and arguably > > performance by assigning (mode & S_IFMT) >> 12 directly. > > > ... > > - case S_IFIFO: > > - de->d_u.d_44.d_type = DT_FIFO; > > - break; > > - default: > > - de->d_u.d_44.d_type = DT_UNKNOWN; > > - } > > + de->d_u.d_44.d_type = (mode & S_IFMT) >> S_SHIFT; > > This requires that the two sets of constants are correctly aligned. They are. BSD folks had (sanely, IMO) put the 'type' bits of st_mode into directory entry verbatim. Again, "symbolic constant" != "can be expected to change"... If, e.g., some port decides to change S_IFIFO, they'll have no end of fun accessing ext*, xfs, ufs, etc. since that value is stored in the on-disk inode and in effect pinned down by that. All S_... constants are universal and going to remain unchanged on any Unices.
On Thu, Oct 18, 2018 at 12:33:05AM +0100, Al Viro wrote: > They are. BSD folks had (sanely, IMO) put the 'type' bits of st_mode > into directory entry verbatim. Again, "symbolic constant" != "can be > expected to change"... If, e.g., some port decides to change S_IFIFO, > they'll have no end of fun accessing ext*, xfs, ufs, etc. since that > value is stored in the on-disk inode and in effect pinned down by > that. > > All S_... constants are universal and going to remain unchanged on > any Unices. Dear Al, Shall I resubmit without the compile-time checks in the latest patch then? Regards, Phil
diff --git a/fs/ufs/util.h b/fs/ufs/util.h index 1fd3011ea623..7e0c0878b9f9 100644 --- a/fs/ufs/util.h +++ b/fs/ufs/util.h @@ -16,6 +16,7 @@ * some useful macros */ #define in_range(b,first,len) ((b)>=(first)&&(b)<(first)+(len)) +#define S_SHIFT 12 /* * functions used for retyping @@ -158,34 +159,7 @@ ufs_set_de_type(struct super_block *sb, struct ufs_dir_entry *de, int mode) if ((UFS_SB(sb)->s_flags & UFS_DE_MASK) != UFS_DE_44BSD) return; - /* - * TODO turn this into a table lookup - */ - switch (mode & S_IFMT) { - case S_IFSOCK: - de->d_u.d_44.d_type = DT_SOCK; - break; - case S_IFLNK: - de->d_u.d_44.d_type = DT_LNK; - break; - case S_IFREG: - de->d_u.d_44.d_type = DT_REG; - break; - case S_IFBLK: - de->d_u.d_44.d_type = DT_BLK; - break; - case S_IFDIR: - de->d_u.d_44.d_type = DT_DIR; - break; - case S_IFCHR: - de->d_u.d_44.d_type = DT_CHR; - break; - case S_IFIFO: - de->d_u.d_44.d_type = DT_FIFO; - break; - default: - de->d_u.d_44.d_type = DT_UNKNOWN; - } + de->d_u.d_44.d_type = (mode & S_IFMT) >> S_SHIFT; } static inline u32
Remove switch statement from ufs_set_de_type function in fs/ufs/util.h header and replace with simple assignment. For each case, S_IFx >> 12 is equal to DT_x, so in valid cases (mode & S_IFMT) >> 12 should give us the correct file type. For invalid cases, upper layer validation catches this anyway, so this improves readability and arguably performance by assigning (mode & S_IFMT) >> 12 directly. Signed-off-by: Phillip Potter <phil@philpotter.co.uk> --- fs/ufs/util.h | 30 ++---------------------------- 1 file changed, 2 insertions(+), 28 deletions(-)