Re: [PATCH 2/3] net/netfilter: refactor notifier registration

[Eric Dumazet <eric.dumazet@xxxxxxxxx>]

Le mercredi 22 février 2012 à 10:48 +0400, Tony Zelenoff a écrit :
> * ret variable initialization removed as useless
> * Similar code strings concatenated and functions code
>   flow became more plain
> 
> Signed-off-by: Tony Zelenoff <antonz@xxxxxxxxxxxxx>
> ---
>  net/netfilter/nf_conntrack_ecache.c |   26 ++++++++++----------------
>  1 files changed, 10 insertions(+), 16 deletions(-)
> 
> diff --git a/net/netfilter/nf_conntrack_ecache.c b/net/netfilter/nf_conntrack_ecache.c
> index aa15977..9b8e986 100644
> --- a/net/netfilter/nf_conntrack_ecache.c
> +++ b/net/netfilter/nf_conntrack_ecache.c
> @@ -81,21 +81,18 @@ EXPORT_SYMBOL_GPL(nf_ct_deliver_cached_events);
>  int nf_conntrack_register_notifier(struct net *net,
>  				   struct nf_ct_event_notifier *new)
>  {
> -	int ret = 0;
> +	int ret;
>  	struct nf_ct_event_notifier *notify;
>  
>  	mutex_lock(&nf_ct_ecache_mutex);
>  	notify = rcu_dereference_protected(net->ct.nf_conntrack_event_cb,
>  					   lockdep_is_held(&nf_ct_ecache_mutex));
> -	if (notify != NULL) {
> +	if (likely(!notify)) {
> +		rcu_assign_pointer(net->ct.nf_conntrack_event_cb, new);
> +		ret = 0;
> +	} else
>  		ret = -EBUSY;
> -		goto out_unlock;
> -	}
> -	rcu_assign_pointer(net->ct.nf_conntrack_event_cb, new);
> -	mutex_unlock(&nf_ct_ecache_mutex);
> -	return ret;
>  
> -out_unlock:
>  	mutex_unlock(&nf_ct_ecache_mutex);
>  	return ret;
>  }
> @@ -118,21 +115,18 @@ EXPORT_SYMBOL_GPL(nf_conntrack_unregister_notifier);
>  int nf_ct_expect_register_notifier(struct net *net,
>  				   struct nf_exp_event_notifier *new)
>  {
> -	int ret = 0;
> +	int ret;
>  	struct nf_exp_event_notifier *notify;
>  
>  	mutex_lock(&nf_ct_ecache_mutex);
>  	notify = rcu_dereference_protected(net->ct.nf_expect_event_cb,
>  					   lockdep_is_held(&nf_ct_ecache_mutex));
> -	if (notify != NULL) {
> +	if (likely(!notify)) {
> +		rcu_assign_pointer(net->ct.nf_expect_event_cb, new);
> +		ret = 0;
> +	} else
>  		ret = -EBUSY;
> -		goto out_unlock;
> -	}
> -	rcu_assign_pointer(net->ct.nf_expect_event_cb, new);
> -	mutex_unlock(&nf_ct_ecache_mutex);
> -	return ret;
>  
> -out_unlock:
>  	mutex_unlock(&nf_ct_ecache_mutex);
>  	return ret;
>  }

Please leave the code as is, I find it more readable.

It is standard coding practice, and permits stacking of new init code,
with proper error path.

Dont add likely()/unlikely() clauses in slow path, this obfuscate code
for litle gain.




--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html