Re: [PATCH -next V2] erofs: code clean up for function erofs_read_inode()

[Zizhi Wo]

在 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@xxxxxxxxxx>
> > >
> > > 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