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
> On May 3, 2020, at 6:52 AM, Jonny Grant <[email protected]> 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
On Mon, May 04, 2020 at 04:26:35PM -0600, Andreas Dilger wrote:
>
> > On May 3, 2020, at 6:52 AM, Jonny Grant <[email protected]> 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
>
>
>
>
>
> On May 4, 2020, at 4:39 PM, Darrick J. Wong <[email protected]> wrote:
>
> On Mon, May 04, 2020 at 04:26:35PM -0600, Andreas Dilger wrote:
>>
>>> On May 3, 2020, at 6:52 AM, Jonny Grant <[email protected]> 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
On 05/05/2020 00:00, Andreas Dilger wrote:
>
>> On May 4, 2020, at 4:39 PM, Darrick J. Wong <[email protected]> wrote:
>>
>> On Mon, May 04, 2020 at 04:26:35PM -0600, Andreas Dilger wrote:
>>>
>>>> On May 3, 2020, at 6:52 AM, Jonny Grant <[email protected]> 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