* [PATCH v3 1/3] fat: Removed FAT_FIRST_ENT macro
2018-12-16 20:03 [PATCH v3 0/3] fat: Added functions to determine the FAT variant (12/16/32bit) Carmeli Tamir
@ 2018-12-16 20:03 ` Carmeli Tamir
2018-12-16 20:03 ` [PATCH v3 2/3] fat: Moved MAX_FAT to fat.h and changed it to inline function Carmeli Tamir
` (2 subsequent siblings)
3 siblings, 0 replies; 6+ messages in thread
From: Carmeli Tamir @ 2018-12-16 20:03 UTC (permalink / raw)
To: carmeli.tamir, hirofumi, linux-kernel, jthumshirn,
sergey.senozhatsky, akpm, bvanassche, axboe, martin.petersen,
sfr
The comment edited in this patch was the only reference to the
FAT_FIRST_ENT macro, which is not used anymore. Moreover, the
commented line of code does not compile with the current code.
Since the FAT_FIRST_ENT macro checks the FAT variant in a way that the
patch series changes, I removed it, and instead wrote a clear explanation
of what was checked.
I verified that the changed comment is correct according to Microsoft FAT
spec, search for "BPB_Media" in the following references:
1. Microsoft FAT specification 2005
(http://read.pudn.com/downloads77/ebook/294884/FAT32%20Spec%20%28SDA%20Contribution%29.pdf).
Search for 'volume label'.
2. Microsoft Extensible Firmware Initiative, FAT32 File System Specification
(https://staff.washington.edu/dittrich/misc/fatgen103.pdf).
Search for 'volume label'.
Signed-off-by: Carmeli Tamir <carmeli.tamir@gmail.com>
---
fs/fat/inode.c | 12 ++++++++----
include/uapi/linux/msdos_fs.h | 3 ---
2 files changed, 8 insertions(+), 7 deletions(-)
diff --git a/fs/fat/inode.c b/fs/fat/inode.c
index e981e9d..708de6d 100644
--- a/fs/fat/inode.c
+++ b/fs/fat/inode.c
@@ -1804,11 +1804,15 @@ int fat_fill_super(struct super_block *sb, void *data, int silent, int isvfat,
fat_ent_access_init(sb);
/*
- * The low byte of FAT's first entry must have same value with
- * media-field. But in real world, too many devices is
- * writing wrong value. So, removed that validity check.
+ * The low byte of the first FAT entry must have the same value as
+ * the media field of the boot sector. But in real world, too many
+ * devices are writing wrong values. So, removed that validity check.
*
- * if (FAT_FIRST_ENT(sb, media) != first)
+ * The removed check compared the first FAT entry to a value dependent
+ * on the media field like this:
+ * == (0x0F00 | media), for FAT12
+ * == (0XFF00 | media), for FAT16
+ * == (0x0FFFFF | media), for FAT32
*/
error = -EINVAL;
diff --git a/include/uapi/linux/msdos_fs.h b/include/uapi/linux/msdos_fs.h
index 1216e6c..833c707 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
--
2.7.4
^ permalink raw reply related [flat|nested] 6+ messages in thread* [PATCH v3 2/3] fat: Moved MAX_FAT to fat.h and changed it to inline function
2018-12-16 20:03 [PATCH v3 0/3] fat: Added functions to determine the FAT variant (12/16/32bit) Carmeli Tamir
2018-12-16 20:03 ` [PATCH v3 1/3] fat: Removed FAT_FIRST_ENT macro Carmeli Tamir
@ 2018-12-16 20:03 ` Carmeli Tamir
2018-12-16 20:04 ` [PATCH v3 3/3] fat: New inline functions to determine the FAT variant (32, 16 or 12) Carmeli Tamir
2018-12-24 12:04 ` [PATCH v3 0/3] fat: Added functions to determine the FAT variant (12/16/32bit) OGAWA Hirofumi
3 siblings, 0 replies; 6+ messages in thread
From: Carmeli Tamir @ 2018-12-16 20:03 UTC (permalink / raw)
To: carmeli.tamir, hirofumi, linux-kernel, jthumshirn,
sergey.senozhatsky, akpm, bvanassche, axboe, martin.petersen,
sfr
MAX_FAT is useless in msdos_fs.h, since it uses the MSDOS_SB function
that is defined in fat.h. So really, this macro can be only called
from code that already includes fat.h.
Hence, this patch moves it to fat.h, right after MSDOS_SB is defined.
I also changed it to an inline function in order to save the double call
to MSDOS_SB. This was suggested by joe@perches.com in the previous
version.
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 | 9 +++++++++
fs/fat/inode.c | 2 +-
include/uapi/linux/msdos_fs.h | 2 --
3 files changed, 10 insertions(+), 3 deletions(-)
diff --git a/fs/fat/fat.h b/fs/fat/fat.h
index 4e1b2f6..979bb11 100644
--- a/fs/fat/fat.h
+++ b/fs/fat/fat.h
@@ -142,6 +142,15 @@ static inline struct msdos_sb_info *MSDOS_SB(struct super_block *sb)
return sb->s_fs_info;
}
+/* Maximum number of clusters */
+static inline u32 max_fat(struct super_block *sb)
+{
+ struct msdos_sb_info *sbi = MSDOS_SB(sb);
+
+ return sbi->fat_bits == 32 ? MAX_FAT32 :
+ sbi->fat_bits == 16 ? MAX_FAT16 : MAX_FAT12;
+}
+
static inline struct msdos_inode_info *MSDOS_I(struct inode *inode)
{
return container_of(inode, struct msdos_inode_info, vfs_inode);
diff --git a/fs/fat/inode.c b/fs/fat/inode.c
index 708de6d..7ede9db 100644
--- a/fs/fat/inode.c
+++ b/fs/fat/inode.c
@@ -1782,7 +1782,7 @@ int fat_fill_super(struct super_block *sb, void *data, int silent, int isvfat,
/* check that FAT table does not overflow */
fat_clusters = calc_fat_clusters(sb);
total_clusters = min(total_clusters, fat_clusters - FAT_START_ENT);
- if (total_clusters > MAX_FAT(sb)) {
+ if (total_clusters > max_fat(sb)) {
if (!silent)
fat_msg(sb, KERN_ERR, "count of clusters too big (%u)",
total_clusters);
diff --git a/include/uapi/linux/msdos_fs.h b/include/uapi/linux/msdos_fs.h
index 833c707..a577389 100644
--- a/include/uapi/linux/msdos_fs.h
+++ b/include/uapi/linux/msdos_fs.h
@@ -65,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
^ permalink raw reply related [flat|nested] 6+ messages in thread* [PATCH v3 3/3] fat: New inline functions to determine the FAT variant (32, 16 or 12)
2018-12-16 20:03 [PATCH v3 0/3] fat: Added functions to determine the FAT variant (12/16/32bit) Carmeli Tamir
2018-12-16 20:03 ` [PATCH v3 1/3] fat: Removed FAT_FIRST_ENT macro Carmeli Tamir
2018-12-16 20:03 ` [PATCH v3 2/3] fat: Moved MAX_FAT to fat.h and changed it to inline function Carmeli Tamir
@ 2018-12-16 20:04 ` Carmeli Tamir
2018-12-24 12:04 ` [PATCH v3 0/3] fat: Added functions to determine the FAT variant (12/16/32bit) OGAWA Hirofumi
3 siblings, 0 replies; 6+ messages in thread
From: Carmeli Tamir @ 2018-12-16 20:04 UTC (permalink / raw)
To: carmeli.tamir, hirofumi, linux-kernel, jthumshirn,
sergey.senozhatsky, akpm, bvanassche, axboe, martin.petersen,
sfr
This patch introduces 3 new inline functions - 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 | 25 ++++++++++++++++++++++---
fs/fat/fatent.c | 16 +++++++---------
fs/fat/inode.c | 12 ++++++------
fs/fat/misc.c | 2 +-
6 files changed, 39 insertions(+), 22 deletions(-)
diff --git a/fs/fat/cache.c b/fs/fat/cache.c
index 78d501c..738e427 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..20acaea 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 979bb11..922a0c6 100644
--- a/fs/fat/fat.h
+++ b/fs/fat/fat.h
@@ -142,13 +142,32 @@ static inline struct msdos_sb_info *MSDOS_SB(struct super_block *sb)
return sb->s_fs_info;
}
+/*
+ * Functions that determine the variant of the FAT file system (i.e.,
+ * whether this is FAT12, FAT16 or FAT32.
+ */
+static inline bool is_fat12(const struct msdos_sb_info *sbi)
+{
+ return sbi->fat_bits == 12;
+}
+
+static inline bool is_fat16(const struct msdos_sb_info *sbi)
+{
+ return sbi->fat_bits == 16;
+}
+
+static inline bool is_fat32(const struct msdos_sb_info *sbi)
+{
+ return sbi->fat_bits == 32;
+}
+
/* Maximum number of clusters */
static inline u32 max_fat(struct super_block *sb)
{
struct msdos_sb_info *sbi = MSDOS_SB(sb);
- return sbi->fat_bits == 32 ? MAX_FAT32 :
- sbi->fat_bits == 16 ? MAX_FAT16 : MAX_FAT12;
+ return is_fat32(sbi) ? MAX_FAT32 :
+ is_fat16(sbi) ? MAX_FAT16 : MAX_FAT12;
}
static inline struct msdos_inode_info *MSDOS_I(struct inode *inode)
@@ -266,7 +285,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..495edea 100644
--- a/fs/fat/fatent.c
+++ b/fs/fat/fatent.c
@@ -290,19 +290,17 @@ 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, %u bits", sbi->fat_bits);
}
}
@@ -310,7 +308,7 @@ 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 +325,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 7ede9db..583dc6f 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..4fc950b 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
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH v3 0/3] fat: Added functions to determine the FAT variant (12/16/32bit)
2018-12-16 20:03 [PATCH v3 0/3] fat: Added functions to determine the FAT variant (12/16/32bit) Carmeli Tamir
` (2 preceding siblings ...)
2018-12-16 20:04 ` [PATCH v3 3/3] fat: New inline functions to determine the FAT variant (32, 16 or 12) Carmeli Tamir
@ 2018-12-24 12:04 ` OGAWA Hirofumi
2018-12-25 9:48 ` Sergey Senozhatsky
3 siblings, 1 reply; 6+ messages in thread
From: OGAWA Hirofumi @ 2018-12-24 12:04 UTC (permalink / raw)
To: Carmeli Tamir
Cc: linux-kernel, jthumshirn, sergey.senozhatsky, akpm, bvanassche,
axboe, martin.petersen, sfr
Carmeli Tamir <carmeli.tamir@gmail.com> writes:
> 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
> inline functions - IS_FAT12, IS_FAT16 and IS_FAT16.
>
> The introduction of these simple inline functions 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.
>
> In addition, minor cleanups around code that checks for the FAT variant,
> and fixed comments from v1 and v2.
>
> Carmeli Tamir (3):
> Removed fat_first_ent
> Moved and inlined MAX_FAT
> IS_FAT functions
>
> fs/fat/cache.c | 2 +-
> fs/fat/dir.c | 4 ++--
> fs/fat/fat.h | 30 +++++++++++++++++++++++++++++-
> fs/fat/fatent.c | 16 +++++++---------
> fs/fat/inode.c | 26 +++++++++++++++-----------
> fs/fat/misc.c | 2 +-
> include/uapi/linux/msdos_fs.h | 5 -----
> 7 files changed, 55 insertions(+), 30 deletions(-)
FWIW,
Acked-by: OGAWA Hirofumi <hirofumi@mail.parknet.co.jp>
for all of this patchset.
Thanks.
--
OGAWA Hirofumi <hirofumi@mail.parknet.co.jp>
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH v3 0/3] fat: Added functions to determine the FAT variant (12/16/32bit)
2018-12-24 12:04 ` [PATCH v3 0/3] fat: Added functions to determine the FAT variant (12/16/32bit) OGAWA Hirofumi
@ 2018-12-25 9:48 ` Sergey Senozhatsky
0 siblings, 0 replies; 6+ messages in thread
From: Sergey Senozhatsky @ 2018-12-25 9:48 UTC (permalink / raw)
To: OGAWA Hirofumi
Cc: Carmeli Tamir, linux-kernel, jthumshirn, sergey.senozhatsky,
akpm, bvanassche, axboe, martin.petersen, sfr
On (12/24/18 21:04), OGAWA Hirofumi wrote:
> FWIW,
>
> Acked-by: OGAWA Hirofumi <hirofumi@mail.parknet.co.jp>
>
> for all of this patchset.
Looks good to me.
FWIW,
Reviewed-by: Sergey Senozhatsky <sergey.senozhatsky@gmail.com>
-ss
^ permalink raw reply [flat|nested] 6+ messages in thread