Re: [PATCH v8 01/13] exfat: add in-memory and on-disk structures and headers

[Pali RohÃr]

On Friday 20 December 2019 01:24:07 Namjae Jeon wrote:
> This adds in-memory and on-disk structures and headers.
> 
> Signed-off-by: Namjae Jeon <namjae.jeon@xxxxxxxxxxx>
> Signed-off-by: Sungjong Seo <sj1557.seo@xxxxxxxxxxx>
> ---
>  fs/exfat/exfat_fs.h  | 559 +++++++++++++++++++++++++++++++++++++++++++
>  fs/exfat/exfat_raw.h | 202 ++++++++++++++++
>  2 files changed, 761 insertions(+)
>  create mode 100644 fs/exfat/exfat_fs.h
>  create mode 100644 fs/exfat/exfat_raw.h

...

> diff --git a/fs/exfat/exfat_raw.h b/fs/exfat/exfat_raw.h
> new file mode 100644
> index 000000000000..a3ccac835993
> --- /dev/null
> +++ b/fs/exfat/exfat_raw.h

...

> +/* file attributes */
> +#define ATTR_READONLY		0x0001
> +#define ATTR_HIDDEN		0x0002
> +#define ATTR_SYSTEM		0x0004
> +#define ATTR_VOLUME		0x0008
> +#define ATTR_SUBDIR		0x0010
> +#define ATTR_ARCHIVE		0x0020
> +#define ATTR_EXTEND		(ATTR_READONLY | ATTR_HIDDEN | ATTR_SYSTEM | \
> +				 ATTR_VOLUME) /* 0x000F */
> +
> +#define ATTR_EXTEND_MASK	(ATTR_EXTEND | ATTR_SUBDIR | ATTR_ARCHIVE)
> +#define ATTR_RWMASK		(ATTR_HIDDEN | ATTR_SYSTEM | ATTR_VOLUME | \
> +				 ATTR_SUBDIR | ATTR_ARCHIVE)
> +
> +#define ATTR_READONLY_LE	cpu_to_le16(0x0001)
> +#define ATTR_HIDDEN_LE		cpu_to_le16(0x0002)
> +#define ATTR_SYSTEM_LE		cpu_to_le16(0x0004)
> +#define ATTR_VOLUME_LE		cpu_to_le16(0x0008)
> +#define ATTR_SUBDIR_LE		cpu_to_le16(0x0010)
> +#define ATTR_ARCHIVE_LE		cpu_to_le16(0x0020)

Hello!

This looks like copy-paste code from /* file attributes */ section
above. What about at least making these macro definitions as?

  #define ATTR_READONLY_LE	cpu_to_le16(ATTR_READONLY)
  #define ATTR_HIDDEN_LE	cpu_to_le16(ATTR_HIDDEN)
  ...

But main question is, are these _LE definitions needed at all?

Looking at the whole patch series and only ATTR_SUBDIR_LE and
ATTR_ARCHIVE_LE are used.

Is not it better to use cpu_to_le16(ATTR_READONLY) directly in code and
do not define duplicate ATTR_READONLY_LE macro at all?

> +
> +#define JUMP_BOOT_LEN			3
> +#define OEM_NAME_LEN			8
> +#define MUST_BE_ZERO_LEN		53
> +#define EXFAT_FILE_NAME_LEN		15
> +
> +/* EXFAT BIOS parameter block (64 bytes) */
> +struct bpb64 {
> +	__u8 jmp_boot[JUMP_BOOT_LEN];
> +	__u8 oem_name[OEM_NAME_LEN];
> +	__u8 res_zero[MUST_BE_ZERO_LEN];
> +};
> +
> +/* EXFAT EXTEND BIOS parameter block (56 bytes) */
> +struct bsx64 {
> +	__le64 vol_offset;
> +	__le64 vol_length;
> +	__le32 fat_offset;
> +	__le32 fat_length;
> +	__le32 clu_offset;
> +	__le32 clu_count;
> +	__le32 root_cluster;
> +	__le32 vol_serial;
> +	__u8 fs_version[2];
> +	__le16 vol_flags;
> +	__u8 sect_size_bits;
> +	__u8 sect_per_clus_bits;
> +	__u8 num_fats;
> +	__u8 phy_drv_no;
> +	__u8 perc_in_use;
> +	__u8 reserved2[7];
> +};

Should not be this structure marked as packed? Also those two below.

> +/* EXFAT PBR[BPB+BSX] (120 bytes) */
> +struct pbr64 {
> +	struct bpb64 bpb;
> +	struct bsx64 bsx;
> +};
> +
> +/* Common PBR[Partition Boot Record] (512 bytes) */
> +struct pbr {
> +	union {
> +		__u8 raw[64];
> +		struct bpb64 f64;
> +	} bpb;
> +	union {
> +		__u8 raw[56];
> +		struct bsx64 f64;
> +	} bsx;
> +	__u8 boot_code[390];
> +	__le16 signature;
> +};

-- 
Pali RohÃr
pali.rohar@xxxxxxxxx

Attachment: signature.asc
Description: PGP signature