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

[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 <wozi...@huawei.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

2) We'd like to avoid defining variables like this (in the
   switch block), and I even don't think this patch can compile.

3) The logic itself is also broken...

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);
  }