From: Markus Elfring <[email protected]>
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(-)
--
2.6.2
From: Markus Elfring <[email protected]>
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 <[email protected]>
---
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)
--
2.6.2
From: Markus Elfring <[email protected]>
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 <[email protected]>
---
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);
--
2.6.2
From: Markus Elfring <[email protected]>
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 <[email protected]>
---
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;
}
--
2.6.2
On 04/11/15 04:56, SF Markus Elfring wrote:
> From: Markus Elfring <[email protected]>
> 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 <[email protected]>
> ---
> 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;
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,
--
Antonio Quartulli
>> -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;
> 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 Tuesday, November 03, 2015 21:52:58 SF Markus Elfring wrote:
> From: Markus Elfring <[email protected]>
> 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 <[email protected]>
> ---
> net/batman-adv/translation-table.c | 3 +--
> 1 file changed, 1 insertion(+), 2 deletions(-)
Applied in revision bbcbe0f.
Thanks,
Marek
On Tuesday, November 03, 2015 21:54:35 SF Markus Elfring wrote:
> From: Markus Elfring <[email protected]>
> 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 <[email protected]>
> ---
> net/batman-adv/translation-table.c | 5 ++++-
> 1 file changed, 4 insertions(+), 1 deletion(-)
Applied in revision b1199c6.
Thanks,
Marek