Re: [B.A.T.M.A.N.] [PATCH 1/2] batman-adv: purge bridge loop avoidance when its disabled

[Simon Wunderlich <sw@simonwunderlich.de>]

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

On Tuesday 17 November 2015 16:01:27 Marek Lindner wrote:
> On Monday, November 09, 2015 16:20:52 Simon Wunderlich wrote:
> > -BATADV_ATTR_SIF_BOOL(bridge_loop_avoidance, S_IRUGO | S_IWUSR, NULL);
> > +BATADV_ATTR_SIF_BOOL(bridge_loop_avoidance, S_IRUGO | S_IWUSR,
> > +                    batadv_bla_status_update);
> > 
> >  #endif
> 
> Are we sure this is correct ? The post function is called whether or not
> there actually was a change in the setting. The check in
> __batadv_store_bool_attr() is this:
> 
> ret = batadv_store_bool_attr(buff, count, net_dev, attr->name,
>                                      attr_store);
> if (post_func && ret)
>             post_func(net_dev);
> 
> Let's ignore for now that ret should be changed to check for '> 0' to avoid
> calling post_func() in case of an error. The return value is always non-
> negative unless the input is broken. You could enable BLA while it already
> is enabled which would reset all claim tables. Is that intended ?

Its not intended, although my initial thought was that it didn't hurt too much 
- the backbone gateway and claim tables would be dropped and the interface 
would go into the "protected" state again, not allowing broadcasts for 30 (or 
60 seconds, if the second patch is applied).

However, since you brought up this point, I think we should really change the 
behaviour of batadv_store_bool_attr() and friends, only calling post_func if 
there really was a change. I've checked the other functions which use that, 
and there shouldn't be any problem with that as far as I see - they do all 
some changes which depend on actual changes of the respective parameter. The 
other update functions are:

 * batadv_dat_status_update
 * batadv_update_min_mtu
 * batadv_post_gw_reselect
 * batadv_nc_status_update

If you agree, I'd send another patch to change the behaviour as proposed.

Cheers,
    Simon

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