From: Markus Elfring elfring@users.sourceforge.net Date: Tue, 3 Nov 2015 21:34:29 +0100
Further update suggestions were taken into account after a patch was applied from static source code analysis.
Markus Elfring (3): Delete an unnecessary check before the function call "batadv_softif_vlan_free_ref" Split a condition check Less function calls in batadv_is_ap_isolated() after error detection
net/batman-adv/translation-table.c | 29 ++++++++++++++--------------- 1 file changed, 14 insertions(+), 15 deletions(-)
From: Markus Elfring elfring@users.sourceforge.net Date: Tue, 3 Nov 2015 19:20:34 +0100
The batadv_softif_vlan_free_ref() 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 --- net/batman-adv/translation-table.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/net/batman-adv/translation-table.c b/net/batman-adv/translation-table.c index 4228b10..48315de 100644 --- a/net/batman-adv/translation-table.c +++ b/net/batman-adv/translation-table.c @@ -3336,8 +3336,7 @@ bool batadv_is_ap_isolated(struct batadv_priv *bat_priv, u8 *src, u8 *dst, ret = true;
out: - if (vlan) - batadv_softif_vlan_free_ref(vlan); + batadv_softif_vlan_free_ref(vlan); if (tt_global_entry) batadv_tt_global_entry_free_ref(tt_global_entry); if (tt_local_entry)
On Tuesday, November 03, 2015 21:52:58 SF Markus Elfring wrote:
From: Markus Elfring elfring@users.sourceforge.net Date: Tue, 3 Nov 2015 19:20:34 +0100
The batadv_softif_vlan_free_ref() 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
net/batman-adv/translation-table.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-)
Applied in revision bbcbe0f.
Thanks, Marek
From: Markus Elfring elfring@users.sourceforge.net Date: Tue, 3 Nov 2015 20:41:02 +0100
Let us split a check for a condition at the beginning of the batadv_is_ap_isolated() function so that a direct return can be performed in this function if the variable "vlan" contained a null pointer.
Signed-off-by: Markus Elfring elfring@users.sourceforge.net --- net/batman-adv/translation-table.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/net/batman-adv/translation-table.c b/net/batman-adv/translation-table.c index 48315de..965a004 100644 --- a/net/batman-adv/translation-table.c +++ b/net/batman-adv/translation-table.c @@ -3319,7 +3319,10 @@ bool batadv_is_ap_isolated(struct batadv_priv *bat_priv, u8 *src, u8 *dst, bool ret = false;
vlan = batadv_softif_vlan_get(bat_priv, vid); - if (!vlan || !atomic_read(&vlan->ap_isolation)) + if (!vlan) + return false; + + if (!atomic_read(&vlan->ap_isolation)) goto out;
tt_local_entry = batadv_tt_local_hash_find(bat_priv, dst, vid);
On Tuesday, November 03, 2015 21:54:35 SF Markus Elfring wrote:
From: Markus Elfring elfring@users.sourceforge.net Date: Tue, 3 Nov 2015 20:41:02 +0100
Let us split a check for a condition at the beginning of the batadv_is_ap_isolated() function so that a direct return can be performed in this function if the variable "vlan" contained a null pointer.
Signed-off-by: Markus Elfring elfring@users.sourceforge.net
net/batman-adv/translation-table.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)
Applied in revision b1199c6.
Thanks, Marek
From: Markus Elfring elfring@users.sourceforge.net Date: Tue, 3 Nov 2015 21:10:51 +0100
The variables "tt_local_entry" and "tt_global_entry" were eventually checked again despite of a corresponding null pointer test before. Let us avoid this double check by reordering a function call sequence and the better selection of jump targets.
Signed-off-by: Markus Elfring elfring@users.sourceforge.net --- net/batman-adv/translation-table.c | 21 +++++++++------------ 1 file changed, 9 insertions(+), 12 deletions(-)
diff --git a/net/batman-adv/translation-table.c b/net/batman-adv/translation-table.c index 965a004..3ac32d9 100644 --- a/net/batman-adv/translation-table.c +++ b/net/batman-adv/translation-table.c @@ -3323,27 +3323,24 @@ bool batadv_is_ap_isolated(struct batadv_priv *bat_priv, u8 *src, u8 *dst, return false;
if (!atomic_read(&vlan->ap_isolation)) - goto out; + goto vlan_free;
tt_local_entry = batadv_tt_local_hash_find(bat_priv, dst, vid); if (!tt_local_entry) - goto out; + goto vlan_free;
tt_global_entry = batadv_tt_global_hash_find(bat_priv, src, vid); if (!tt_global_entry) - goto out; + goto local_entry_free;
- if (!_batadv_is_ap_isolated(tt_local_entry, tt_global_entry)) - goto out; - - ret = true; + if (_batadv_is_ap_isolated(tt_local_entry, tt_global_entry)) + ret = true;
-out: + batadv_tt_global_entry_free_ref(tt_global_entry); +local_entry_free: + batadv_tt_local_entry_free_ref(tt_local_entry); +vlan_free: batadv_softif_vlan_free_ref(vlan); - if (tt_global_entry) - batadv_tt_global_entry_free_ref(tt_global_entry); - if (tt_local_entry) - batadv_tt_local_entry_free_ref(tt_local_entry); return ret; }
On 04/11/15 04:56, SF Markus Elfring wrote:
From: Markus Elfring elfring@users.sourceforge.net Date: Tue, 3 Nov 2015 21:10:51 +0100
The variables "tt_local_entry" and "tt_global_entry" were eventually checked again despite of a corresponding null pointer test before. Let us avoid this double check by reordering a function call sequence and the better selection of jump targets.
Signed-off-by: Markus Elfring elfring@users.sourceforge.net
net/batman-adv/translation-table.c | 21 +++++++++------------ 1 file changed, 9 insertions(+), 12 deletions(-)
diff --git a/net/batman-adv/translation-table.c b/net/batman-adv/translation-table.c index 965a004..3ac32d9 100644 --- a/net/batman-adv/translation-table.c +++ b/net/batman-adv/translation-table.c @@ -3323,27 +3323,24 @@ bool batadv_is_ap_isolated(struct batadv_priv *bat_priv, u8 *src, u8 *dst, return false;
if (!atomic_read(&vlan->ap_isolation))
goto out;
goto vlan_free;tt_local_entry = batadv_tt_local_hash_find(bat_priv, dst, vid); if (!tt_local_entry)
goto out;
goto vlan_free;tt_global_entry = batadv_tt_global_hash_find(bat_priv, src, vid); if (!tt_global_entry)
goto out;
goto local_entry_free;
- if (!_batadv_is_ap_isolated(tt_local_entry, tt_global_entry))
goto out;- ret = true;
- if (_batadv_is_ap_isolated(tt_local_entry, tt_global_entry))
ret = true;-out:
- batadv_tt_global_entry_free_ref(tt_global_entry);
+local_entry_free:
- batadv_tt_local_entry_free_ref(tt_local_entry);
+vlan_free: batadv_softif_vlan_free_ref(vlan);
- if (tt_global_entry)
batadv_tt_global_entry_free_ref(tt_global_entry);- if (tt_local_entry)
batadv_tt_local_entry_free_ref(tt_local_entry);
Markus, if you really want to make this codestyle change, I'd suggest you to go through the whole batman-adv code and apply the same change where needed. It does not make sense to change the codestyle in one spot only.
On top of that, by going through the batman-adv code you might agree that the current style is actually not a bad idea.
Cheers,
-out:
- batadv_tt_global_entry_free_ref(tt_global_entry);
+local_entry_free:
- batadv_tt_local_entry_free_ref(tt_local_entry);
+vlan_free: batadv_softif_vlan_free_ref(vlan);
- if (tt_global_entry)
batadv_tt_global_entry_free_ref(tt_global_entry);- if (tt_local_entry)
batadv_tt_local_entry_free_ref(tt_local_entry);
if you really want to make this codestyle change, I'd suggest you to go through the whole batman-adv code and apply the same change where needed.
Thanks for your interest in similar source code changes.
I would prefer general acceptance for this specific update suggestion before I might invest further software development efforts for the affected network module.
It does not make sense to change the codestyle in one spot only.
I agree in the way that I would be nice if more places can still be improved.
On top of that, by going through the batman-adv code you might agree that the current style is actually not a bad idea.
I got the impression that the current Linux coding style convention disagrees around the affected jump label selection to some degree, doesn't it?
Regards, Markus
On Friday 20 November 2015 11:56:39 SF Markus Elfring wrote:
On top of that, by going through the batman-adv code you might agree that the current style is actually not a bad idea.
I got the impression that the current Linux coding style convention disagrees around the affected jump label selection to some degree, doesn't it?
Yes, see "Chapter 7: Centralized exiting of functions". But your patch doesn't seem to apply anymore. Can you please resent it or mark it correctly in patchwork [1].
There was also another suggestion in the past:
* https://patchwork.open-mesh.org/patch/4081/ [2]
I have prepared a overview of functions and their goto's [3] to make it easier to spot interesting places.
Kind regards, Sven
[1] https://patchwork.open-mesh.org/patch/4724/ You can find an updated version at https://git.open-mesh.org/batman-adv.git/patch/1b79cb12821da928b4cf2d116469d... [2] Update version: https://git.open-mesh.org/batman-adv.git/commit/28578c9dd0e592f1c69c44ab7723... [3] https://git.open-mesh.org/batman-adv.git/blob/783c827d01b3644ac09a0f32fe4a9d...
From: Markus Elfring elfring@users.sourceforge.net Date: Fri, 11 Mar 2016 13:10:20 +0100
The variables "tt_local_entry" and "tt_global_entry" were eventually checked again despite of a corresponding null pointer test before.
* Avoid this double check by reordering a function call sequence and the better selection of jump targets.
* Omit the initialisation for these variables at the beginning then.
Signed-off-by: Markus Elfring elfring@users.sourceforge.net --- net/batman-adv/translation-table.c | 25 +++++++++++-------------- 1 file changed, 11 insertions(+), 14 deletions(-)
diff --git a/net/batman-adv/translation-table.c b/net/batman-adv/translation-table.c index 0b43e86..9c0193ee 100644 --- a/net/batman-adv/translation-table.c +++ b/net/batman-adv/translation-table.c @@ -3403,8 +3403,8 @@ void batadv_tt_local_commit_changes(struct batadv_priv *bat_priv) bool batadv_is_ap_isolated(struct batadv_priv *bat_priv, u8 *src, u8 *dst, unsigned short vid) { - struct batadv_tt_local_entry *tt_local_entry = NULL; - struct batadv_tt_global_entry *tt_global_entry = NULL; + struct batadv_tt_local_entry *tt_local_entry; + struct batadv_tt_global_entry *tt_global_entry; struct batadv_softif_vlan *vlan; bool ret = false;
@@ -3413,27 +3413,24 @@ bool batadv_is_ap_isolated(struct batadv_priv *bat_priv, u8 *src, u8 *dst, return false;
if (!atomic_read(&vlan->ap_isolation)) - goto out; + goto vlan_put;
tt_local_entry = batadv_tt_local_hash_find(bat_priv, dst, vid); if (!tt_local_entry) - goto out; + goto vlan_put;
tt_global_entry = batadv_tt_global_hash_find(bat_priv, src, vid); if (!tt_global_entry) - goto out; - - if (!_batadv_is_ap_isolated(tt_local_entry, tt_global_entry)) - goto out; + goto local_entry_put;
- ret = true; + if (_batadv_is_ap_isolated(tt_local_entry, tt_global_entry)) + ret = true;
-out: + batadv_tt_global_entry_put(tt_global_entry); +local_entry_put: + batadv_tt_local_entry_put(tt_local_entry); +vlan_put: batadv_softif_vlan_put(vlan); - if (tt_global_entry) - batadv_tt_global_entry_put(tt_global_entry); - if (tt_local_entry) - batadv_tt_local_entry_put(tt_local_entry); return ret; }
From: SF Markus Elfring elfring@users.sourceforge.net Date: Fri, 11 Mar 2016 13:40:56 +0100
From: Markus Elfring elfring@users.sourceforge.net Date: Fri, 11 Mar 2016 13:10:20 +0100
The variables "tt_local_entry" and "tt_global_entry" were eventually checked again despite of a corresponding null pointer test before.
Avoid this double check by reordering a function call sequence and the better selection of jump targets.
Omit the initialisation for these variables at the beginning then.
Signed-off-by: Markus Elfring elfring@users.sourceforge.net
I am assuming Antonio will take this in via his tree.
On Mon, Mar 14, 2016 at 03:25:02PM -0400, David Miller wrote:
From: SF Markus Elfring elfring@users.sourceforge.net Date: Fri, 11 Mar 2016 13:40:56 +0100
From: Markus Elfring elfring@users.sourceforge.net Date: Fri, 11 Mar 2016 13:10:20 +0100
The variables "tt_local_entry" and "tt_global_entry" were eventually checked again despite of a corresponding null pointer test before.
Avoid this double check by reordering a function call sequence and the better selection of jump targets.
Omit the initialisation for these variables at the beginning then.
Signed-off-by: Markus Elfring elfring@users.sourceforge.net
I am assuming Antonio will take this in via his tree.
Yeah, it will go through our tree. Still under review right now.
Cheers,
On Friday 11 March 2016 13:40:56 SF Markus Elfring wrote:
From: Markus Elfring elfring@users.sourceforge.net Date: Fri, 11 Mar 2016 13:10:20 +0100
The variables "tt_local_entry" and "tt_global_entry" were eventually checked again despite of a corresponding null pointer test before.
Avoid this double check by reordering a function call sequence and the better selection of jump targets.
Omit the initialisation for these variables at the beginning then.
Signed-off-by: Markus Elfring elfring@users.sourceforge.net
Reviewed-by: Sven Eckelmann sven@narfation.org
See
* https://www.kernel.org/doc/Documentation/SubmittingPatches Chapter 7: Centralized exiting of functions * https://plus.google.com/106378716002406849458/posts/dnanfhQ4mHQ * http://article.gmane.org/gmane.linux.documentation/28552
Kind regards, Sven
On Freitag, 11. März 2016 13:40:56 CEST SF Markus Elfring wrote:
From: Markus Elfring elfring@users.sourceforge.net Date: Fri, 11 Mar 2016 13:10:20 +0100
The variables "tt_local_entry" and "tt_global_entry" were eventually checked again despite of a corresponding null pointer test before.
Avoid this double check by reordering a function call sequence and the better selection of jump targets.
Omit the initialisation for these variables at the beginning then.
Signed-off-by: Markus Elfring elfring@users.sourceforge.net Reviewed-by: Sven Eckelmann sven@narfation.org
net/batman-adv/translation-table.c | 25 +++++++++++-------------- 1 file changed, 11 insertions(+), 14 deletions(-)
Applied in 58d8ef2dd0c9f7274f0473a1b1d1ffdacdd914f6 [1].
Kind regards, Sven
[1] https://git.open-mesh.org/batman-adv.git/commit/58d8ef2dd0c9f7274f0473a1b1d1...
b.a.t.m.a.n@lists.open-mesh.org
-
Antonio Quartulli -
Antonio Quartulli -
David Miller -
Marek Lindner -
SF Markus Elfring -
Sven Eckelmann