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

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

[Jonny Grant]

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 */


Thank you
Jonny

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

[Andreas Dilger]

[-- Attachment #1: Type: text/plain, Size: 1513 bytes --]


> On May 3, 2020, at 6:52 AM, Jonny Grant <jg@jguk.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.

Cheers, Andreas






[-- Attachment #2: Message signed with OpenPGP --]
[-- Type: application/pgp-signature, Size: 873 bytes --]

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

[Darrick J. Wong]

On Mon, May 04, 2020 at 04:26:35PM -0600, Andreas Dilger wrote:
> 
> > On May 3, 2020, at 6:52 AM, Jonny Grant <jg@jguk.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.

--D

> Cheers, Andreas
> 
> 
> 
> 
> 

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

[Andreas Dilger]

[-- Attachment #1: Type: text/plain, Size: 2967 bytes --]


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






[-- Attachment #2: Message signed with OpenPGP --]
[-- Type: application/pgp-signature, Size: 873 bytes --]

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

[Jonny Grant]

On 05/05/2020 00:00, Andreas Dilger wrote:
> 
>> On May 4, 2020, at 4:39 PM, Darrick J. Wong <darrick.wong@oracle.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@jguk.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

Hi Andreas

Re changing the macros,
how about using the following approach?


const __u8 EXT4_FT_UNKNOWN = 0;
const __u8 EXT4_FT_REG_FILE = 1;
etc

Generally I prefer to avoid macros if I can personally.

Cheers, Jonny