linux-ext4 - Re: /fs/ext4/ext4.h add a comment to ext4_dir_entry

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-Id: <0FC238E1-59D7-448B-BD72-C2794A3BD99E@dilger.ca>
Date:   Mon, 4 May 2020 17:00:03 -0600
From:   Andreas Dilger <adilger@...ger.ca>
To:     "Darrick J. Wong" <darrick.wong@...cle.com>
Cc:     Jonny Grant <jg@...k.org>,
        Ext4 Developers List <linux-ext4@...r.kernel.org>
Subject: Re: /fs/ext4/ext4.h add a comment to ext4_dir_entry_2


> On May 4, 2020, at 4:39 PM, Darrick J. Wong <darrick.wong@...cle.com> wrote:
> 
> On Mon, May 04, 2020 at 04:26:35PM -0600, Andreas Dilger wrote:
>> 
>>> On May 3, 2020, at 6:52 AM, Jonny Grant <jg@...k.org> wrote:
>>> 
>>> Hello
>>> 
>>> Could a comment be added to clarify 'file_type' ?
>>> 
>>> struct ext4_dir_entry_2 {
>>>   __le32    inode;            /* Inode number */
>>>   __le16    rec_len;        /* Directory entry length */
>>>   __u8    name_len;        /* Name length */
>>>   __u8    file_type;
>>>   char    name[EXT4_NAME_LEN];    /* File name */
>>> };
>>> 
>>> 
>>> 
>>> This what I am proposing to add:
>>> 
>>>   __u8    file_type;        /* See directory file type macros below */
>> 
>> For this kind of structure field, it makes sense to reference the macro
>> names directly, like:
>> 
>> 	__u8	file_type;	/* See EXT4_FT_* type macros below */
>> 
>> since "macros below" may be ambiguous as the header changes over time.
>> 
>> 
>> Even better (IMHO) is to use a named enum for this, like:
>> 
>>        enum ext4_file_type file_type:8; /* See EXT4_FT_ types below */
>> 
>> /*
>> * Ext4 directory file types.  Only the low 3 bits are used.  The
>> * other bits are reserved for now.
>> */
>> enum ext4_file_type {
>> 	EXT4_FT_UNKNOWN		= 0,
>> 	EXT4_FT_REG_FILE	= 1,
>> 	EXT4_FT_DIR		= 2,
>> 	EXT4_FT_CHRDEV		= 3,
>> 	EXT4_FT_BLKDEV		= 4,
>> 	EXT4_FT_FIFO		= 5,
>> 	EXT4_FT_SOCK		= 6,
>> 	EXT4_FT_SYMLINK		= 7,
>> 	EXT4_FT_MAX,
>> 	EXT4_FT_DIR_CSUM	= 0xDE
>> };
>> 
>> so that the allowed values for this field are clear from the definition.
>> However, the use of a fixed-with bitfield (enum :8) is a GCC-ism and Ted
>> may be against that for portability reasons, since the kernel and
>> userspace headers should be as similar as possible.
> 
> This is an on-disk structure.  Do /not/ make this an enum because that
> would replace a __u8 with an int, which will break directories.

No, that is what the fixed bitfield declaration "enum ... :8" would do -
declare this enum to be an 8-bit integer.  I've verified that this works
as expected with GCC, to allow an enum with a specific size, like :8 or
:32 or :64.  Obviously, if you specify a bitfield size that doesn't align
with the start of the next structure field, there would be padding added
so that the next field is properly aligned, but that isn't the case here.

Since e2fsprogs needs to be portable to other compilers/OS, I'm not sure
if Ted would want the kernel header declaration to be different than the
e2fsprogs header.  I've grown to like using enum for these kind of "flags"
definitions, since they are much more concrete than a bare "int flags"
declaration, and still better than "int flags; /* see EXT4_FT_* below */"
since the enum is a hard compiler linkage vs. just a comment, for the
same reasons that static inline functions are better than CPP macros.

Cheers, Andreas






Download attachment "signature.asc" of type "application/pgp-signature" (874 bytes)