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