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.
