Re: /fs/ext4/ext4.h add a comment to ext4_dir_entry_2

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

 



> On May 4, 2020, at 4:39 PM, Darrick J. Wong <darrick.wong@xxxxxxxxxx> 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@xxxxxxxx> 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





Attachment: signature.asc
Description: Message signed with OpenPGP


[Index of Archives]     [Reiser Filesystem Development]     [Ceph FS]     [Kernel Newbies]     [Security]     [Netfilter]     [Bugtraq]     [Linux FS]     [Yosemite National Park]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Samba]     [Device Mapper]     [Linux Media]

  Powered by Linux