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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Mon, 10 Dec 2018 12:09:31 -0800
From:   Joe Perches <joe@...ches.com>
To:     Carmeli Tamir <carmeli.tamir@...il.com>,
        hirofumi@...l.parknet.co.jp, jthumshirn@...e.de,
        sergey.senozhatsky@...il.com, akpm@...ux-foundation.org,
        axboe@...nel.dk, martin.petersen@...cle.com, bvanassche@....org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH 2/2] fat: New macros to determine the FAT variant (32,
 16 or 12)

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.