1. Patchew
  2. linux
  3. View series

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

WoZ1zh1 posted 1 patch 2 years, 1 month ago 
fs/erofs/inode.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
[PATCH -next V2] erofs: code clean up for function erofs_read_inode()
Posted by WoZ1zh1 2 years, 1 month ago 
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@huawei.com>
---
 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;
+
 		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);
 }
-- 
2.39.2
Re: [PATCH -next V2] erofs: code clean up for function erofs_read_inode()
Posted by kernel test robot 2 years, 1 month ago 
Hi WoZ1zh1,

kernel test robot noticed the following build errors:

[auto build test ERROR on next-20231109]

url:    https://github.com/intel-lab-lkp/linux/commits/WoZ1zh1/erofs-code-clean-up-for-function-erofs_read_inode/20231110-033810
base:   next-20231109
patch link:    https://lore.kernel.org/r/20231109194821.1719430-1-wozizhi%40huawei.com
patch subject: [PATCH -next V2] erofs: code clean up for function erofs_read_inode()
config: i386-buildonly-randconfig-004-20231111 (https://download.01.org/0day-ci/archive/20231111/202311111038.Atx87gVV-lkp@intel.com/config)
compiler: gcc-9 (Debian 9.3.0-22) 9.3.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20231111/202311111038.Atx87gVV-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202311111038.Atx87gVV-lkp@intel.com/

All errors (new ones prefixed by >>):

   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;
         |   ^~~~~~


vim +55 fs/erofs/inode.c

    10	
    11	static void *erofs_read_inode(struct erofs_buf *buf,
    12				      struct inode *inode, unsigned int *ofs)
    13	{
    14		struct super_block *sb = inode->i_sb;
    15		struct erofs_sb_info *sbi = EROFS_SB(sb);
    16		struct erofs_inode *vi = EROFS_I(inode);
    17		const erofs_off_t inode_loc = erofs_iloc(inode);
    18	
    19		erofs_blk_t blkaddr, nblks = 0;
    20		void *kaddr;
    21		struct erofs_inode_compact *dic;
    22		unsigned int ifmt;
    23		int err;
    24	
    25		blkaddr = erofs_blknr(sb, inode_loc);
    26		*ofs = erofs_blkoff(sb, inode_loc);
    27	
    28		kaddr = erofs_read_metabuf(buf, sb, blkaddr, EROFS_KMAP);
    29		if (IS_ERR(kaddr)) {
    30			erofs_err(sb, "failed to get inode (nid: %llu) page, err %ld",
    31				  vi->nid, PTR_ERR(kaddr));
    32			return kaddr;
    33		}
    34	
    35		dic = kaddr + *ofs;
    36		ifmt = le16_to_cpu(dic->i_format);
    37	
    38		if (ifmt & ~EROFS_I_ALL) {
    39			erofs_err(inode->i_sb, "unsupported i_format %u of nid %llu",
    40				  ifmt, vi->nid);
    41			err = -EOPNOTSUPP;
    42			goto err_out;
    43		}
    44	
    45		vi->datalayout = erofs_inode_datalayout(ifmt);
    46		if (vi->datalayout >= EROFS_INODE_DATALAYOUT_MAX) {
    47			erofs_err(inode->i_sb, "unsupported datalayout %u of nid %llu",
    48				  vi->datalayout, vi->nid);
    49			err = -EOPNOTSUPP;
    50			goto err_out;
    51		}
    52	
    53		switch (erofs_inode_version(ifmt)) {
    54		case EROFS_INODE_LAYOUT_EXTENDED:
  > 55			struct erofs_inode_extended *die, *copied = NULL;
    56	
    57			vi->inode_isize = sizeof(struct erofs_inode_extended);
    58			/* check if the extended inode acrosses block boundary */
    59			if (*ofs + vi->inode_isize <= sb->s_blocksize) {
    60				*ofs += vi->inode_isize;
    61				die = (struct erofs_inode_extended *)dic;
    62			} else {
    63				const unsigned int gotten = sb->s_blocksize - *ofs;
    64	
    65				copied = kmalloc(vi->inode_isize, GFP_NOFS);
    66				if (!copied) {
    67					err = -ENOMEM;
    68					goto err_out;
    69				}
    70				memcpy(copied, dic, gotten);
    71				kaddr = erofs_read_metabuf(buf, sb, blkaddr + 1,
    72							   EROFS_KMAP);
    73				if (IS_ERR(kaddr)) {
    74					erofs_err(sb, "failed to get inode payload block (nid: %llu), err %ld",
    75						  vi->nid, PTR_ERR(kaddr));
    76					kfree(copied);
    77					return kaddr;
    78				}
    79				*ofs = vi->inode_isize - gotten;
    80				memcpy((u8 *)copied + gotten, kaddr, *ofs);
    81				die = copied;
    82			}
    83			vi->xattr_isize = erofs_xattr_ibody_size(die->i_xattr_icount);
    84	
    85			inode->i_mode = le16_to_cpu(die->i_mode);
    86			switch (inode->i_mode & S_IFMT) {
    87			case S_IFREG:
    88			case S_IFDIR:
    89			case S_IFLNK:
    90				vi->raw_blkaddr = le32_to_cpu(die->i_u.raw_blkaddr);
    91				break;
    92			case S_IFCHR:
    93			case S_IFBLK:
    94				inode->i_rdev =
    95					new_decode_dev(le32_to_cpu(die->i_u.rdev));
    96				break;
    97			case S_IFIFO:
    98			case S_IFSOCK:
    99				inode->i_rdev = 0;
   100				break;
   101			default:
   102				kfree(copied);
   103				goto bogusimode;
   104			}
   105			i_uid_write(inode, le32_to_cpu(die->i_uid));
   106			i_gid_write(inode, le32_to_cpu(die->i_gid));
   107			set_nlink(inode, le32_to_cpu(die->i_nlink));
   108	
   109			/* extended inode has its own timestamp */
   110			inode_set_ctime(inode, le64_to_cpu(die->i_mtime),
   111					le32_to_cpu(die->i_mtime_nsec));
   112	
   113			inode->i_size = le64_to_cpu(die->i_size);
   114	
   115			/* total blocks for compressed files */
   116			if (erofs_inode_is_data_compressed(vi->datalayout))
   117				nblks = le32_to_cpu(die->i_u.compressed_blocks);
   118			else if (vi->datalayout == EROFS_INODE_CHUNK_BASED)
   119				/* fill chunked inode summary info */
   120				vi->chunkformat = le16_to_cpu(die->i_u.c.format);
   121			kfree(copied);
   122			break;
   123		case EROFS_INODE_LAYOUT_COMPACT:
   124			vi->inode_isize = sizeof(struct erofs_inode_compact);
   125			*ofs += vi->inode_isize;
   126			vi->xattr_isize = erofs_xattr_ibody_size(dic->i_xattr_icount);
   127	
   128			inode->i_mode = le16_to_cpu(dic->i_mode);
   129			switch (inode->i_mode & S_IFMT) {
   130			case S_IFREG:
   131			case S_IFDIR:
   132			case S_IFLNK:
   133				vi->raw_blkaddr = le32_to_cpu(dic->i_u.raw_blkaddr);
   134				break;
   135			case S_IFCHR:
   136			case S_IFBLK:
   137				inode->i_rdev =
   138					new_decode_dev(le32_to_cpu(dic->i_u.rdev));
   139				break;
   140			case S_IFIFO:
   141			case S_IFSOCK:
   142				inode->i_rdev = 0;
   143				break;
   144			default:
   145				goto bogusimode;
   146			}
   147			i_uid_write(inode, le16_to_cpu(dic->i_uid));
   148			i_gid_write(inode, le16_to_cpu(dic->i_gid));
   149			set_nlink(inode, le16_to_cpu(dic->i_nlink));
   150	
   151			/* use build time for compact inodes */
   152			inode_set_ctime(inode, sbi->build_time, sbi->build_time_nsec);
   153	
   154			inode->i_size = le32_to_cpu(dic->i_size);
   155			if (erofs_inode_is_data_compressed(vi->datalayout))
   156				nblks = le32_to_cpu(dic->i_u.compressed_blocks);
   157			else if (vi->datalayout == EROFS_INODE_CHUNK_BASED)
   158				vi->chunkformat = le16_to_cpu(dic->i_u.c.format);
   159			break;
   160		default:
   161			erofs_err(inode->i_sb,
   162				  "unsupported on-disk inode version %u of nid %llu",
   163				  erofs_inode_version(ifmt), vi->nid);
   164			err = -EOPNOTSUPP;
   165			goto err_out;
   166		}
   167	
   168		if (vi->datalayout == EROFS_INODE_CHUNK_BASED) {
   169			if (vi->chunkformat & ~EROFS_CHUNK_FORMAT_ALL) {
   170				erofs_err(inode->i_sb,
   171					  "unsupported chunk format %x of nid %llu",
   172					  vi->chunkformat, vi->nid);
   173				err = -EOPNOTSUPP;
   174				goto err_out;
   175			}
   176			vi->chunkbits = sb->s_blocksize_bits +
   177				(vi->chunkformat & EROFS_CHUNK_FORMAT_BLKBITS_MASK);
   178		}
   179		inode_set_mtime_to_ts(inode,
   180				      inode_set_atime_to_ts(inode, inode_get_ctime(inode)));
   181	
   182		inode->i_flags &= ~S_DAX;
   183		if (test_opt(&sbi->opt, DAX_ALWAYS) && S_ISREG(inode->i_mode) &&
   184		    (vi->datalayout == EROFS_INODE_FLAT_PLAIN ||
   185		     vi->datalayout == EROFS_INODE_CHUNK_BASED))
   186			inode->i_flags |= S_DAX;
   187	
   188		if (!nblks)
   189			/* measure inode.i_blocks as generic filesystems */
   190			inode->i_blocks = round_up(inode->i_size, sb->s_blocksize) >> 9;
   191		else
   192			inode->i_blocks = nblks << (sb->s_blocksize_bits - 9);
   193		return kaddr;
   194	
   195	bogusimode:
   196		erofs_err(inode->i_sb, "bogus i_mode (%o) @ nid %llu",
   197			  inode->i_mode, vi->nid);
   198		err = -EFSCORRUPTED;
   199	err_out:
   200		DBG_BUGON(1);
   201		erofs_put_metabuf(buf);
   202		return ERR_PTR(err);
   203	}
   204	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
Re: [PATCH -next V2] erofs: code clean up for function erofs_read_inode()
Posted by Gao Xiang 2 years, 1 month ago 
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@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);
>   }
Re: [PATCH -next V2] erofs: code clean up for function erofs_read_inode()
Posted by Zizhi Wo 2 years, 1 month ago 

在 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@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
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);
>>   }
>
Re: [PATCH -next V2] erofs: code clean up for function erofs_read_inode()
Posted by Gao Xiang 2 years, 1 month ago 

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@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
> 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
Re: [PATCH -next V2] erofs: code clean up for function erofs_read_inode()
Posted by Zizhi Wo 2 years, 1 month ago 

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

Patchew 2.1-devel-b67e4c2

© 2016 - 2025 Red Hat, Inc.