[PATCH 0/2] fat: Added macros to determine the FAT FS variant (12/16/32bit)

[PATCH 0/2] fat: Added macros to determine the FAT FS variant (12/16/32bit)

[Carmeli Tamir]

Along the FAT FS code, the FAT variant (whether this is FAT12, FAT16 or FAT32) is
determined by checking the fat_bits field of struct msdos_sb_info. 
This is somewhat error prone as it forces the usage of magics (12, 16, 32)
multiple times in the code.

This series replaces the places in which the variant is checked with three simple
macros - IS_FAT12, IS_FAT16 and IS_FAT16.

The introduction of these simple macros makes a clearer API for determining the variant,
rather than searching the code for some field in a struct, and therefore
increases the code's maintainability and readability.

Carmeli Tamir (2):
  fat: Moved macros that won't work without fat.h
  fat: New macros to determine the FAT variant (32, 16 or 12)

 fs/fat/cache.c                |  2 +-
 fs/fat/dir.c                  |  4 ++--
 fs/fat/fat.h                  | 28 ++++++++++++++++++++++------
 fs/fat/fatent.c               | 17 +++++++----------
 fs/fat/inode.c                | 12 ++++++------
 fs/fat/misc.c                 |  2 +-
 include/uapi/linux/msdos_fs.h |  5 -----
 7 files changed, 39 insertions(+), 31 deletions(-)

-- 
2.7.4

[PATCH 1/2] fat: Moved macros that won't work without fat.h

[Carmeli Tamir]

Both FAT_FIRST_ENT and MAX_FAT are useless in msdos_fs.h, since they need the
MSDOS_SB function that is defined in fat.h. So really, both can be only called
from code that already includes fat.h.

Hence, this patch moves them to fat.h, right after MSDOS_SB is defined.
This patch is required for the next in the series, in which the variant (whether
this is FAT12, FAT16 or FAT32) checks are replaced with new macros.


Signed-off-by: Carmeli Tamir <carmeli.tamir@gmail.com>
---
 fs/fat/fat.h                  | 22 +++++++++++++++++-----
 include/uapi/linux/msdos_fs.h |  5 -----
 2 files changed, 17 insertions(+), 10 deletions(-)

diff --git a/fs/fat/fat.h b/fs/fat/fat.h
index 4e1b2f6..e06fdd3 100644
--- a/fs/fat/fat.h
+++ b/fs/fat/fat.h
@@ -106,6 +106,23 @@ struct msdos_sb_info {
 
 #define FAT_CACHE_VALID	0	/* special case for valid cache */
 
+static inline struct msdos_sb_info *MSDOS_SB(struct super_block *sb)
+{
+	return sb->s_fs_info;
+}
+
+/*
+ * Definitions that depend on the variant of the FAT file system (i.e., whether
+ * this is FAT12, FAT16 or FAT32.
+ */
+
+#define FAT_FIRST_ENT(s, x)     ((MSDOS_SB(s)->fat_bits == 32 ? 0x0FFFFF00 : \
+	MSDOS_SB(s)->fat_bits == 16 ? 0xFF00 : 0xF00) | (x))
+
+/* maximum number of clusters */
+#define MAX_FAT(s)      (MSDOS_SB(s)->fat_bits == 32 ? MAX_FAT32 : \
+	MSDOS_SB(s)->fat_bits == 16 ? MAX_FAT16 : MAX_FAT12)
+
 /*
  * MS-DOS file system inode data in memory
  */
@@ -137,11 +154,6 @@ struct fat_slot_info {
 	struct buffer_head *bh;
 };
 
-static inline struct msdos_sb_info *MSDOS_SB(struct super_block *sb)
-{
-	return sb->s_fs_info;
-}
-
 static inline struct msdos_inode_info *MSDOS_I(struct inode *inode)
 {
 	return container_of(inode, struct msdos_inode_info, vfs_inode);
diff --git a/include/uapi/linux/msdos_fs.h b/include/uapi/linux/msdos_fs.h
index 1216e6c..a577389 100644
--- a/include/uapi/linux/msdos_fs.h
+++ b/include/uapi/linux/msdos_fs.h
@@ -58,9 +58,6 @@
 #define MSDOS_DOT	".          "	/* ".", padded to MSDOS_NAME chars */
 #define MSDOS_DOTDOT	"..         "	/* "..", padded to MSDOS_NAME chars */
 
-#define FAT_FIRST_ENT(s, x)	((MSDOS_SB(s)->fat_bits == 32 ? 0x0FFFFF00 : \
-	MSDOS_SB(s)->fat_bits == 16 ? 0xFF00 : 0xF00) | (x))
-
 /* start of data cluster's entry (number of reserved clusters) */
 #define FAT_START_ENT	2
 
@@ -68,8 +65,6 @@
 #define MAX_FAT12	0xFF4
 #define MAX_FAT16	0xFFF4
 #define MAX_FAT32	0x0FFFFFF6
-#define MAX_FAT(s)	(MSDOS_SB(s)->fat_bits == 32 ? MAX_FAT32 : \
-	MSDOS_SB(s)->fat_bits == 16 ? MAX_FAT16 : MAX_FAT12)
 
 /* bad cluster mark */
 #define BAD_FAT12	0xFF7
-- 
2.7.4

[PATCH 2/2] fat: New macros to determine the FAT variant (32, 16 or 12)

[Carmeli Tamir]

This patch introduces 3 new macros - IS_FAT12, IS_FAT16 and IS_FAT32,
and replaces every occurrence in the code in which the FS variant (whether
this is FAT12, FAT16 or FAT32) was previously checked using
msdos_sb_info->fat_bits.

Signed-off-by: Carmeli Tamir <carmeli.tamir@gmail.com>
---
 fs/fat/cache.c  |  2 +-
 fs/fat/dir.c    |  4 ++--
 fs/fat/fat.h    | 14 +++++++++-----
 fs/fat/fatent.c | 17 +++++++----------
 fs/fat/inode.c  | 12 ++++++------
 fs/fat/misc.c   |  2 +-
 6 files changed, 26 insertions(+), 25 deletions(-)

diff --git a/fs/fat/cache.c b/fs/fat/cache.c
index 78d501c..99962b3 100644
--- a/fs/fat/cache.c
+++ b/fs/fat/cache.c
@@ -363,7 +363,7 @@ int fat_bmap(struct inode *inode, sector_t sector, sector_t *phys,
 
 	*phys = 0;
 	*mapped_blocks = 0;
-	if ((sbi->fat_bits != 32) && (inode->i_ino == MSDOS_ROOT_INO)) {
+	if ((!IS_FAT32(sbi)) && (inode->i_ino == MSDOS_ROOT_INO)) {
 		if (sector < (sbi->dir_entries >> sbi->dir_per_block_bits)) {
 			*phys = sector + sbi->dir_start;
 			*mapped_blocks = 1;
diff --git a/fs/fat/dir.c b/fs/fat/dir.c
index c8366cb..9acbaed 100644
--- a/fs/fat/dir.c
+++ b/fs/fat/dir.c
@@ -57,7 +57,7 @@ static inline void fat_dir_readahead(struct inode *dir, sector_t iblock,
 	if ((iblock & (sbi->sec_per_clus - 1)) || sbi->sec_per_clus == 1)
 		return;
 	/* root dir of FAT12/FAT16 */
-	if ((sbi->fat_bits != 32) && (dir->i_ino == MSDOS_ROOT_INO))
+	if ((!IS_FAT32(sbi)) && (dir->i_ino == MSDOS_ROOT_INO))
 		return;
 
 	bh = sb_find_get_block(sb, phys);
@@ -1313,7 +1313,7 @@ int fat_add_entries(struct inode *dir, void *slots, int nr_slots,
 		}
 	}
 	if (dir->i_ino == MSDOS_ROOT_INO) {
-		if (sbi->fat_bits != 32)
+		if (!IS_FAT32(sbi))
 			goto error;
 	} else if (MSDOS_I(dir)->i_start == 0) {
 		fat_msg(sb, KERN_ERR, "Corrupted directory (i_pos %lld)",
diff --git a/fs/fat/fat.h b/fs/fat/fat.h
index e06fdd3..de7c4c8 100644
--- a/fs/fat/fat.h
+++ b/fs/fat/fat.h
@@ -116,12 +116,16 @@ static inline struct msdos_sb_info *MSDOS_SB(struct super_block *sb)
  * this is FAT12, FAT16 or FAT32.
  */
 
-#define FAT_FIRST_ENT(s, x)     ((MSDOS_SB(s)->fat_bits == 32 ? 0x0FFFFF00 : \
-	MSDOS_SB(s)->fat_bits == 16 ? 0xFF00 : 0xF00) | (x))
+#define IS_FAT12(sbi) (sbi->fat_bits == 12)
+#define IS_FAT16(sbi) (sbi->fat_bits == 16)
+#define IS_FAT32(sbi) (sbi->fat_bits == 32)
+
+#define FAT_FIRST_ENT(s, x)     ((IS_FAT32(MSDOS_SB(s)) ? 0x0FFFFF00 : \
+	IS_FAT16(MSDOS_SB(s)) ? 0xFF00 : 0xF00) | (x))
 
 /* maximum number of clusters */
-#define MAX_FAT(s)      (MSDOS_SB(s)->fat_bits == 32 ? MAX_FAT32 : \
-	MSDOS_SB(s)->fat_bits == 16 ? MAX_FAT16 : MAX_FAT12)
+#define MAX_FAT(s)      (IS_FAT32(MSDOS_SB(s)) ? MAX_FAT32 : \
+	IS_FAT16(MSDOS_SB(s)) ? MAX_FAT16 : MAX_FAT12)
 
 /*
  * MS-DOS file system inode data in memory
@@ -269,7 +273,7 @@ static inline int fat_get_start(const struct msdos_sb_info *sbi,
 				const struct msdos_dir_entry *de)
 {
 	int cluster = le16_to_cpu(de->start);
-	if (sbi->fat_bits == 32)
+	if (IS_FAT32(sbi))
 		cluster |= (le16_to_cpu(de->starthi) << 16);
 	return cluster;
 }
diff --git a/fs/fat/fatent.c b/fs/fat/fatent.c
index f58c0ca..da0c922 100644
--- a/fs/fat/fatent.c
+++ b/fs/fat/fatent.c
@@ -290,27 +290,24 @@ void fat_ent_access_init(struct super_block *sb)
 
 	mutex_init(&sbi->fat_lock);
 
-	switch (sbi->fat_bits) {
-	case 32:
+	if (IS_FAT32(sbi)) {
 		sbi->fatent_shift = 2;
 		sbi->fatent_ops = &fat32_ops;
-		break;
-	case 16:
+	} else if (IS_FAT16(sbi)) {
 		sbi->fatent_shift = 1;
 		sbi->fatent_ops = &fat16_ops;
-		break;
-	case 12:
+	} else if (IS_FAT12(sbi)) {
 		sbi->fatent_shift = -1;
 		sbi->fatent_ops = &fat12_ops;
-		break;
-	}
+	} else
+		fat_fs_error(sb, "invalid FAT variant");
 }
 
 static void mark_fsinfo_dirty(struct super_block *sb)
 {
 	struct msdos_sb_info *sbi = MSDOS_SB(sb);
 
-	if (sb_rdonly(sb) || sbi->fat_bits != 32)
+	if (sb_rdonly(sb) || !IS_FAT32(sbi))
 		return;
 
 	__mark_inode_dirty(sbi->fsinfo_inode, I_DIRTY_SYNC);
@@ -327,7 +324,7 @@ static inline int fat_ent_update_ptr(struct super_block *sb,
 	/* Is this fatent's blocks including this entry? */
 	if (!fatent->nr_bhs || bhs[0]->b_blocknr != blocknr)
 		return 0;
-	if (sbi->fat_bits == 12) {
+	if (IS_FAT12(sbi)) {
 		if ((offset + 1) < sb->s_blocksize) {
 			/* This entry is on bhs[0]. */
 			if (fatent->nr_bhs == 2) {
diff --git a/fs/fat/inode.c b/fs/fat/inode.c
index e981e9d..68834fb 100644
--- a/fs/fat/inode.c
+++ b/fs/fat/inode.c
@@ -686,7 +686,7 @@ static void fat_set_state(struct super_block *sb,
 
 	b = (struct fat_boot_sector *) bh->b_data;
 
-	if (sbi->fat_bits == 32) {
+	if (IS_FAT32(sbi)) {
 		if (set)
 			b->fat32.state |= FAT_STATE_DIRTY;
 		else
@@ -1397,7 +1397,7 @@ static int fat_read_root(struct inode *inode)
 	inode->i_mode = fat_make_mode(sbi, ATTR_DIR, S_IRWXUGO);
 	inode->i_op = sbi->dir_ops;
 	inode->i_fop = &fat_dir_operations;
-	if (sbi->fat_bits == 32) {
+	if (IS_FAT32(sbi)) {
 		MSDOS_I(inode)->i_start = sbi->root_cluster;
 		error = fat_calc_dir_size(inode);
 		if (error < 0)
@@ -1424,7 +1424,7 @@ static unsigned long calc_fat_clusters(struct super_block *sb)
 	struct msdos_sb_info *sbi = MSDOS_SB(sb);
 
 	/* Divide first to avoid overflow */
-	if (sbi->fat_bits != 12) {
+	if (!IS_FAT12(sbi)) {
 		unsigned long ent_per_sec = sb->s_blocksize * 8 / sbi->fat_bits;
 		return ent_per_sec * sbi->fat_length;
 	}
@@ -1744,7 +1744,7 @@ int fat_fill_super(struct super_block *sb, void *data, int silent, int isvfat,
 	}
 
 	/* interpret volume ID as a little endian 32 bit integer */
-	if (sbi->fat_bits == 32)
+	if (IS_FAT32(sbi))
 		sbi->vol_id = bpb.fat32_vol_id;
 	else /* fat 16 or 12 */
 		sbi->vol_id = bpb.fat16_vol_id;
@@ -1770,11 +1770,11 @@ int fat_fill_super(struct super_block *sb, void *data, int silent, int isvfat,
 
 	total_clusters = (total_sectors - sbi->data_start) / sbi->sec_per_clus;
 
-	if (sbi->fat_bits != 32)
+	if (!IS_FAT32(sbi))
 		sbi->fat_bits = (total_clusters > MAX_FAT12) ? 16 : 12;
 
 	/* some OSes set FAT_STATE_DIRTY and clean it on unmount. */
-	if (sbi->fat_bits == 32)
+	if (IS_FAT32(sbi))
 		sbi->dirty = bpb.fat32_state & FAT_STATE_DIRTY;
 	else /* fat 16 or 12 */
 		sbi->dirty = bpb.fat16_state & FAT_STATE_DIRTY;
diff --git a/fs/fat/misc.c b/fs/fat/misc.c
index fce0a76..5368c6a 100644
--- a/fs/fat/misc.c
+++ b/fs/fat/misc.c
@@ -64,7 +64,7 @@ int fat_clusters_flush(struct super_block *sb)
 	struct buffer_head *bh;
 	struct fat_boot_fsinfo *fsinfo;
 
-	if (sbi->fat_bits != 32)
+	if (!IS_FAT32(sbi))
 		return 0;
 
 	bh = sb_bread(sb, sbi->fsinfo_sector);
-- 
2.7.4

Re: [PATCH 2/2] fat: New macros to determine the FAT variant (32, 16 or 12)

[Joe Perches]

On Mon, 2018-12-10 at 14:41 -0500, Carmeli Tamir wrote:
> This patch introduces 3 new macros - IS_FAT12, IS_FAT16 and IS_FAT32,
> and replaces every occurrence in the code in which the FS variant (whether
> this is FAT12, FAT16 or FAT32) was previously checked using
> msdos_sb_info->fat_bits.

Overall a nice cleanup and a couple style suggestions:

> diff --git a/fs/fat/cache.c b/fs/fat/cache.c
> index 78d501c..99962b3 100644
> --- a/fs/fat/cache.c
> +++ b/fs/fat/cache.c
> @@ -363,7 +363,7 @@ int fat_bmap(struct inode *inode, sector_t sector, sector_t *phys,
>  
>  	*phys = 0;
>  	*mapped_blocks = 0;
> -	if ((sbi->fat_bits != 32) && (inode->i_ino == MSDOS_ROOT_INO)) {
> +	if ((!IS_FAT32(sbi)) && (inode->i_ino == MSDOS_ROOT_INO)) {

Perhaps nicer without the parens around !IS_FAT32(sbi)

[]

> diff --git a/fs/fat/fat.h b/fs/fat/fat.h
[]
> @@ -116,12 +116,16 @@ static inline struct msdos_sb_info *MSDOS_SB(struct super_block *sb)
>   * this is FAT12, FAT16 or FAT32.
>   */
>  
> -#define FAT_FIRST_ENT(s, x)     ((MSDOS_SB(s)->fat_bits == 32 ? 0x0FFFFF00 : \
> -	MSDOS_SB(s)->fat_bits == 16 ? 0xFF00 : 0xF00) | (x))
> +#define IS_FAT12(sbi) (sbi->fat_bits == 12)
> +#define IS_FAT16(sbi) (sbi->fat_bits == 16)
> +#define IS_FAT32(sbi) (sbi->fat_bits == 32)

sbi should be parenthesized or perhaps better these should be
static inline bool functions

> +
> +#define FAT_FIRST_ENT(s, x)     ((IS_FAT32(MSDOS_SB(s)) ? 0x0FFFFF00 : \
> +	IS_FAT16(MSDOS_SB(s)) ? 0xFF00 : 0xF00) | (x))

This is probably nicer to use a statement expression macro
so MSDOS_SB(s) is only evaluated once or convert this to a
static inline, but it may even better to remove it altogether
instead as it seems unused anywhere.

>  
>  /* maximum number of clusters */
> -#define MAX_FAT(s)      (MSDOS_SB(s)->fat_bits == 32 ? MAX_FAT32 : \
> -	MSDOS_SB(s)->fat_bits == 16 ? MAX_FAT16 : MAX_FAT12)
> +#define MAX_FAT(s)      (IS_FAT32(MSDOS_SB(s)) ? MAX_FAT32 : \
> +	IS_FAT16(MSDOS_SB(s)) ? MAX_FAT16 : MAX_FAT12)

Used once, so perhaps better inlined there.

Re: [PATCH 2/2] fat: New macros to determine the FAT variant (32, 16 or 12)

[OGAWA Hirofumi]

Joe Perches <joe@perches.com> writes:

>>  
>> -#define FAT_FIRST_ENT(s, x)     ((MSDOS_SB(s)->fat_bits == 32 ? 0x0FFFFF00 : \
>> -	MSDOS_SB(s)->fat_bits == 16 ? 0xFF00 : 0xF00) | (x))
>> +#define IS_FAT12(sbi) (sbi->fat_bits == 12)
>> +#define IS_FAT16(sbi) (sbi->fat_bits == 16)
>> +#define IS_FAT32(sbi) (sbi->fat_bits == 32)
>
> sbi should be parenthesized or perhaps better these should be
> static inline bool functions

Right, rather this is the bug (not hit yet though) that should be fixed.

Thanks.
-- 
OGAWA Hirofumi <hirofumi@mail.parknet.co.jp>

Re: [PATCH 2/2] fat: New macros to determine the FAT variant (32, 16 or 12)

[Tamir Carmeli]

Thanks a lot for the comments, I'll work on V2 to fix them.
I just want to make sure, is there a reason why I shouldn't delete
FAT_FIRST_ENT, as Joe Perches commented?

On Fri, Dec 14, 2018 at 5:16 AM OGAWA Hirofumi
<hirofumi@mail.parknet.co.jp> wrote:
>
> Joe Perches <joe@perches.com> writes:
>
> >>
> >> -#define FAT_FIRST_ENT(s, x)     ((MSDOS_SB(s)->fat_bits == 32 ? 0x0FFFFF00 : \
> >> -    MSDOS_SB(s)->fat_bits == 16 ? 0xFF00 : 0xF00) | (x))
> >> +#define IS_FAT12(sbi) (sbi->fat_bits == 12)
> >> +#define IS_FAT16(sbi) (sbi->fat_bits == 16)
> >> +#define IS_FAT32(sbi) (sbi->fat_bits == 32)
> >
> > sbi should be parenthesized or perhaps better these should be
> > static inline bool functions
>
> Right, rather this is the bug (not hit yet though) that should be fixed.
>
> Thanks.
> --
> OGAWA Hirofumi <hirofumi@mail.parknet.co.jp>

Re: [PATCH 2/2] fat: New macros to determine the FAT variant (32, 16 or 12)

[OGAWA Hirofumi]

Tamir Carmeli <carmeli.tamir@gmail.com> writes:

> I just want to make sure, is there a reason why I shouldn't delete
> FAT_FIRST_ENT, as Joe Perches commented?

FAT_FIRST_ENT() was used to check if fat spec compliance. But in real
world, there are too many implementations that didn't follow spec.

Well, so, now FAT_FIRST_ENT() is pointed only from comment. The reason
is only for this comment.

Thanks.
-- 
OGAWA Hirofumi <hirofumi@mail.parknet.co.jp>