[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <bd728618-222b-43ba-8b47-91e90624b49c@huawei.com>
Date: Fri, 10 Nov 2023 09:41:19 +0800
From: Zizhi Wo <wozizhi@...wei.com>
To: Gao Xiang <hsiangkao@...ux.alibaba.com>, <xiang@...nel.org>,
<chao@...nel.org>
CC: <linux-erofs@...ts.ozlabs.org>, <linux-kernel@...r.kernel.org>,
<yangerkun@...wei.com>, <wozizhi@...wei.com>
Subject: Re: [PATCH -next V2] erofs: code clean up for function
erofs_read_inode()
在 2023/11/9 23:42, Gao Xiang 写道:
>
>
> 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...
Oh, I'm sorry for the confusion I caused you. I have changed my name on
.gitconfig.
>>>
>>>> ---
>>>> 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;
> | ^~~~~~
>
Oh, I'm sorry about that! I still need to learn more. Thank you for your
assistance!
Thanks,
Zizhi Wo
>>
>>> 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
>
Powered by blists - more mailing lists