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

[Joe Perches <joe@perches.com>]

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.