[PATCH 1/1] bcache: Deletion of an unnecessary check before the function call "kobject_put"

[PATCH 1/1] bcache: Deletion of an unnecessary check before the function call "kobject_put"

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Wed, 19 Nov 2014 21:25:39 +0100

The kobject_put() 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 <elfring@users.sourceforge.net>
---
 drivers/md/bcache/super.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/md/bcache/super.c b/drivers/md/bcache/super.c
index d4713d0..1443c55 100644
--- a/drivers/md/bcache/super.c
+++ b/drivers/md/bcache/super.c
@@ -2076,8 +2076,7 @@ static void bcache_exit(void)
 {
 	bch_debug_exit();
 	bch_request_exit();
-	if (bcache_kobj)
-		kobject_put(bcache_kobj);
+	kobject_put(bcache_kobj);
 	if (bcache_wq)
 		destroy_workqueue(bcache_wq);
 	if (bcache_major)
-- 
2.1.3

[PATCH 1/1] dm: Deletion of unnecessary checks before two function calls

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Wed, 19 Nov 2014 21:48:10 +0100

The functions dm_table_destroy() and vfree() perform 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 <elfring@users.sourceforge.net>
---
 drivers/md/dm-ioctl.c           |  3 +--
 drivers/md/dm-snap-persistent.c | 12 ++++--------
 2 files changed, 5 insertions(+), 10 deletions(-)

diff --git a/drivers/md/dm-ioctl.c b/drivers/md/dm-ioctl.c
index 5152142..0b925a4 100644
--- a/drivers/md/dm-ioctl.c
+++ b/drivers/md/dm-ioctl.c
@@ -1050,8 +1050,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);
diff --git a/drivers/md/dm-snap-persistent.c b/drivers/md/dm-snap-persistent.c
index d6e8817..4b29bac 100644
--- a/drivers/md/dm-snap-persistent.c
+++ b/drivers/md/dm-snap-persistent.c
@@ -200,16 +200,13 @@ 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 +602,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.1.3

[PATCH 0/3] md: Deletion of a few unnecessary checks

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
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

[PATCH 1/3] dm snapshot: Deletion of unnecessary checks before the function call "vfree"

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
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 <elfring@users.sourceforge.net>
---
 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

[PATCH 2/3] md/bitmap: Delete an unnecessary check before the function call "kfree"

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
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 <elfring@users.sourceforge.net>
---
 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

[PATCH 3/3] dm ioctl: Delete an unnecessary check before the function call "dm_table_destroy"

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
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 <elfring@users.sourceforge.net>
---
 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

Re: [PATCH 2/3] md/bitmap: Delete an unnecessary check before the function call "kfree"

[NeilBrown]

[-- Attachment #1: Type: text/plain, Size: 2388 bytes --]

On Mon, 02 Feb 2015 16:20:42 +0100 SF Markus Elfring
<elfring@users.sourceforge.net> wrote:

> From: Markus Elfring <elfring@users.sourceforge.net>
> 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 <elfring@users.sourceforge.net>
> ---
>  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

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 811 bytes --]

Re: [PATCH 3/3] dm ioctl: Delete an unnecessary check before the function call "dm_table_destroy"

[Mike Snitzer]

On Mon, Feb 02 2015 at 10:23am -0500,
SF Markus Elfring <elfring@users.sourceforge.net> wrote:

> From: Markus Elfring <elfring@users.sourceforge.net>
> 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.

Re: [PATCH 3/3] dm ioctl: Delete an unnecessary check before the function call "dm_table_destroy"

[SF Markus Elfring]

> 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

Re: [PATCH 3/3] dm ioctl: Delete an unnecessary check before the function call "dm_table_destroy"

[Mike Snitzer]

On Sun, Feb 8, 2015 at 4:55 AM, SF Markus Elfring
<elfring@users.sourceforge.net> 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.

Re: [PATCH 3/3] dm ioctl: Delete an unnecessary check before the function call "dm_table_destroy"

[SF Markus Elfring]

> 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

Re: [PATCH] bcache: Deletion of an unnecessary check before the function call "kobject_put"

[SF Markus Elfring]

> From: Markus Elfring <elfring@users.sourceforge.net>
> Date: Wed, 19 Nov 2014 21:25:39 +0100
> 
> The kobject_put() 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 <elfring@users.sourceforge.net>
> ---
>  drivers/md/bcache/super.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/drivers/md/bcache/super.c b/drivers/md/bcache/super.c
> index d4713d0..1443c55 100644
> --- a/drivers/md/bcache/super.c
> +++ b/drivers/md/bcache/super.c
> @@ -2076,8 +2076,7 @@ static void bcache_exit(void)
>  {
>  	bch_debug_exit();
>  	bch_request_exit();
> -	if (bcache_kobj)
> -		kobject_put(bcache_kobj);
> +	kobject_put(bcache_kobj);
>  	if (bcache_wq)
>  		destroy_workqueue(bcache_wq);
>  	if (bcache_major)
> 

Would you like to integrate this update suggestion
into another source code repository?

Regards,
Markus