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

Commit Message

SF Markus Elfring Feb. 2, 2015, 3:23 p.m. UTC

From: Markus Elfring <[e-mail]>
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 <[e-mail]>
---
 drivers/md/dm-ioctl.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

Comments

Mike Snitzer Feb. 6, 2015, 9:12 p.m. UTC | #1

On Mon, Feb 02 2015 at 10:23am -0500,
SF Markus Elfring <[e-mail]> wrote:

> From: Markus Elfring <[e-mail]>
> 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.

--
dm-devel mailing list
[e-mail]
https://www.redhat.com/mailman/listinfo/dm-devel

SF Markus Elfring Feb. 8, 2015, 9:55 a.m. UTC | #2

> 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

--
dm-devel mailing list
[e-mail]
https://www.redhat.com/mailman/listinfo/dm-devel

Mike Snitzer Feb. 8, 2015, 2:02 p.m. UTC | #3

On Sun, Feb 8, 2015 at 4:55 AM, SF Markus Elfring
<[e-mail]> 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.

--
dm-devel mailing list
[e-mail]
https://www.redhat.com/mailman/listinfo/dm-devel

SF Markus Elfring Feb. 8, 2015, 3:52 p.m. UTC | #4

> 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

--
dm-devel mailing list
[e-mail]
https://www.redhat.com/mailman/listinfo/dm-devel

Patch

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);