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

[Zizhi Wo]

在 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...
>
> > ---
> >   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...

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

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
>
> > +
> >           vi->inode_isize = sizeof(struct erofs_inode_extended);
> >           /* check if the extended inode acrosses block boundary */
> >           if (*ofs + vi->inode_isize <= sb->s_blocksize) {
> > @@ -98,6 +99,7 @@ static void *erofs_read_inode(struct erofs_buf *buf,
> >               inode->i_rdev = 0;
> >               break;
> >           default:
> > +            kfree(copied);
> >               goto bogusimode;
> >           }
> >           i_uid_write(inode, le32_to_cpu(die->i_uid));
> > @@ -117,7 +119,6 @@ static void *erofs_read_inode(struct erofs_buf *buf,
> >               /* fill chunked inode summary info */
> >               vi->chunkformat = le16_to_cpu(die->i_u.c.format);
> >           kfree(copied);
> > -        copied = NULL;
> >           break;
> >       case EROFS_INODE_LAYOUT_COMPACT:
> >           vi->inode_isize = sizeof(struct erofs_inode_compact);
> > @@ -197,7 +198,6 @@ static void *erofs_read_inode(struct erofs_buf *buf,
> >       err = -EFSCORRUPTED;
> >   err_out:
> >       DBG_BUGON(1);
> > -    kfree(copied);
> >       erofs_put_metabuf(buf);
> >       return ERR_PTR(err);
> >   }