Re: [B.A.T.M.A.N.] Antwort: Re: [PATCHv2 1/7] batman-adv: prevent multiple ARP replies sent by gateways if dat enabled

[Simon Wunderlich <sw@simonwunderlich.de>]

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

Hi Andreas,

On Friday 04 March 2016 08:11:05 Andreas Pape wrote:
> Hi Antonio,
> 
> > > +bool batadv_bla_handle_local_claim(struct batadv_priv *bat_priv,
> > > +               u8 *addr, unsigned short vid)
> > > +{
> > > +   struct batadv_bla_claim search_claim;
> > > +   struct batadv_bla_claim *claim = NULL;
> > > +   struct batadv_hard_iface *primary_if = NULL;
> > > +   bool ret = true;
> > > +
> > > +   if (!atomic_read(&bat_priv->bridge_loop_avoidance))
> > > +      return ret;
> > > +
> > > +   primary_if = batadv_primary_if_get_selected(bat_priv);
> > > +   if (!primary_if)
> > > +      goto out;
> > 
> > do you really need this goto here ? I think you can just return ret.
> > 
> > This way you can avoid the blind initialization of claim and
> > primary_if and you
> > can also remove the 'out' label at all.
> 
> I tried to "copy" the coding style of other functions. But in this case
> you are right that this is senseless here.
> 
> > > +
> > > +   /* First look if the mac address is claimed */
> > > +   ether_addr_copy(search_claim.addr, addr);
> > > +   search_claim.vid = vid;
> > > +
> > > +   claim = batadv_claim_hash_find(bat_priv, &search_claim);
> > > +
> > > +   /* If there is a claim and we are not owner of the claim,
> > > +    * return false;
> > 
> > no need for the ';' here :)
> > 
> > > +    */
> > > +   if (claim) {
> > > +      if (!batadv_compare_eth(claim->backbone_gw->orig,
> > > +               primary_if->net_dev->dev_addr))
> > > +         ret = false;
> > > +   } else {
> > > +      /* If there is no claim, claim the device */
> > > +      batadv_dbg(BATADV_DBG_BLA, bat_priv,
> > > +            "Handle claim locally for currently not claimed mac
> 
> %pM.\n",
> 
> > > +            search_claim.addr);
> > > +
> > > +      batadv_handle_claim(bat_priv, primary_if,
> > > +                primary_if->net_dev->dev_addr, addr, vid);
> > > +   }
> > > +
> > 
> > ...
> > 
> > > @@ -1000,6 +1001,20 @@ bool batadv_dat_snoop_outgoing_arp_request
> > 
> > (struct batadv_priv *bat_priv,
> > 
> > >           goto out;
> > >        
> > >        }
> > > 
> > > +      /* If BLA is enabled, only send ARP replies if we have claimed
> > > +       * the destination for the ARP request or if no one else of
> > > +       * the backbone gws belonging to our backbone has claimed the
> > > +       * destination.
> > > +       */
> > > +      if (!batadv_bla_handle_local_claim(bat_priv,
> > > +                     dat_entry->mac_addr, vid)) {
> > 
> > I like the approach you take to fix this issue, however I don't
> 
> understand why
> 
> > you want to "manually" claim a client.
> 
> This was meant to be something like a safety measure. DAT and BLA tables
> are not
> synchronized and can timeout separately, right? What, if a client address
> is still
> available in DAT but the client is not claimed anymore. In this case I
> think that
> claiming the client might be a good idea. I misused DAT here to make sure
> that the
> claim table is up to date. In my test setup running DAT and BLA on a
> number of
> backbone gateways I still have problems with looping packets (although
> having all
> of my patches applied). The problems have become significantly less often
> but
> from time to time there are still some occurring.
> In all of these cases clients from the mesh have been
> unclaimed somehow or they have been considered as having roamed to the
> backbone
> (for an unknown reason the workaround in patch 7/7 did not work in that
> case).

I would agree to Antonio that sending a claim is not necessary, and might 
actually lead to bad choices of the outgoing backbone gateway. After all, at 
this point any "random" backbone gateway may answer to this request, just 
because it has the DAT entry, but maybe may actually not be able to reach the 
client via the mesh.

I think checking if a local claim exists is a good idea, but sending a claim 
is not a good idea - this might introduce effects which will be even more 
complicated to debug.

Cheers,
      Simon

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 819 bytes --]