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

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