[01/17] fs: fat: extend get_fs_info() for write use

Message ID 20180720025723.6736-2-takahiro.akashi@linaro.org
State New
Headers show
Series
  • fs: fat: extend FAT write operations
Related show

Commit Message

AKASHI Takahiro July 20, 2018, 2:57 a.m.
get_fs_info() was introduced in major re-work of read operation by Rob.
We want to reuse this function in write operation by extending it with
additional members in fsdata structure.

Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
---
 fs/fat/fat.c  | 3 +++
 include/fat.h | 2 ++
 2 files changed, 5 insertions(+)

Comments

Heinrich Schuchardt July 20, 2018, 6:06 p.m. | #1
On 07/20/2018 04:57 AM, AKASHI Takahiro wrote:
> get_fs_info() was introduced in major re-work of read operation by Rob.
> We want to reuse this function in write operation by extending it with
> additional members in fsdata structure.
> 
> Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
> ---
>  fs/fat/fat.c  | 3 +++
>  include/fat.h | 2 ++
>  2 files changed, 5 insertions(+)
> 
> diff --git a/fs/fat/fat.c b/fs/fat/fat.c
> index 4efe8a3eda..b48f48a751 100644
> --- a/fs/fat/fat.c
> +++ b/fs/fat/fat.c
> @@ -556,6 +556,9 @@ static int get_fs_info(fsdata *mydata)
>  		return ret;
>  	}
>  
> +	mydata->bs_total_sect = bs.total_sect;
> +	mydata->bs_fats = bs.fats;
> +
>  	if (mydata->fatsize == 32) {
>  		mydata->fatlength = bs.fat32_length;
>  	} else {
> diff --git a/include/fat.h b/include/fat.h
> index 09e1423685..0c88b59a4a 100644
> --- a/include/fat.h
> +++ b/include/fat.h
> @@ -173,6 +173,8 @@ typedef struct {
>  	int	fatbufnum;	/* Used by get_fatent, init to -1 */
>  	int	rootdir_size;	/* Size of root dir for non-FAT32 */
>  	__u32	root_cluster;	/* First cluster of root dir for FAT32 */
> +	__u32	bs_total_sect;	/* Boot sector's number of sectors */

How can one sector have multiple sectors?
Please, reword the comment to something more obvious.

Best regards

Heinrich

> +	__u8	bs_fats;	/* Boot sector's number of FATs */
>  } fsdata;
>  
>  static inline u32 clust_to_sect(fsdata *fsdata, u32 clust)
>
Heinrich Schuchardt July 22, 2018, 7:43 a.m. | #2
On 07/20/2018 08:06 PM, Heinrich Schuchardt wrote:
> On 07/20/2018 04:57 AM, AKASHI Takahiro wrote:
>> get_fs_info() was introduced in major re-work of read operation by Rob.
>> We want to reuse this function in write operation by extending it with
>> additional members in fsdata structure.
>>
>> Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
>> ---
>>  fs/fat/fat.c  | 3 +++
>>  include/fat.h | 2 ++
>>  2 files changed, 5 insertions(+)
>>
>> diff --git a/fs/fat/fat.c b/fs/fat/fat.c
>> index 4efe8a3eda..b48f48a751 100644
>> --- a/fs/fat/fat.c
>> +++ b/fs/fat/fat.c
>> @@ -556,6 +556,9 @@ static int get_fs_info(fsdata *mydata)
>>  		return ret;
>>  	}
>>  
>> +	mydata->bs_total_sect = bs.total_sect;

Please, check here if sectors is non-zero and in this case use that
value. Do not rely on total_sect = 0 in later coding to read sectors.
Anyway there should be only one place in the coding where we determine
the sector count.

The same logic is used in the Linux kernel:

fs/fat/inode.c:1766:   total_sectors = bpb.fat_sectors;
fs/fat/inode.c:1767:   if (total_sectors == 0)
fs/fat/inode.c:1768:           total_sectors = bpb.fat_total_sect;

>> +	mydata->bs_fats = bs.fats;
>> +
>>  	if (mydata->fatsize == 32) {
>>  		mydata->fatlength = bs.fat32_length;
>>  	} else {
>> diff --git a/include/fat.h b/include/fat.h
>> index 09e1423685..0c88b59a4a 100644
>> --- a/include/fat.h
>> +++ b/include/fat.h
>> @@ -173,6 +173,8 @@ typedef struct {
>>  	int	fatbufnum;	/* Used by get_fatent, init to -1 */
>>  	int	rootdir_size;	/* Size of root dir for non-FAT32 */
>>  	__u32	root_cluster;	/* First cluster of root dir for FAT32 */
>> +	__u32	bs_total_sect;	/* Boot sector's number of sectors */

Please, use

__u32 total_sect; /* Number of sectors */

> 
> How can one sector have multiple sectors?
> Please, reword the comment to something more obvious.
> 
> Best regards
> 
> Heinrich
> 
>> +	__u8	bs_fats;	/* Boot sector's number of FATs */

and

__u8 fats; /* Number of FATs */

This is not information about the boot sector but about the partition.

Best regards

Heinich

>>  } fsdata;
>>  
>>  static inline u32 clust_to_sect(fsdata *fsdata, u32 clust)
>>
> 
>

Patch

diff --git a/fs/fat/fat.c b/fs/fat/fat.c
index 4efe8a3eda..b48f48a751 100644
--- a/fs/fat/fat.c
+++ b/fs/fat/fat.c
@@ -556,6 +556,9 @@  static int get_fs_info(fsdata *mydata)
 		return ret;
 	}
 
+	mydata->bs_total_sect = bs.total_sect;
+	mydata->bs_fats = bs.fats;
+
 	if (mydata->fatsize == 32) {
 		mydata->fatlength = bs.fat32_length;
 	} else {
diff --git a/include/fat.h b/include/fat.h
index 09e1423685..0c88b59a4a 100644
--- a/include/fat.h
+++ b/include/fat.h
@@ -173,6 +173,8 @@  typedef struct {
 	int	fatbufnum;	/* Used by get_fatent, init to -1 */
 	int	rootdir_size;	/* Size of root dir for non-FAT32 */
 	__u32	root_cluster;	/* First cluster of root dir for FAT32 */
+	__u32	bs_total_sect;	/* Boot sector's number of sectors */
+	__u8	bs_fats;	/* Boot sector's number of FATs */
 } fsdata;
 
 static inline u32 clust_to_sect(fsdata *fsdata, u32 clust)