Skip to contentSkip to conversation

fs: ufs: Remove switch statement from ufs_set_de_type function

PATCH In review Phillip Potter 3 participants 4 messages Oct 17, 2018 – Oct 18, 2018 VFS

lore
Share & tools
lore

Phillip Potter

author
#0 lore raw

PATCH 1/1 fs: ufs: Remove switch statement from ufs_set_de_type function

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 <[email protected]>
---
 fs/ufs/util.h | 30 ++----------------------------
 1 file changed, 2 insertions(+), 28 deletions(-)
fs/ufs/util.h +2 -28
index 1fd3011ea623..7e0c0878b9f9 100644
@@ -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
Signature · 1 line
2.17.0
#1 lore raw

From: Phillip Potter

Quoted 9 lines

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.

...

fs/ufs/util.h ufs_set_de_type(struct super_block *sb, struct ufs_dir_entry *de, int mode) · line 162

Quoted 7 lines
fs/ufs/util.h +1 -6
-	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)

maintainer
#2 lore raw
Quoted 11 lines · David Laight

From: Phillip Potter

Quoted 9 lines

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.

...

Quoted 9 lines

fs/ufs/util.h ufs_set_de_type(struct super_block *sb, struct ufs_dir_entry *de, int mode) · line 162

Quoted 7 lines
fs/ufs/util.h +1 -6
-	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.

author
#3 lore raw
Quoted 9 lines · Al Viro

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