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

From: Gao Xiang
Date: Thu Nov 09 2023 - 10:44:44 EST




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

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