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.
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.