linux-kernel - Re: [PATCH -next V2] erofs: code clean up for function erofs_read

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Thu, 9 Nov 2023 23:42:03 +0800
From:   Gao Xiang <hsiangkao@...ux.alibaba.com>
To:     Zizhi Wo <wozizhi@...wei.com>, xiang@...nel.org, chao@...nel.org
Cc:     linux-erofs@...ts.ozlabs.org, linux-kernel@...r.kernel.org,
        yangerkun@...wei.com
Subject: Re: [PATCH -next V2] erofs: code clean up for function
 erofs_read_inode()



On 2023/11/9 21:45, Zizhi Wo wrote:
> 
> 
> 在 2023/11/9 21:14, Gao Xiang 写道:
>> Hi,
>>
>> On 2023/11/10 03:48, WoZ1zh1 wrote:
>>> Because variables "die" and "copied" only appear in case
>>> EROFS_INODE_LAYOUT_EXTENDED, move them from the outer space into this
>>> case. Also, call "kfree(copied)" earlier to avoid double free in the
>>> "error_out" branch. Some cleanups, no logic changes.
>>>
>>> Signed-off-by: WoZ1zh1 <wozizhi@...wei.com>
>>
>> Please help use your real name here...
>>
>>> ---
>>>   fs/erofs/inode.c | 6 +++---
>>>   1 file changed, 3 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/fs/erofs/inode.c b/fs/erofs/inode.c
>>> index b8ad05b4509d..a388c93eec34 100644
>>> --- a/fs/erofs/inode.c
>>> +++ b/fs/erofs/inode.c
>>> @@ -19,7 +19,6 @@ static void *erofs_read_inode(struct erofs_buf *buf,
>>>       erofs_blk_t blkaddr, nblks = 0;
>>>       void *kaddr;
>>>       struct erofs_inode_compact *dic;
>>> -    struct erofs_inode_extended *die, *copied = NULL;
>>>       unsigned int ifmt;
>>>       int err;
>>> @@ -53,6 +52,8 @@ static void *erofs_read_inode(struct erofs_buf *buf,
>>>       switch (erofs_inode_version(ifmt)) {
>>>       case EROFS_INODE_LAYOUT_EXTENDED:
>>> +        struct erofs_inode_extended *die, *copied = NULL;
>>
>> Thanks for the patch, but in my own opinion:
>>
>> 1) It doesn't simplify the code
> OK, I'm sorry for the noise(;´༎ຶД༎ຶ`)
>>
>> 2) We'd like to avoid defining variables like this (in the
>>     switch block), and I even don't think this patch can compile.
> I tested this patch with gcc-12.2.1 locally and it compiled
> successfully. I'm not sure if this patch will fail in other environment
> with different compiler...

For example, it fails as below on gcc 10.2.1:

fs/erofs/inode.c: In function 'erofs_read_inode':
fs/erofs/inode.c:55:3: error: a label can only be part of a statement and a declaration is not a statement
    55 |   struct erofs_inode_extended *die, *copied = NULL;
       |   ^~~~~~

> 
>> 3) The logic itself is also broken...

Maybe I was missing something, but this usage makes
me uneasy...

Thanks,
Gao Xiang

> 
> Sorry, but I just don't understand why the logic itself is broken, and
> can you please explain more?
> 
> Thanks,
> Zizhi Wo
> 
>> Thanks,
>> Gao Xiang