From: Jan Kara <jack@suse.cz>
To: Arnd Bergmann <arnd@arndb.de>
Cc: y2038@lists.linaro.org, Jan Kara <jack@suse.cz>,
Al Viro <viro@zeniv.linux.org.uk>,
Deepa Dinamani <deepa.kernel@gmail.com>,
linux-fsdevel@vger.kernel.org, Anders Larsen <al@alarsen.net>,
David Howells <dhowells@redhat.com>,
linux-kernel@vger.kernel.org
Subject: Re: [PATCH v2 2/2] isofs: use unsigned char types consistently
Date: Tue, 31 Oct 2017 18:11:56 +0100 [thread overview]
Message-ID: <20171031171156.GI26128@quack2.suse.cz> (raw)
In-Reply-To: <20171019144811.2144678-2-arnd@arndb.de>
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@arndb.de>
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@suse.com>
SUSE Labs, CR
next prev parent reply other threads:[~2017-10-31 17:12 UTC|newest]
Thread overview: 7+ messages / expand[flat|nested] mbox.gz Atom feed top
2017-10-19 14:47 [PATCH v2 1/2] isofs: fix timestamps beyond 2027 Arnd Bergmann
2017-10-19 14:47 ` [PATCH v2 2/2] isofs: use unsigned char types consistently Arnd Bergmann
2017-10-31 17:11 ` Jan Kara [this message]
2017-10-19 15:17 ` [PATCH v2 1/2] isofs: fix timestamps beyond 2027 Jan Kara
2017-10-19 15:29 ` Arnd Bergmann
2017-10-31 16:59 ` Jan Kara
2017-10-31 21:36 ` Arnd Bergmann
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20171031171156.GI26128@quack2.suse.cz \
--to=jack@suse.cz \
--cc=al@alarsen.net \
--cc=arnd@arndb.de \
--cc=deepa.kernel@gmail.com \
--cc=dhowells@redhat.com \
--cc=linux-fsdevel@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=viro@zeniv.linux.org.uk \
--cc=y2038@lists.linaro.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).