| Message ID | 20240402024804.29411-1-zeming@nfschina.com |
|---|---|
| State | New |
| Headers | show |
| Series | ext4: extents: Remove unnecessary ‘NULL’ values from ablocks | expand |
On Tue, Apr 02, 2024 at 10:48:04AM +0800, Li zeming wrote: > ablocks is assigned first, so it does not need to initialize the > assignment. That's technically true, but the compiler is perfectly capable of optimizing it out. So it's harmless, and removing it does make the code a bit more fragile, since it needs to be set so that the cleanup code doesn't accidentally dereference an uninitialized pointer. Cheers, - Ted
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
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;
ablocks is assigned first, so it does not need to initialize the assignment. Signed-off-by: Li zeming <zeming@nfschina.com> --- fs/ext4/extents.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)