Re: [PATCH v1 2/2] fs: fat: calculate FAT type based on cluster count

Heinrich Schuchardt
2023-11-08T13:39:48Z

On 11/8/23 04:12, christian.taedcke-oss@weidmueller.com wrote:
> From: Christian Taedcke <christian.taedcke@weidmueller.com>
>
> This fixes an issue where the FAT type (FAT12, FAT16) is not
> correctly detected, e.g. when the BPB field BS_FilSysType contains the
> valid value "FAT ".
>
> According to the FAT spec the field BS_FilSysType has only
> informational character and does not determine the FAT type.
>
> The logic of this code is based on the linux kernel implementation
> from the file fs/fat/inode.c function fat_fill_super().
>
> For details about FAT see http://elm-chan.org/docs/fat_e.html
>
> Signed-off-by: Christian Taedcke <christian.taedcke@weidmueller.com>
> ---
>
> fs/fat/fat.c | 58 +++++++++++++++++++++++-----------------
> include/fat.h | 6 -----
> test/image/spl_load_fs.c | 2 +-
> 3 files changed, 35 insertions(+), 31 deletions(-)
>
> diff --git a/fs/fat/fat.c b/fs/fat/fat.c
> index 8a0f4e4e6c..9179257ff2 100644
> --- a/fs/fat/fat.c
> +++ b/fs/fat/fat.c
> @@ -27,6 +27,9 @@
>
> #define FATU8ARRAY2CPU16(x) (((x)[1] << 8) + (x)[0])
>
> +/* maximum number of clusters for FAT12 */
> +#define MAX_FAT12 0xFF4
> +
> /*
> * Convert a string to lowercase. Converts at most 'len' characters,
> * 'len' may be larger than the length of 'str' if 'str' is NULL
> @@ -485,6 +488,32 @@ static __u8 mkcksum(struct nameext *nameext)
> return ret;
> }
>
> +/*
> + * Determine the FAT type
> + *
> + * Based on fat_fill_super() from the Linux kernel's fs/fat/inode.c
> + */
> +static int
> +determine_fat_bits(const boot_sector *bs)
> +{
> + if (!bs->fat_length && bs->fat32_length) {
> + return 32;
> + } else {
> + u16 fat_start = bs->reserved;
> + u32 dir_start = fat_start + bs->fats * bs->fat_length;
> + u32 rootdir_sectors = FATU8ARRAY2CPU16(bs->dir_entries) *
> + sizeof(dir_entry) /
> + FATU8ARRAY2CPU16(bs->sector_size);
> + u32 data_start = dir_start + rootdir_sectors;
> + u16 sectors = FATU8ARRAY2CPU16(bs->sectors);
> + u32 total_sectors = sectors ? sectors : bs->total_sect;
> + u32 total_clusters = (total_sectors - data_start) /
> + bs->cluster_size;
> +
> + return (total_clusters > MAX_FAT12) ? 16 : 12;
> + }
> +}
> +
> /*
> * Read boot sector and volume info from a FAT filesystem
> */
> @@ -493,7 +522,6 @@ read_bootsectandvi(boot_sector *bs, volume_info *volinfo, int *fatsize)
> {
> __u8 *block;
> volume_info *vistart;
> - int ret = 0;
>
> if (cur_dev == NULL) {
> debug("Error: no device selected\n");
> @@ -508,7 +536,8 @@ read_bootsectandvi(boot_sector *bs, volume_info *volinfo, int *fatsize)
>
> if (disk_read(0, 1, block) < 0) {
> debug("Error: reading block\n");
> - goto fail;
> + free(block);
> + return -1;

Thanks for correcting the FAT type determination.

The change above does not look wrong. But as it is an unrelated change,
please, put it into a separate patch.

> }
>
> memcpy(bs, block, sizeof(boot_sector));
> @@ -527,33 +556,15 @@ read_bootsectandvi(boot_sector *bs, volume_info *volinfo, int *fatsize)
> bs->info_sector = FAT2CPU16(bs->info_sector);
> bs->backup_boot = FAT2CPU16(bs->backup_boot);
> vistart = (volume_info *)(block + sizeof(boot_sector));
> - *fatsize = 32;
> } else {
> vistart = (volume_info *)&(bs->fat32_length);
> - *fatsize = 0;
> }
> memcpy(volinfo, vistart, sizeof(volume_info));
>
> - if (*fatsize == 32) {
> - if (strncmp(FAT32_SIGN, vistart->fs_type, SIGNLEN) == 0)
> - goto exit;
> - } else {
> - if (strncmp(FAT12_SIGN, vistart->fs_type, SIGNLEN) == 0) {
> - *fatsize = 12;
> - goto exit;
> - }
> - if (strncmp(FAT16_SIGN, vistart->fs_type, SIGNLEN) == 0) {
> - *fatsize = 16;
> - goto exit;
> - }
> - }
> + *fatsize = determine_fat_bits(bs);
>
> - debug("Error: broken fs_type sign\n");
> -fail:
> - ret = -1;
> -exit:
> free(block);
> - return ret;
> + return 0;
> }
>
> static int get_fs_info(fsdata *mydata)
> @@ -1158,9 +1169,8 @@ int file_fat_detectfs(void)
>
> memcpy(vol_label, volinfo.volume_label, 11);
> vol_label[11] = '\0';
> - volinfo.fs_type[5] = '\0';
>
> - printf("Filesystem: %s \"%s\"\n", volinfo.fs_type, vol_label);
> + printf("Filesystem: FAT%d \"%s\"\n", fatsize, vol_label);
>
> return 0;
> }
> diff --git a/include/fat.h b/include/fat.h
> index a9756fb4cd..3dce99a23c 100644
> --- a/include/fat.h
> +++ b/include/fat.h
> @@ -34,12 +34,6 @@ struct disk_partition;
> /* Maximum number of entry for long file name according to spec */
> #define MAX_LFN_SLOT 20
>
> -/* Filesystem identifiers */
> -#define FAT12_SIGN "FAT12 "
> -#define FAT16_SIGN "FAT16 "
> -#define FAT32_SIGN "FAT32 "
> -#define SIGNLEN 8
> -
> /* File attributes */
> #define ATTR_RO 1
> #define ATTR_HIDDEN 2
> diff --git a/test/image/spl_load_fs.c b/test/image/spl_load_fs.c
> index 297ab08a82..90e640b5de 100644
> --- a/test/image/spl_load_fs.c
> +++ b/test/image/spl_load_fs.c
> @@ -220,7 +220,7 @@ static size_t create_fat(void *dst, size_t size, const char *filename,
> bs->root_cluster = cpu_to_le32(root_sector);
>
> vi->ext_boot_sign = 0x29;
> - memcpy(vi->fs_type, FAT32_SIGN, sizeof(vi->fs_type));
> + memcpy(vi->fs_type, "FAT32 ", sizeof(vi->fs_type));

This change is not described in the commit message. Putting it into a
separate patch would be preferable.

Best regards

Heinrich

>
> memcpy(dst + 0x1fe, "\x55\xAA", 2);
>