Re: [B.A.T.M.A.N.] [PATCH 2/5] batman-adv: clear ADD+DEL (and viceversa) events in the same orig-interval

[Antonio Quartulli <ordex@autistici.org>]

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

On Wed, Apr 18, 2012 at 10:33:34AM +0200, Martin Hundebøll wrote:
> Hi Antonio,
> 
> On 04/18/2012 12:27 AM, Antonio Quartulli wrote:
> > +	/* check for ADD+DEL or DEL+ADD events */
> >   	spin_lock_bh(&bat_priv->tt_changes_list_lock);
> > +	list_for_each_entry_safe(entry, safe,&bat_priv->tt_changes_list,
> > +				 list) {
> > +		if (!compare_eth(entry->change.addr, addr))
> > +			continue;
> 
> Please add an empty line here.

Is this really needed for some specific reason?

> 
> > +		if (!(!(flags&  TT_CLIENT_DEL)&&  /* ADD op */
> > +		      entry->change.flags&  TT_CLIENT_DEL)&&
> > +		    !(flags&  TT_CLIENT_DEL&&
> > +		      !(entry->change.flags&  TT_CLIENT_DEL))) /* ADD op */
> > +			continue;
> 
> This is messy and hard to unerstand. Couldn't you use some tmp vars like this:
> 
> int local_del = (flags & TT_CLIENT_DEL) == TT_CLIENT_DEL;
> int change_del = (entry->change.flags & TT_CLIENT_DEL) == TT_CLIENT_DEL;
> 
> if (local_del == change_del)
> 	continue;
> 
> I'm not 100% sure I understood the original if correctly, but that just proofs the need to rework it :)

eheh, I know I like to mess up the code with boolean formulas :D
Thank you for your feedback, I will simplify it.

> 
> > +		/* DEL+ADD in the same orig interval have no effect and can be
> > +		 * removed to avoid silly behaviour on the receiver side. The
> > +		 * other way around (ADD+DEL) can happen in case of roaming of
> > +		 * a client still in the NEW state. Roaming of NEW clients is
> > +		 * now possible due to automatically recognition of "temporary"
> > +		 * clients */
> 
> Remember newline for */ :)

yes .-.


Thanks again!


-- 
Antonio Quartulli

..each of us alone is worth nothing..
Ernesto "Che" Guevara

[-- Attachment #2: Type: application/pgp-signature, Size: 490 bytes --]