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