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

["Linus Lüssing" <linus.luessing@c0d3.blue>]

On Sun, Jun 19, 2016 at 01:00:18PM +0200, Sven Eckelmann wrote:
> 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

Looks good to me, I like all your suggestions for
forw_packet_alloc() :).

> > @@ -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

Regarding these two goto's, usually using a "return" directly instead of a
"goto out" is better, I agree. For these two cases I think I would
prefer keeping it that way, because it's not a simple return but a
return of a macro value. Having NETDEV_TX_OK and NETDEV_TX_BUSY
each only once in a function makes it easier to spot the good or
bad cases, I think?

Or maybe I could rename "out" and "packet_free" to "err" and
"err_packet_free" to make the error cases even more visible?

Regards, Linus