Re: [PATCH] hugetlbfs:inode: initialize 'error' variable at definition to reduce code redundancy

[Ye Liu]

On 2025/6/11 02:51, Oscar Salvador wrote:
> On Mon, Jun 09, 2025 at 02:56:15PM +0800, Hu Song wrote:
>> Initialize the error variable to -ENOMEM at definition in
>> init_hugetlbfs_fs().This removes the need for a separate
>> initialization later and makes the code slightly more concise,
>> while still preserving the original logic.
>> No functional change intended.
>>
>> Signed-off-by: Hu Song <husong@xxxxxxxxxx>
>> ---
>>  fs/hugetlbfs/inode.c | 3 +--
>>  1 file changed, 1 insertion(+), 2 deletions(-)
>>
>> diff --git a/fs/hugetlbfs/inode.c b/fs/hugetlbfs/inode.c
>> index e4de5425838d..390cddd5872c 100644
>> --- a/fs/hugetlbfs/inode.c
>> +++ b/fs/hugetlbfs/inode.c
>> @@ -1600,7 +1600,7 @@ static int __init init_hugetlbfs_fs(void)
>>  {
>>  	struct vfsmount *mnt;
>>  	struct hstate *h;
>> -	int error;
>> +	int error = -ENOMEM;
>>  	int i;
>>  
>>  	if (!hugepages_supported()) {
>> @@ -1608,7 +1608,6 @@ static int __init init_hugetlbfs_fs(void)
>>  		return -ENOTSUPP;
>>  	}
>>  
>> -	error = -ENOMEM;
>>  	hugetlbfs_inode_cachep = kmem_cache_create("hugetlbfs_inode_cache",
>>  					sizeof(struct hugetlbfs_inode_info),
>>  					0, SLAB_ACCOUNT, init_once);
> 
> Uhmf, I do not know. 
> 
> Often, we tend to use those declarations in order to mark what error
> code we will return if we fail the next operationg. E.g:
> 
>   error = -ENOMEM
>   if (try_to_allocate)
>     goto out;
>  
>   error = -EINVAL
>   if (check_params)
>     goto out;
>  
>  out:
>   return error
> 

I prefer the following format, as it avoids unnecessary assignments:

        if (try_to_allocate) {
                error = -ENOMEM
                goto out;
        }
        
        if (check_params) {
                error = -EINVAL
                goto out;
        }
out:
	return error                   

> No really strong opinion here, but I'd vote for leave it as is?

Currently init_hugetlbfs_fs() pre-initializes the error variable
with -ENOMEM before the allocation that might fail. This creates
an unnecessary assignment in the success path.                  

Therefore, this change might be preferable:

diff --git a/fs/hugetlbfs/inode.c b/fs/hugetlbfs/inode.c
index e4de5425838d..3fa7892ac865 100644
--- a/fs/hugetlbfs/inode.c
+++ b/fs/hugetlbfs/inode.c
@@ -1608,12 +1608,13 @@ static int __init init_hugetlbfs_fs(void)
                return -ENOTSUPP;
        }
 
-       error = -ENOMEM;
        hugetlbfs_inode_cachep = kmem_cache_create("hugetlbfs_inode_cache",
                                        sizeof(struct hugetlbfs_inode_info),
                                        0, SLAB_ACCOUNT, init_once);
-       if (hugetlbfs_inode_cachep == NULL)
+       if (hugetlbfs_inode_cachep == NULL) {
+               error = -ENOMEM;
                goto out;
+       }
 
        error = register_filesystem(&hugetlbfs_fs_type);
        if (error)

Thanks,
Ye