Re: [PATCH v2 2/2] isofs: use unsigned char types consistently

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Thu 19-10-17 16:47:49, Arnd Bergmann wrote:
> Based on the discussion about the signed character field for the year,
> I went through all fields in the iso9660 and rockridge standards to see
> whether they should used signed or unsigned characters. Only a single
> 8-bit value is defined as signed per 'section 7.1.2': the timezone
> offset in a timestamp, this has always been handled correctly through
> explicit sign-extension.
> 
> All others are either '7.1.1 8-bit unsigned numerical values' or
> composite fields. I also read the linux source code and came to the
> same conclusion, also I could not find any other part of the
> implementation that actually behaves differently for signed or
> unsigned values.
> 
> Since it is still ambigous to use plain 'char' in interface definitions,
> I'm changing all fields representing numbers and reserved bytes to
> the unambiguous '__u8'. Fields that hold actual strings are left as
> 'char' arrays. I built the code with '-Wpointer-sign -Wsign-compare'
> to see if anything got left out, but couldn't find anything wrong
> with the remaining warnings.
> 
> This patch should not change runtime behavior and does not need to
> be backported.
> 
> Signed-off-by: Arnd Bergmann <arnd@xxxxxxxx>

The patch looks good to me so I've picked it up to my tree.

								Honza

> ---
>  fs/isofs/isofs.h            |  20 +++---
>  fs/isofs/rock.h             |  62 ++++++++---------
>  include/uapi/linux/iso_fs.h | 162 ++++++++++++++++++++++----------------------
>  3 files changed, 122 insertions(+), 122 deletions(-)
> 
> diff --git a/fs/isofs/isofs.h b/fs/isofs/isofs.h
> index bd4047585431..c882f207dd5c 100644
> --- a/fs/isofs/isofs.h
> +++ b/fs/isofs/isofs.h
> @@ -72,36 +72,36 @@ static inline struct iso_inode_info *ISOFS_I(struct inode *inode)
>  	return container_of(inode, struct iso_inode_info, vfs_inode);
>  }
>  
> -static inline int isonum_711(char *p)
> +static inline int isonum_711(u8 *p)
>  {
> -	return *(u8 *)p;
> +	return *p;
>  }
> -static inline int isonum_712(char *p)
> +static inline int isonum_712(s8 *p)
>  {
> -	return *(s8 *)p;
> +	return *p;
>  }
> -static inline unsigned int isonum_721(char *p)
> +static inline unsigned int isonum_721(u8 *p)
>  {
>  	return get_unaligned_le16(p);
>  }
> -static inline unsigned int isonum_722(char *p)
> +static inline unsigned int isonum_722(u8 *p)
>  {
>  	return get_unaligned_be16(p);
>  }
> -static inline unsigned int isonum_723(char *p)
> +static inline unsigned int isonum_723(u8 *p)
>  {
>  	/* Ignore bigendian datum due to broken mastering programs */
>  	return get_unaligned_le16(p);
>  }
> -static inline unsigned int isonum_731(char *p)
> +static inline unsigned int isonum_731(u8 *p)
>  {
>  	return get_unaligned_le32(p);
>  }
> -static inline unsigned int isonum_732(char *p)
> +static inline unsigned int isonum_732(u8 *p)
>  {
>  	return get_unaligned_be32(p);
>  }
> -static inline unsigned int isonum_733(char *p)
> +static inline unsigned int isonum_733(u8 *p)
>  {
>  	/* Ignore bigendian datum due to broken mastering programs */
>  	return get_unaligned_le32(p);
> diff --git a/fs/isofs/rock.h b/fs/isofs/rock.h
> index f835976ce033..8780d67c6ca5 100644
> --- a/fs/isofs/rock.h
> +++ b/fs/isofs/rock.h
> @@ -6,62 +6,62 @@
>   */
>  
>  struct SU_SP_s {
> -	unsigned char magic[2];
> -	unsigned char skip;
> +	__u8 magic[2];
> +	__u8 skip;
>  } __attribute__ ((packed));
>  
>  struct SU_CE_s {
> -	char extent[8];
> -	char offset[8];
> -	char size[8];
> +	__u8 extent[8];
> +	__u8 offset[8];
> +	__u8 size[8];
>  };
>  
>  struct SU_ER_s {
> -	unsigned char len_id;
> -	unsigned char len_des;
> -	unsigned char len_src;
> -	unsigned char ext_ver;
> -	char data[0];
> +	__u8 len_id;
> +	__u8 len_des;
> +	__u8 len_src;
> +	__u8 ext_ver;
> +	__u8 data[0];
>  } __attribute__ ((packed));
>  
>  struct RR_RR_s {
> -	char flags[1];
> +	__u8 flags[1];
>  } __attribute__ ((packed));
>  
>  struct RR_PX_s {
> -	char mode[8];
> -	char n_links[8];
> -	char uid[8];
> -	char gid[8];
> +	__u8 mode[8];
> +	__u8 n_links[8];
> +	__u8 uid[8];
> +	__u8 gid[8];
>  };
>  
>  struct RR_PN_s {
> -	char dev_high[8];
> -	char dev_low[8];
> +	__u8 dev_high[8];
> +	__u8 dev_low[8];
>  };
>  
>  struct SL_component {
> -	unsigned char flags;
> -	unsigned char len;
> -	char text[0];
> +	__u8 flags;
> +	__u8 len;
> +	__u8 text[0];
>  } __attribute__ ((packed));
>  
>  struct RR_SL_s {
> -	unsigned char flags;
> +	__u8 flags;
>  	struct SL_component link;
>  } __attribute__ ((packed));
>  
>  struct RR_NM_s {
> -	unsigned char flags;
> +	__u8 flags;
>  	char name[0];
>  } __attribute__ ((packed));
>  
>  struct RR_CL_s {
> -	char location[8];
> +	__u8 location[8];
>  };
>  
>  struct RR_PL_s {
> -	char location[8];
> +	__u8 location[8];
>  };
>  
>  struct stamp {
> @@ -69,15 +69,15 @@ struct stamp {
>  } __attribute__ ((packed));
>  
>  struct RR_TF_s {
> -	char flags;
> +	__u8 flags;
>  	struct stamp times[0];	/* Variable number of these beasts */
>  } __attribute__ ((packed));
>  
>  /* Linux-specific extension for transparent decompression */
>  struct RR_ZF_s {
> -	char algorithm[2];
> -	char parms[2];
> -	char real_size[8];
> +	__u8 algorithm[2];
> +	__u8 parms[2];
> +	__u8 real_size[8];
>  };
>  
>  /*
> @@ -93,9 +93,9 @@ struct RR_ZF_s {
>  #define TF_LONG_FORM 128
>  
>  struct rock_ridge {
> -	char signature[2];
> -	unsigned char len;
> -	unsigned char version;
> +	__u8 signature[2];
> +	__u8 len;
> +	__u8 version;
>  	union {
>  		struct SU_SP_s SP;
>  		struct SU_CE_s CE;
> diff --git a/include/uapi/linux/iso_fs.h b/include/uapi/linux/iso_fs.h
> index 4688ac4284e2..07c4c6405b3c 100644
> --- a/include/uapi/linux/iso_fs.h
> +++ b/include/uapi/linux/iso_fs.h
> @@ -12,10 +12,10 @@
>  #define ISODCL(from, to) (to - from + 1)
>  
>  struct iso_volume_descriptor {
> -	char type[ISODCL(1,1)]; /* 711 */
> +	__u8 type[ISODCL(1,1)]; /* 711 */
>  	char id[ISODCL(2,6)];
> -	char version[ISODCL(7,7)];
> -	char data[ISODCL(8,2048)];
> +	__u8 version[ISODCL(7,7)];
> +	__u8 data[ISODCL(8,2048)];
>  };
>  
>  /* volume descriptor types */
> @@ -26,24 +26,24 @@ struct iso_volume_descriptor {
>  #define ISO_STANDARD_ID "CD001"
>  
>  struct iso_primary_descriptor {
> -	char type			[ISODCL (  1,   1)]; /* 711 */
> +	__u8 type			[ISODCL (  1,   1)]; /* 711 */
>  	char id				[ISODCL (  2,   6)];
> -	char version			[ISODCL (  7,   7)]; /* 711 */
> -	char unused1			[ISODCL (  8,   8)];
> +	__u8 version			[ISODCL (  7,   7)]; /* 711 */
> +	__u8 unused1			[ISODCL (  8,   8)];
>  	char system_id			[ISODCL (  9,  40)]; /* achars */
>  	char volume_id			[ISODCL ( 41,  72)]; /* dchars */
> -	char unused2			[ISODCL ( 73,  80)];
> -	char volume_space_size		[ISODCL ( 81,  88)]; /* 733 */
> -	char unused3			[ISODCL ( 89, 120)];
> -	char volume_set_size		[ISODCL (121, 124)]; /* 723 */
> -	char volume_sequence_number	[ISODCL (125, 128)]; /* 723 */
> -	char logical_block_size		[ISODCL (129, 132)]; /* 723 */
> -	char path_table_size		[ISODCL (133, 140)]; /* 733 */
> -	char type_l_path_table		[ISODCL (141, 144)]; /* 731 */
> -	char opt_type_l_path_table	[ISODCL (145, 148)]; /* 731 */
> -	char type_m_path_table		[ISODCL (149, 152)]; /* 732 */
> -	char opt_type_m_path_table	[ISODCL (153, 156)]; /* 732 */
> -	char root_directory_record	[ISODCL (157, 190)]; /* 9.1 */
> +	__u8 unused2			[ISODCL ( 73,  80)];
> +	__u8 volume_space_size		[ISODCL ( 81,  88)]; /* 733 */
> +	__u8 unused3			[ISODCL ( 89, 120)];
> +	__u8 volume_set_size		[ISODCL (121, 124)]; /* 723 */
> +	__u8 volume_sequence_number	[ISODCL (125, 128)]; /* 723 */
> +	__u8 logical_block_size		[ISODCL (129, 132)]; /* 723 */
> +	__u8 path_table_size		[ISODCL (133, 140)]; /* 733 */
> +	__u8 type_l_path_table		[ISODCL (141, 144)]; /* 731 */
> +	__u8 opt_type_l_path_table	[ISODCL (145, 148)]; /* 731 */
> +	__u8 type_m_path_table		[ISODCL (149, 152)]; /* 732 */
> +	__u8 opt_type_m_path_table	[ISODCL (153, 156)]; /* 732 */
> +	__u8 root_directory_record	[ISODCL (157, 190)]; /* 9.1 */
>  	char volume_set_id		[ISODCL (191, 318)]; /* dchars */
>  	char publisher_id		[ISODCL (319, 446)]; /* achars */
>  	char preparer_id		[ISODCL (447, 574)]; /* achars */
> @@ -51,36 +51,36 @@ struct iso_primary_descriptor {
>  	char copyright_file_id		[ISODCL (703, 739)]; /* 7.5 dchars */
>  	char abstract_file_id		[ISODCL (740, 776)]; /* 7.5 dchars */
>  	char bibliographic_file_id	[ISODCL (777, 813)]; /* 7.5 dchars */
> -	char creation_date		[ISODCL (814, 830)]; /* 8.4.26.1 */
> -	char modification_date		[ISODCL (831, 847)]; /* 8.4.26.1 */
> -	char expiration_date		[ISODCL (848, 864)]; /* 8.4.26.1 */
> -	char effective_date		[ISODCL (865, 881)]; /* 8.4.26.1 */
> -	char file_structure_version	[ISODCL (882, 882)]; /* 711 */
> -	char unused4			[ISODCL (883, 883)];
> -	char application_data		[ISODCL (884, 1395)];
> -	char unused5			[ISODCL (1396, 2048)];
> +	__u8 creation_date		[ISODCL (814, 830)]; /* 8.4.26.1 */
> +	__u8 modification_date		[ISODCL (831, 847)]; /* 8.4.26.1 */
> +	__u8 expiration_date		[ISODCL (848, 864)]; /* 8.4.26.1 */
> +	__u8 effective_date		[ISODCL (865, 881)]; /* 8.4.26.1 */
> +	__u8 file_structure_version	[ISODCL (882, 882)]; /* 711 */
> +	__u8 unused4			[ISODCL (883, 883)];
> +	__u8 application_data		[ISODCL (884, 1395)];
> +	__u8 unused5			[ISODCL (1396, 2048)];
>  };
>  
>  /* Almost the same as the primary descriptor but two fields are specified */
>  struct iso_supplementary_descriptor {
> -	char type			[ISODCL (  1,   1)]; /* 711 */
> +	__u8 type			[ISODCL (  1,   1)]; /* 711 */
>  	char id				[ISODCL (  2,   6)];
> -	char version			[ISODCL (  7,   7)]; /* 711 */
> -	char flags			[ISODCL (  8,   8)]; /* 853 */
> +	__u8 version			[ISODCL (  7,   7)]; /* 711 */
> +	__u8 flags			[ISODCL (  8,   8)]; /* 853 */
>  	char system_id			[ISODCL (  9,  40)]; /* achars */
>  	char volume_id			[ISODCL ( 41,  72)]; /* dchars */
> -	char unused2			[ISODCL ( 73,  80)];
> -	char volume_space_size		[ISODCL ( 81,  88)]; /* 733 */
> -	char escape			[ISODCL ( 89, 120)]; /* 856 */
> -	char volume_set_size		[ISODCL (121, 124)]; /* 723 */
> -	char volume_sequence_number	[ISODCL (125, 128)]; /* 723 */
> -	char logical_block_size		[ISODCL (129, 132)]; /* 723 */
> -	char path_table_size		[ISODCL (133, 140)]; /* 733 */
> -	char type_l_path_table		[ISODCL (141, 144)]; /* 731 */
> -	char opt_type_l_path_table	[ISODCL (145, 148)]; /* 731 */
> -	char type_m_path_table		[ISODCL (149, 152)]; /* 732 */
> -	char opt_type_m_path_table	[ISODCL (153, 156)]; /* 732 */
> -	char root_directory_record	[ISODCL (157, 190)]; /* 9.1 */
> +	__u8 unused2			[ISODCL ( 73,  80)];
> +	__u8 volume_space_size		[ISODCL ( 81,  88)]; /* 733 */
> +	__u8 escape			[ISODCL ( 89, 120)]; /* 856 */
> +	__u8 volume_set_size		[ISODCL (121, 124)]; /* 723 */
> +	__u8 volume_sequence_number	[ISODCL (125, 128)]; /* 723 */
> +	__u8 logical_block_size		[ISODCL (129, 132)]; /* 723 */
> +	__u8 path_table_size		[ISODCL (133, 140)]; /* 733 */
> +	__u8 type_l_path_table		[ISODCL (141, 144)]; /* 731 */
> +	__u8 opt_type_l_path_table	[ISODCL (145, 148)]; /* 731 */
> +	__u8 type_m_path_table		[ISODCL (149, 152)]; /* 732 */
> +	__u8 opt_type_m_path_table	[ISODCL (153, 156)]; /* 732 */
> +	__u8 root_directory_record	[ISODCL (157, 190)]; /* 9.1 */
>  	char volume_set_id		[ISODCL (191, 318)]; /* dchars */
>  	char publisher_id		[ISODCL (319, 446)]; /* achars */
>  	char preparer_id		[ISODCL (447, 574)]; /* achars */
> @@ -88,54 +88,54 @@ struct iso_supplementary_descriptor {
>  	char copyright_file_id		[ISODCL (703, 739)]; /* 7.5 dchars */
>  	char abstract_file_id		[ISODCL (740, 776)]; /* 7.5 dchars */
>  	char bibliographic_file_id	[ISODCL (777, 813)]; /* 7.5 dchars */
> -	char creation_date		[ISODCL (814, 830)]; /* 8.4.26.1 */
> -	char modification_date		[ISODCL (831, 847)]; /* 8.4.26.1 */
> -	char expiration_date		[ISODCL (848, 864)]; /* 8.4.26.1 */
> -	char effective_date		[ISODCL (865, 881)]; /* 8.4.26.1 */
> -	char file_structure_version	[ISODCL (882, 882)]; /* 711 */
> -	char unused4			[ISODCL (883, 883)];
> -	char application_data		[ISODCL (884, 1395)];
> -	char unused5			[ISODCL (1396, 2048)];
> +	__u8 creation_date		[ISODCL (814, 830)]; /* 8.4.26.1 */
> +	__u8 modification_date		[ISODCL (831, 847)]; /* 8.4.26.1 */
> +	__u8 expiration_date		[ISODCL (848, 864)]; /* 8.4.26.1 */
> +	__u8 effective_date		[ISODCL (865, 881)]; /* 8.4.26.1 */
> +	__u8 file_structure_version	[ISODCL (882, 882)]; /* 711 */
> +	__u8 unused4			[ISODCL (883, 883)];
> +	__u8 application_data		[ISODCL (884, 1395)];
> +	__u8 unused5			[ISODCL (1396, 2048)];
>  };
>  
>  
>  #define HS_STANDARD_ID "CDROM"
>  
>  struct  hs_volume_descriptor {
> -	char foo			[ISODCL (  1,   8)]; /* 733 */
> -	char type			[ISODCL (  9,   9)]; /* 711 */
> +	__u8 foo			[ISODCL (  1,   8)]; /* 733 */
> +	__u8 type			[ISODCL (  9,   9)]; /* 711 */
>  	char id				[ISODCL ( 10,  14)];
> -	char version			[ISODCL ( 15,  15)]; /* 711 */
> -	char data[ISODCL(16,2048)];
> +	__u8 version			[ISODCL ( 15,  15)]; /* 711 */
> +	__u8 data[ISODCL(16,2048)];
>  };
>  
>  
>  struct hs_primary_descriptor {
> -	char foo			[ISODCL (  1,   8)]; /* 733 */
> -	char type			[ISODCL (  9,   9)]; /* 711 */
> -	char id				[ISODCL ( 10,  14)];
> -	char version			[ISODCL ( 15,  15)]; /* 711 */
> -	char unused1			[ISODCL ( 16,  16)]; /* 711 */
> +	__u8 foo			[ISODCL (  1,   8)]; /* 733 */
> +	__u8 type			[ISODCL (  9,   9)]; /* 711 */
> +	__u8 id				[ISODCL ( 10,  14)];
> +	__u8 version			[ISODCL ( 15,  15)]; /* 711 */
> +	__u8 unused1			[ISODCL ( 16,  16)]; /* 711 */
>  	char system_id			[ISODCL ( 17,  48)]; /* achars */
>  	char volume_id			[ISODCL ( 49,  80)]; /* dchars */
> -	char unused2			[ISODCL ( 81,  88)]; /* 733 */
> -	char volume_space_size		[ISODCL ( 89,  96)]; /* 733 */
> -	char unused3			[ISODCL ( 97, 128)]; /* 733 */
> -	char volume_set_size		[ISODCL (129, 132)]; /* 723 */
> -	char volume_sequence_number	[ISODCL (133, 136)]; /* 723 */
> -	char logical_block_size		[ISODCL (137, 140)]; /* 723 */
> -	char path_table_size		[ISODCL (141, 148)]; /* 733 */
> -	char type_l_path_table		[ISODCL (149, 152)]; /* 731 */
> -	char unused4			[ISODCL (153, 180)]; /* 733 */
> -	char root_directory_record	[ISODCL (181, 214)]; /* 9.1 */
> +	__u8 unused2			[ISODCL ( 81,  88)]; /* 733 */
> +	__u8 volume_space_size		[ISODCL ( 89,  96)]; /* 733 */
> +	__u8 unused3			[ISODCL ( 97, 128)]; /* 733 */
> +	__u8 volume_set_size		[ISODCL (129, 132)]; /* 723 */
> +	__u8 volume_sequence_number	[ISODCL (133, 136)]; /* 723 */
> +	__u8 logical_block_size		[ISODCL (137, 140)]; /* 723 */
> +	__u8 path_table_size		[ISODCL (141, 148)]; /* 733 */
> +	__u8 type_l_path_table		[ISODCL (149, 152)]; /* 731 */
> +	__u8 unused4			[ISODCL (153, 180)]; /* 733 */
> +	__u8 root_directory_record	[ISODCL (181, 214)]; /* 9.1 */
>  };
>  
>  /* We use this to help us look up the parent inode numbers. */
>  
>  struct iso_path_table{
> -	unsigned char  name_len[2];	/* 721 */
> -	char extent[4];		/* 731 */
> -	char  parent[2];	/* 721 */
> +	__u8  name_len[2];	/* 721 */
> +	__u8  extent[4];	/* 731 */
> +	__u8  parent[2];	/* 721 */
>  	char name[0];
>  } __attribute__((packed));
>  
> @@ -143,16 +143,16 @@ struct iso_path_table{
>     there is an extra reserved byte after the flags */
>  
>  struct iso_directory_record {
> -	char length			[ISODCL (1, 1)]; /* 711 */
> -	char ext_attr_length		[ISODCL (2, 2)]; /* 711 */
> -	char extent			[ISODCL (3, 10)]; /* 733 */
> -	char size			[ISODCL (11, 18)]; /* 733 */
> -	char date			[ISODCL (19, 25)]; /* 7 by 711 */
> -	char flags			[ISODCL (26, 26)];
> -	char file_unit_size		[ISODCL (27, 27)]; /* 711 */
> -	char interleave			[ISODCL (28, 28)]; /* 711 */
> -	char volume_sequence_number	[ISODCL (29, 32)]; /* 723 */
> -	unsigned char name_len		[ISODCL (33, 33)]; /* 711 */
> +	__u8 length			[ISODCL (1, 1)]; /* 711 */
> +	__u8 ext_attr_length		[ISODCL (2, 2)]; /* 711 */
> +	__u8 extent			[ISODCL (3, 10)]; /* 733 */
> +	__u8 size			[ISODCL (11, 18)]; /* 733 */
> +	__u8 date			[ISODCL (19, 25)]; /* 7 by 711 */
> +	__u8 flags			[ISODCL (26, 26)];
> +	__u8 file_unit_size		[ISODCL (27, 27)]; /* 711 */
> +	__u8 interleave			[ISODCL (28, 28)]; /* 711 */
> +	__u8 volume_sequence_number	[ISODCL (29, 32)]; /* 723 */
> +	__u8 name_len			[ISODCL (33, 33)]; /* 711 */
>  	char name			[0];
>  } __attribute__((packed));
>  
> -- 
> 2.9.0
> 
-- 
Jan Kara <jack@xxxxxxxx>
SUSE Labs, CR



[Index of Archives]     [Linux Ext4 Filesystem]     [Union Filesystem]     [Filesystem Testing]     [Ceph Users]     [Ecryptfs]     [AutoFS]     [Kernel Newbies]     [Share Photos]     [Security]     [Netfilter]     [Bugtraq]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux Cachefs]     [Reiser Filesystem]     [Linux RAID]     [Samba]     [Device Mapper]     [CEPH Development]
  Powered by Linux