From: Markus Elfring <[email protected]>
Date: Mon, 2 Feb 2015 15:51:43 +0100
Further update suggestions were taken into account together with results
from static source code analysis.
Markus Elfring (3):
dm snapshot: Deletion of unnecessary checks before the function call "vfree"
md/bitmap: Delete an unnecessary check before the function call "kfree"
dm ioctl: Delete an unnecessary check before the function call "dm_table_destroy"
drivers/md/bitmap.c | 10 +++++-----
drivers/md/dm-ioctl.c | 3 +--
drivers/md/dm-snap-persistent.c | 14 ++++----------
3 files changed, 10 insertions(+), 17 deletions(-)
--
2.2.2
From: Markus Elfring <[email protected]>
Date: Mon, 2 Feb 2015 14:38:29 +0100
The vfree() function performs also input parameter validation.
Thus the test around the call is not needed.
This issue was detected by using the Coccinelle software.
Signed-off-by: Markus Elfring <[email protected]>
---
drivers/md/dm-snap-persistent.c | 14 ++++----------
1 file changed, 4 insertions(+), 10 deletions(-)
diff --git a/drivers/md/dm-snap-persistent.c b/drivers/md/dm-snap-persistent.c
index d6e8817..808b841 100644
--- a/drivers/md/dm-snap-persistent.c
+++ b/drivers/md/dm-snap-persistent.c
@@ -200,16 +200,11 @@ err_area:
static void free_area(struct pstore *ps)
{
- if (ps->area)
- vfree(ps->area);
+ vfree(ps->area);
ps->area = NULL;
-
- if (ps->zero_area)
- vfree(ps->zero_area);
+ vfree(ps->zero_area);
ps->zero_area = NULL;
-
- if (ps->header_area)
- vfree(ps->header_area);
+ vfree(ps->header_area);
ps->header_area = NULL;
}
@@ -605,8 +600,7 @@ static void persistent_dtr(struct dm_exception_store *store)
free_area(ps);
/* Allocated in persistent_read_metadata */
- if (ps->callbacks)
- vfree(ps->callbacks);
+ vfree(ps->callbacks);
kfree(ps);
}
--
2.2.2
From: Markus Elfring <[email protected]>
Date: Mon, 2 Feb 2015 15:10:57 +0100
The kfree() function tests whether its argument is NULL and then
returns immediately. Thus the test around the call is not needed.
* This issue was detected by using the Coccinelle software.
* Let us also move an assignment for the variable "pages" to the place
directly before it is really needed for a loop.
* Let us also move another kfree() call into a block which should belong
to a previous check for the variable "bp".
Signed-off-by: Markus Elfring <[email protected]>
---
drivers/md/bitmap.c | 10 +++++-----
1 file changed, 5 insertions(+), 5 deletions(-)
diff --git a/drivers/md/bitmap.c b/drivers/md/bitmap.c
index da3604e..47d72df 100644
--- a/drivers/md/bitmap.c
+++ b/drivers/md/bitmap.c
@@ -1586,15 +1586,15 @@ static void bitmap_free(struct bitmap *bitmap)
bitmap_file_unmap(&bitmap->storage);
bp = bitmap->counts.bp;
- pages = bitmap->counts.pages;
/* free all allocated memory */
-
- if (bp) /* deallocate the page memory */
+ if (bp) { /* deallocate the page memory */
+ pages = bitmap->counts.pages;
for (k = 0; k < pages; k++)
- if (bp[k].map && !bp[k].hijacked)
+ if (!bp[k].hijacked)
kfree(bp[k].map);
- kfree(bp);
+ kfree(bp);
+ }
kfree(bitmap);
}
--
2.2.2
From: Markus Elfring <[email protected]>
Date: Mon, 2 Feb 2015 15:30:37 +0100
The dm_table_destroy() function tests whether its argument is NULL and then
returns immediately. Thus the test around the call is not needed.
This issue was detected by using the Coccinelle software.
Signed-off-by: Markus Elfring <[email protected]>
---
drivers/md/dm-ioctl.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/drivers/md/dm-ioctl.c b/drivers/md/dm-ioctl.c
index 73f791b..4fac6cf 100644
--- a/drivers/md/dm-ioctl.c
+++ b/drivers/md/dm-ioctl.c
@@ -1053,8 +1053,7 @@ static int do_resume(struct dm_ioctl *param)
* Since dm_swap_table synchronizes RCU, nobody should be in
* read-side critical section already.
*/
- if (old_map)
- dm_table_destroy(old_map);
+ dm_table_destroy(old_map);
if (!r)
__dev_status(md, param);
--
2.2.2
On Mon, 02 Feb 2015 16:20:42 +0100 SF Markus Elfring
<[email protected]> wrote:
> From: Markus Elfring <[email protected]>
> Date: Mon, 2 Feb 2015 15:10:57 +0100
>
> The kfree() function tests whether its argument is NULL and then
> returns immediately. Thus the test around the call is not needed.
>
> * This issue was detected by using the Coccinelle software.
>
> * Let us also move an assignment for the variable "pages" to the place
> directly before it is really needed for a loop.
>
> * Let us also move another kfree() call into a block which should belong
> to a previous check for the variable "bp".
>
> Signed-off-by: Markus Elfring <[email protected]>
> ---
> drivers/md/bitmap.c | 10 +++++-----
> 1 file changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/md/bitmap.c b/drivers/md/bitmap.c
> index da3604e..47d72df 100644
> --- a/drivers/md/bitmap.c
> +++ b/drivers/md/bitmap.c
> @@ -1586,15 +1586,15 @@ static void bitmap_free(struct bitmap *bitmap)
> bitmap_file_unmap(&bitmap->storage);
>
> bp = bitmap->counts.bp;
> - pages = bitmap->counts.pages;
>
> /* free all allocated memory */
> -
> - if (bp) /* deallocate the page memory */
> + if (bp) { /* deallocate the page memory */
> + pages = bitmap->counts.pages;
> for (k = 0; k < pages; k++)
> - if (bp[k].map && !bp[k].hijacked)
> + if (!bp[k].hijacked)
> kfree(bp[k].map);
> - kfree(bp);
> + kfree(bp);
> + }
> kfree(bitmap);
> }
>
Hi,
I'm somewhat amused that you removed a test for one kfree, but imposed a
test on another. I realised the second test was already there, but why not
just:
diff --git a/drivers/md/bitmap.c b/drivers/md/bitmap.c
index da3604e73e8a..ad13b2e1bf1f 100644
--- a/drivers/md/bitmap.c
+++ b/drivers/md/bitmap.c
@@ -1592,7 +1592,7 @@ static void bitmap_free(struct bitmap *bitmap)
if (bp) /* deallocate the page memory */
for (k = 0; k < pages; k++)
- if (bp[k].map && !bp[k].hijacked)
+ if (!bp[k].hijacked)
kfree(bp[k].map);
kfree(bp);
kfree(bitmap);
It makes the intention of the patch much clearer.
I'd probably prefer to leave the code as it is. I don't think either patch
is really an improvement in readability, and readability trumps performance
in places like this.
Thanks,
NeilBrown
On Mon, Feb 02 2015 at 10:23am -0500,
SF Markus Elfring <[email protected]> wrote:
> From: Markus Elfring <[email protected]>
> Date: Mon, 2 Feb 2015 15:30:37 +0100
>
> The dm_table_destroy() function tests whether its argument is NULL and then
> returns immediately. Thus the test around the call is not needed.
>
> This issue was detected by using the Coccinelle software.
Your proposed patch (while technically correct) hurts code clarity.
Nack.
> Your proposed patch (while technically correct) hurts code clarity.
How many source code readability and understanding challenges does each
additional condition check cause?
Can the affected place become also a bit more efficient?
Regards,
Markus
On Sun, Feb 8, 2015 at 4:55 AM, SF Markus Elfring
<[email protected]> wrote:
>> Your proposed patch (while technically correct) hurts code clarity.
>
> How many source code readability and understanding challenges does each
> additional condition check cause?
Please don't make a mountain out of a mole hill in an attempt to
defend your robotic patch (I'm quite tired of some of these static
analyzer patch submissions).
FYI, I did stage your other patch for 3.20, see:
https://git.kernel.org/cgit/linux/kernel/git/device-mapper/linux-dm.git/commit/?h=dm-for-3.20&id=d0ce7e911c97c7c6df1081dcedfefced82a0c6bf
> Can the affected place become also a bit more efficient?
Efficiency isn't a concern in this instance (it isn't a hot IO path).
And even if it were, a branch (with current code) is more efficient vs
a a jump + branch (your proposed patch) -- in the case that no active
table exists. Now if it likely that old_map does exist then yes your
patch is always a very slight win.
But given the duality of the calling function (deals with loading a
new map and destroying the old map if it exists) I prefer to keep the
code as is. Sorry.
> FYI, I did stage your other patch for 3.20, see:
> https://git.kernel.org/cgit/linux/kernel/git/device-mapper/linux-dm.git/commit/?h=dm-for-3.20&id=d0ce7e911c97c7c6df1081dcedfefced82a0c6bf
Thanks for your acceptance of the suggested clean-up around
vfree() function calls at least.
Additional source code places can also be reconsidered at other times,
can't they?
Regards,
Markus