Re: [PATCH] ext4: Use struct_size() to improve ext4_htree_store_dirent()

[Thorsten Blum <thorsten.blum@linux.dev>]

On 5. Nov 2024, at 11:39, Greg KH wrote:
> On Tue, Nov 05, 2024 at 11:33:54AM +0100, Thorsten Blum wrote:
>> Inline and use struct_size() to calculate the number of bytes to
>> allocate for new_fn and remove the local variable len.
>> 
>> Reviewed-by: Jan Kara <jack@suse.cz>
>> Signed-off-by: Thorsten Blum <thorsten.blum@linux.dev>
>> ---
>> This change was originally part of another patch that was split into two
>> separate patches after feedback from Greg KH
>> - Link: https://lore.kernel.org/r/20241104234214.8094-2-thorsten.blum@linux.dev/
>> ---
>> fs/ext4/dir.c | 5 ++---
>> 1 file changed, 2 insertions(+), 3 deletions(-)
>> 
>> diff --git a/fs/ext4/dir.c b/fs/ext4/dir.c
>> index 233479647f1b..02d47a64e8d1 100644
>> --- a/fs/ext4/dir.c
>> +++ b/fs/ext4/dir.c
>> @@ -471,14 +471,13 @@ int ext4_htree_store_dirent(struct file *dir_file, __u32 hash,
>> struct rb_node **p, *parent = NULL;
>> struct fname *fname, *new_fn;
>> struct dir_private_info *info;
>> - int len;
>> 
>> info = dir_file->private_data;
>> p = &info->root.rb_node;
>> 
>> /* Create and allocate the fname structure */
>> - len = sizeof(struct fname) + ent_name->len + 1;
>> - new_fn = kzalloc(len, GFP_KERNEL);
>> + new_fn = kzalloc(struct_size(new_fn, name, ent_name->len + 1),
>> + GFP_KERNEL);
> 
> Does this actually matter and make the code any more robust or faster?
> 
> The original code here is easier to read and understand, why add
> complexity if it is not required?

I find struct_size() to be more readable because it explicitly
communicates the relationship between the flexible array member name and
ent_name->len that the open-coded version doesn't. Plus, struct_size()
has some additional compile-time checks (e.g., __must_be_array()).

Thanks,
Thorsten