fs: ufs: Convert ufs_set_de_type to use lookup table

PATCH In review Phillip Potter 2 participants 3 messages Oct 1, 2018 – Oct 2, 2018

Share & tools

Phillip Potter

author

2018-10-01 15:33 UTC #0 lore raw

PATCH 1/1 fs: ufs: Convert ufs_set_de_type to use lookup table

Modify ufs_set_de_type function in fs/ufs/util.h to use a lookup table rather than a switch statement, as per the TODO comment.

Signed-off-by Phillip Potter <[email protected]>

---

fs/ufs/util.h +22 ~~-28~~

index 1907be6d5808..1edf4c6454e3 100644
@@ -155,37 +155,31 @@ ufs_set_de_namlen(struct super_block *sb, struct ufs_dir_entry *de, u16 value)
 static inline void
 ufs_set_de_type(struct super_block *sb, struct ufs_dir_entry *de, int mode)
 {
+	/* type lookup table, DT_UNKNOWN is default type if no case holds */
+	const int mode_table[] = {
+		DT_UNKNOWN,
+		DT_FIFO,	/* mode & S_IFMT == S_IFIFO case */
+		DT_CHR,		/* mode & S_IFMT == S_IFCHR case */
+		DT_UNKNOWN,
+		DT_DIR,		/* mode & S_IFMT == S_IFDIR case */
+		DT_UNKNOWN,
+		DT_BLK,		/* mode & S_IFMT == S_IFBLK case */
+		DT_UNKNOWN,
+		DT_REG,		/* mode & S_IFMT == S_IFREG case */
+		DT_UNKNOWN,
+		DT_LNK,		/* mode & S_IFMT == S_IFLNK case */
+		DT_UNKNOWN,
+		DT_SOCK,	/* mode & S_IFMT == S_IFSOCK case */
+		DT_UNKNOWN,
+		DT_UNKNOWN,
+		DT_UNKNOWN
+	};
+
 	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;
-	}
+	/* shift (mode & S_IFMT) right 12 bits to index into table */
+	de->d_u.d_44.d_type = mode_table[(mode & S_IFMT) >> 12];
 }
 
 static inline u32

Al Viro

2018-10-02 02:28 UTC #1 lore raw

Quoted 2 lines · Phillip Potter

Modify ufs_set_de_type function in fs/ufs/util.h to use a lookup table rather than a switch statement, as per the TODO comment.

Brittle, that... Something like fs/ext2/dir.c approach (that is,
#define S_SHIFT 12

static unsigned char ext2_type_by_mode[S_IFMT >> S_SHIFT] = {
        [S_IFREG >> S_SHIFT]    = EXT2_FT_REG_FILE,
        [S_IFDIR >> S_SHIFT]    = EXT2_FT_DIR,
        [S_IFCHR >> S_SHIFT]    = EXT2_FT_CHRDEV,
        [S_IFBLK >> S_SHIFT]    = EXT2_FT_BLKDEV,
        [S_IFIFO >> S_SHIFT]    = EXT2_FT_FIFO,
        [S_IFSOCK >> S_SHIFT]   = EXT2_FT_SOCK,
        [S_IFLNK >> S_SHIFT]    = EXT2_FT_SYMLINK,
};

in there) would be saner, IMO. Note that DT_UNKNOWN is zero, so the array elements lacking an explicit initializer will end up with that.

What's more, the values are ->i_mode >> 12 or 0, depending upon the value being valid. And since the upper layers do validate the type, I'd consider simply using (inode->i_mode & S_IFMT) >> 12 there. Unlike ext2, ufs stores straight bits 12..15 there (ext2 uses an enum with sequential values instead; e.g. regular files are encoded as 1 there, not 8 as on ufs)...

Phillip Potter

author

2018-10-02 08:31 UTC #2 lore raw

Quoted 18 lines · Al Viro

Quoted 2 lines · Phillip Potter
Modify ufs_set_de_type function in fs/ufs/util.h to use a lookup table rather than a switch statement, as per the TODO comment.
Brittle, that... Something like fs/ext2/dir.c approach (that is,
#define S_SHIFT 12
static unsigned char ext2_type_by_mode[S_IFMT >> S_SHIFT] = {
        [S_IFREG >> S_SHIFT]    = EXT2_FT_REG_FILE,
        [S_IFDIR >> S_SHIFT]    = EXT2_FT_DIR,
        [S_IFCHR >> S_SHIFT]    = EXT2_FT_CHRDEV,
        [S_IFBLK >> S_SHIFT]    = EXT2_FT_BLKDEV,
        [S_IFIFO >> S_SHIFT]    = EXT2_FT_FIFO,
        [S_IFSOCK >> S_SHIFT]   = EXT2_FT_SOCK,
        [S_IFLNK >> S_SHIFT]    = EXT2_FT_SYMLINK,
};
in there) would be saner, IMO. Note that DT_UNKNOWN is zero, so the array elements lacking an explicit initializer will end up with that.

Quoted 6 lines

What's more, the values are ->i_mode >> 12 or 0, depending upon the value being valid. And since the upper layers do validate the type, I'd consider simply using (inode->i_mode & S_IFMT) >> 12 there. Unlike ext2, ufs stores straight bits 12..15 there (ext2 uses an enum with sequential values instead; e.g. regular files are encoded as 1 there, not 8 as on ufs)...

Dear Al,

Thank you for taking time to offer your feedback, I really appreciate it. I will rework this patch this evening and resubmit.

Regards,
Phil