Messages in this thread Patch in this message | | | From | Andreas Dilger <> | Subject | Re: [PATCH] ext4: extents: Remove unnecessary ‘NULL’ values from ablocks | Date | Mon, 8 Apr 2024 14:51:23 -0600 |
| |
On Apr 1, 2024, at 8:48 PM, Li zeming <zeming@nfschina.com> wrote: > > ablocks is assigned first, so it does not need to initialize the > assignment.
While it is true that "ablocks" is currently set before use, this is happening a long way away from the variable declaration and also "ablocks" is used after the "cleanup:" label error case:
cleanup: if (bh) { if (buffer_locked(bh)) unlock_buffer(bh); brelse(bh); }
if (err) { /* free all allocated blocks in error case */ for (i = 0; i < depth; i++) { if (!ablocks[i]) continue; ext4_free_blocks(handle, inode, NULL, ablocks[i], 1, EXT4_FREE_BLOCKS_METADATA); } } kfree(ablocks);
So there is definitely a risk that a code change in the future would introduce hard-to-debug problems, crashes, or even just spurious static code analysis warnings. My recommendation would be to keep this 1-cycle local variable initialization in place rather than spend hours or days trying to debug and fix a crash here in the future.
Cheers, Andreas
> > Signed-off-by: Li zeming <zeming@nfschina.com> > --- > fs/ext4/extents.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/fs/ext4/extents.c b/fs/ext4/extents.c > index 4ab96f01a6f31..caace8c3fd3c1 100644 > --- a/fs/ext4/extents.c > +++ b/fs/ext4/extents.c > @@ -1061,7 +1061,7 @@ static int ext4_ext_split(handle_t *handle, struct inode *inode, > int i = at, k, m, a; > ext4_fsblk_t newblock, oldblock; > __le32 border; > - ext4_fsblk_t *ablocks = NULL; /* array of allocated blocks */ > + ext4_fsblk_t *ablocks; /* array of allocated blocks */ > gfp_t gfp_flags = GFP_NOFS; > int err = 0; > size_t ext_size = 0; > -- > 2.18.2 >
Cheers, Andreas
[unhandled content-type:application/pgp-signature] | |