Re: [B.A.T.M.A.N.] [RFC 02/11] batman-adv: add basic bridge loop avoidance code

[Simon Wunderlich <simon.wunderlich@s2003.tu-chemnitz.de>]

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

Hi,

On Tue, Nov 01, 2011 at 11:52:45AM +0100, Marek Lindner wrote:
> On Sunday, October 30, 2011 23:51:03 Simon Wunderlich wrote:
> > +static const uint8_t announce_mac[6] = {0x43, 0x05, 0x43, 0x05, 0x00,
> > 0x00};
> 
> All we ever use are 4 bytes - we could make this shorter. Otherwise please use 
> ETH_ALEN. Please use ETH_ALEN throughout the code instead of 6.
> 

OK, that's right, i'll change it to 4 and use ETH_ALEN at the other place where i missed it.
 
> > +       bat_dbg(DBG_BLA, bat_priv,
> > +               "handle_announce(): ANNOUNCE vid %d (sent "
> > +               "by %pM)... CRC = %04x (nw order)\n",
> > +               vid, backbone_gw->orig, crc);
> 
> It is not network byte order anymore ..
> 

OK

> +       /* TODO: we could cal something like tt_local_del() here. */
> 
> Why should we ?
> 
> 

Because when someone claims the adress, the the client has roamed to the mesh.

Usually this is handled by the tt roaming code, this will only fail in a limited
horizon scenario where a node does not know the other node the client roamed to.
In this case, the tt will time out after a long while.

This is just an idea/optimization to add the tt_local_del(). :)

> > @@ -634,7 +640,7 @@ static int batman_skb_recv(struct sk_buff *skb, struct
> > net_device *dev, case BAT_TT_QUERY:
> >                 ret = recv_tt_query(skb, hard_iface);
> >                 break;
> > -               /* Roaming advertisement */
> > +               /* bridge roop avoidance query */
> >         case BAT_ROAM_ADV:
> >                 ret = recv_roam_adv(skb, hard_iface);
> >                 break;
> 
> Small typo here but why are you even changing the comment ?
> 

Oh thanks, this is wrong. This comes from an earlier version of the BLA table
changes where I used a special packet type. 

The comment should not be changed.

> >         atomic_t gw_reselect;
> >         struct hard_iface __rcu *primary_if;  /* rcu protected pointer */
> >         struct vis_info *my_vis_info;
> > +       uint8_t own_orig[6];            /* cache primary hardifs address */
> >  };
> 
> I'd call this "primary_addr" instead of "own_orig" to be consistent with 
> "primary_if". Furthermore ETH_ALEN should be used for the length.
> 

OK.

> > +struct backbone_gw {
> > +       uint8_t orig[ETH_ALEN];
> > +       short vid;              /* used VLAN ID */
> > +       struct hlist_node hash_entry;
> > +       struct bat_priv *bat_priv;
> > +       unsigned long lasttime; /* last time we heard of this backbone gw */
> > +       atomic_t request_sent;
> > +       atomic_t refcount;
> > +       struct rcu_head rcu;
> > +       uint16_t crc;           /* crc checksum over all claims */
> > +} __packed;
> > +
> > +struct claim {
> > +       uint8_t addr[ETH_ALEN];
> > +       short vid;
> > +       struct backbone_gw *backbone_gw;
> > +       unsigned long lasttime; /* last time we heard of claim (locals only)
> > */
> > +       struct rcu_head rcu;
> > +       atomic_t refcount;
> > +       struct hlist_node hash_entry;
> > +} __packed;
> 
> Why are these structs packed ?

For similar reasons as the tt_global/local_entry issue: The hash functions both
access orig and vid from these structures.

However, as there are not that many sharing function I'll split it into
choose_claim()/choose_backbone_gw() and remove the __packed attribute.

In this case, it should be a little bit shorter and cleaner than using a common
structure (no container_of() stuff).

Thanks for the comments, you'll find the changed stuff as patches in my blaII_dirty repo.

Cheers,
	Simon

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