Re: [B.A.T.M.A.N.] [PATCHv3] batman-adv: Introduce forward packet creation helper

[Sven Eckelmann <sven@narfation.org>]

[-- Attachment #1: Type: text/plain, Size: 3065 bytes --]

On Tuesday 14 June 2016 23:19:43 Linus Lüssing wrote:
[...]
> +struct batadv_forw_packet *
> +batadv_forw_packet_alloc(struct batadv_hard_iface *if_incoming,
> +			 struct batadv_hard_iface *if_outgoing,
> +			 atomic_t *queue_left,
> +			 struct batadv_priv *bat_priv)
> +{
> +	struct batadv_forw_packet *forw_packet = NULL;
> +
> +	if (queue_left && !batadv_atomic_dec_not_zero(queue_left)) {
> +		if (queue_left == &bat_priv->bcast_queue_left)
> +			batadv_dbg(BATADV_DBG_BATMAN, bat_priv,
> +				   "bcast queue full\n");
> +		else if (queue_left == &bat_priv->batman_queue_left)
> +			batadv_dbg(BATADV_DBG_BATMAN, bat_priv,
> +				   "batman queue full\n");
> +		else
> +			batadv_dbg(BATADV_DBG_BATMAN, bat_priv,
> +				   "a mysterious queue is full\n");
> +		goto out;
> +	}

Returning here directly is most likely easier to read. At least I had to
check twice if the gotos in this function are all correct.

And I would have written it slightly different. I don't want to say that your
way is worse or my version is better but just add a slightly different way
into the discussion

    	struct batadv_forw_packet *forw_packet;
    	const char *qname;
    
    	if (queue_left && !batadv_atomic_dec_not_zero(queue_left)) {
    		qname = "unknown";
    
    		if (queue_left == &bat_priv->bcast_queue_left)
    			qname = "bcast";
    
    		if (queue_left == &bat_priv->batman_queue_left)
    			qname = "batman";
    
    		batadv_dbg(BATADV_DBG_BATMAN, bat_priv, "%s queue is full\n",
    			  qname);
    
    		return NULL;
    	}

> +       goto out;
> +
> +err:
> +       if (queue_left)
> +               atomic_inc(queue_left);
> +out:
> +       return forw_packet;
> +}

Maybe we can return here directly forw_packet and in the error case
return NULL;

    	return forw_packet;

    err_inc_queue_left:
    	if (queue_left)
    		atomic_inc(queue_left);
    
    	return NULL;


> @@ -478,20 +546,16 @@ int batadv_add_bcast_packet_to_list(struct batadv_priv *bat_priv,
>  	struct batadv_bcast_packet *bcast_packet;
>  	struct sk_buff *newskb;
>  
> -	if (!batadv_atomic_dec_not_zero(&bat_priv->bcast_queue_left)) {
> -		batadv_dbg(BATADV_DBG_BATMAN, bat_priv,
> -			   "bcast packet queue full\n");
> -		goto out;
> -	}
> -
>  	primary_if = batadv_primary_if_get_selected(bat_priv);
>  	if (!primary_if)
> -		goto out_and_inc;
> -
> -	forw_packet = kmalloc(sizeof(*forw_packet), GFP_ATOMIC);
> +		goto out;

I think you can return here directly

>  
> +	forw_packet = batadv_forw_packet_alloc(primary_if, NULL,
> +					       &bat_priv->bcast_queue_left,
> +					       bat_priv);
> +	batadv_hardif_put(primary_if);
>  	if (!forw_packet)
> -		goto out_and_inc;
> +		goto out;

Same here


All the mentioned things are just nitpicking. So overall:

Reviewed-by: Sven Eckelmann <sven@narfation.org>

Please keep the Reviewed-by in case you you resent the patch and only
include changes which were mentioned in this mail.

Kind regards,
	Sven

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 819 bytes --]