[PATCH] ufs: Add table lookup to set d_type based on mode & S_IFMT

[PATCH] ufs: Add table lookup to set d_type based on mode & S_IFMT

[Abhinav Jain]

Add usf_de_type_mapping structure to map file mode masks to dir entries.
Add a static ufs_type_table array to map file mode to dir entries.
Remove switch and add a table based lookup for the mapping.
Add ARRAY_SIZE macro on ufs_type_table to eliminate checkpatch warning.

Signed-off-by: Abhinav Jain <jain.abhinav177@gmail.com>
---
 fs/ufs/util.h | 57 +++++++++++++++++++++++++++++----------------------
 1 file changed, 32 insertions(+), 25 deletions(-)

diff --git a/fs/ufs/util.h b/fs/ufs/util.h
index 0ecd2ed792f5..8a941d69270a 100644
--- a/fs/ufs/util.h
+++ b/fs/ufs/util.h
@@ -9,7 +9,28 @@
 
 #include <linux/buffer_head.h>
 #include <linux/fs.h>
+#include <linux/types.h>
 #include "swab.h"
+#include "ufs_fs.h"
+
+#ifndef ARRAY_SIZE
+#define ARRAY_SIZE(arr) (sizeof(arr) / sizeof((arr)[0]))
+#endif
+
+struct ufs_de_type_mapping {
+	umode_t mode_mask;
+	unsigned char d_type;
+};
+
+static const struct ufs_de_type_mapping ufs_type_table[] = {
+	{ S_IFSOCK, DT_SOCK },
+	{ S_IFLNK,  DT_LNK },
+	{ S_IFREG,  DT_REG },
+	{ S_IFBLK,  DT_BLK },
+	{ S_IFDIR,  DT_DIR },
+	{ S_IFCHR,  DT_CHR },
+	{ S_IFIFO,  DT_FIFO },
+};
 
 /*
  * functions used for retyping
@@ -153,32 +174,18 @@ ufs_set_de_type(struct super_block *sb, struct ufs_dir_entry *de, int mode)
 		return;
 
 	/*
-	 * TODO turn this into a table lookup
+	 * Table lookup to set d_type based on mode & S_IFMT
 	 */
-	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;
+
+	size_t i;
+	/* Default to DT_UNKNOWN if not found */
+	de->d_u.d_44.d_type = DT_UNKNOWN;
+
+	for (i = 0; i < ARRAY_SIZE(ufs_type_table); i++) {
+		if ((mode & S_IFMT) == ufs_type_table[i].mode_mask) {
+			de->d_u.d_44.d_type = ufs_type_table[i].d_type;
+			break;
+		}
 	}
 }
 
-- 
2.34.1

Re: [PATCH] ufs: Add table lookup to set d_type based on mode & S_IFMT

[Al Viro]

On Thu, Jun 13, 2024 at 08:26:50PM +0000, Abhinav Jain wrote:
> Add usf_de_type_mapping structure to map file mode masks to dir entries.
> Add a static ufs_type_table array to map file mode to dir entries.
> Remove switch and add a table based lookup for the mapping.
> Add ARRAY_SIZE macro on ufs_type_table to eliminate checkpatch warning.

For one thing, ARRAY_SIZE is already defined.  Finding the header
in question (and figuring out if its include needs to be added) is
left as an exercise for reader.

For another, you have added a copy of that array to *every* *file*
that inludes "util.h".  Finding the number of those files is, again,
left as an exercise for reader.

Finally, this

> +	for (i = 0; i < ARRAY_SIZE(ufs_type_table); i++) {
> +		if ((mode & S_IFMT) == ufs_type_table[i].mode_mask) {
> +			de->d_u.d_44.d_type = ufs_type_table[i].d_type;
> +			break;
> +		}

should've raised an arseload of mental red flags.  That loop is
bloody ridiculous, even if you don't bother to check what other
similar filesystems actually do in the counterpart of that logics.

"Table lookup" does *NOT* refer to that.  What you've got is strictly
worse than the original switch, and that takes some doing.

NAK.

Re: [PATCH] ufs: Add table lookup to set d_type based on mode & S_IFMT

[Al Viro]

On Thu, Jun 13, 2024 at 09:40:47PM +0100, Al Viro wrote:

> should've raised an arseload of mental red flags.  That loop is
> bloody ridiculous, even if you don't bother to check what other
> similar filesystems actually do in the counterpart of that logics.
> 
> "Table lookup" does *NOT* refer to that.  What you've got is strictly
> worse than the original switch, and that takes some doing.
> 
> NAK.

Note, BTW, that for DT_... values are exactly the same as bits 12..15 of
mode_t.  That's not accidental.  So for valid mode values it's simply

{
        if ((UFS_SB(sb)->s_flags & UFS_DE_MASK) == UFS_DE_44BSD)
		de->d_u.d_44.d_type = (mode & S_IFMT) >> 12;
}

The 'mode' argument is always inode->i_mode for some inode.  Two possible
variants: put validation into ufs_set_inode_ops() and make it fail when
the type is bad (note that init_special_inode() will already scream bloody
murder in that case) or put the validation in ufs_set_de_type() - that
would be

{
        if ((UFS_SB(sb)->s_flags & UFS_DE_MASK) == UFS_DE_44BSD) {
		unsigned char v = (mode & S_IFMT) >> 12;
		switch (v) {
		case DT_REG: case DT_DIR: case DT_LNK: case DT_SOCK:
		case DT_CHR: case DT_BLK: case DT_FIFO:
			break;
		default:
			v = DT_UNKNOWN;
		}
		de->d_u.d_44.d_type = v;
	}
}

The first variant has a potential weakness - you might be unable to
unlink() junk on corrupted filesystem, but then you really don't
want to do that; it's likely to be not only thing that got corrupted
there.