[3/3] batman-adv: Less function calls in batadv_is_ap_isolated() after error detection

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

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

>> -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 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/1b79cb12821da928b4cf2d116469dfdcbe66d8cd
[2] Update version:
    https://git.open-mesh.org/batman-adv.git/commit/28578c9dd0e592f1c69c44ab7723021b1e8c5b28
[3] https://git.open-mesh.org/batman-adv.git/blob/783c827d01b3644ac09a0f32fe4a9dfa8d6debd3:/TODO