[OpenWrt-Devel] [PATCH netifd] bridge: allow enabling or disabling the multicast querier independently of IGMP snooping

[John Crispin]

On 28/01/2015 21:31, Matthias Schiffer wrote:
> On 01/28/2015 11:54 AM, John Crispin wrote:
>> Hi,
>> 
>> On 27/01/2015 03:49, Matthias Schiffer wrote:
>>> In larger networks, especially big batman-adv meshes, it may be
>>> desirable to enable IGMP snooping on every bridge without
>>> enabling the multicast querier to specifically put the querier
>>> on a well-connected node.
>>> 
>>> This patch adds a new UCI option 'multicast_querier' for
>>> bridges which allows this. The default is still the value of
>>> the 'igmp_snooping' option to maintain backwards compatiblity.
>>> 
>> 
>> per-se a good patch but its not reentrant
>> 
>> 
>>> Signed-off-by: Matthias Schiffer
>>> <mschiffer at universe-factory.net> --- bridge.c       | 8
>>> +++++++- system-linux.c | 2 +- system.h       | 1 + 3 files
>>> changed, 9 insertions(+), 2 deletions(-)
>>> 
>>> diff --git a/bridge.c b/bridge.c index f8478ad..f7dbf61 100644 
>>> --- a/bridge.c +++ b/bridge.c @@ -32,6 +32,7 @@ enum { 
>>> BRIDGE_ATTR_HELLO_TIME, BRIDGE_ATTR_MAX_AGE, 
>>> BRIDGE_ATTR_BRIDGE_EMPTY, +	BRIDGE_ATTR_MULTICAST_QUERIER, 
>>> __BRIDGE_ATTR_MAX };
>>> 
>>> @@ -45,6 +46,7 @@ static const struct blobmsg_policy
>>> bridge_attrs[__BRIDGE_ATTR_MAX] = { [BRIDGE_ATTR_MAX_AGE] = {
>>> "max_age", BLOBMSG_TYPE_INT32 }, [BRIDGE_ATTR_IGMP_SNOOP] = {
>>> "igmp_snooping", BLOBMSG_TYPE_BOOL }, 
>>> [BRIDGE_ATTR_BRIDGE_EMPTY] = { "bridge_empty",
>>> BLOBMSG_TYPE_BOOL }, +	[BRIDGE_ATTR_MULTICAST_QUERIER] = {
>>> "multicast_querier", BLOBMSG_TYPE_BOOL }, };
>>> 
>>> static const struct uci_blob_param_info
>>> bridge_attr_info[__BRIDGE_ATTR_MAX] = { @@ -547,6 +549,7 @@
>>> bridge_apply_settings(struct bridge_state *bst, struct
>>> blob_attr **tb) cfg->stp = false; cfg->forward_delay = 2; 
>>> cfg->igmp_snoop = true; +	cfg->multicast_querier = true; 
>>> cfg->bridge_empty = false; cfg->priority = 0x7FFF;
>>> 
>>> @@ -560,7 +563,10 @@ bridge_apply_settings(struct bridge_state
>>> *bst, struct blob_attr **tb) cfg->priority =
>>> blobmsg_get_u32(cur);
>>> 
>>> if ((cur = tb[BRIDGE_ATTR_IGMP_SNOOP])) -		cfg->igmp_snoop =
>>> blobmsg_get_bool(cur); +		cfg->multicast_querier =
>>> cfg->igmp_snoop = blobmsg_get_bool(cur);
>> 
>> i make 1 ubus call and set multicast_querier=0 and then a 2nd
>> call and set igmp_snoop=1 this will break my prior call.
>> 
>> dont change the above line and then ....
>> 
>>> + +	if ((cur = tb[BRIDGE_ATTR_MULTICAST_QUERIER])) +
>>> cfg->multicast_querier = blobmsg_get_bool(cur);
>>> 
>>> if ((cur = tb[BRIDGE_ATTR_AGEING_TIME])) { cfg->ageing_time =
>>> blobmsg_get_u32(cur); diff --git a/system-linux.c
>>> b/system-linux.c index 4737fa6..ef90880 100644 ---
>>> a/system-linux.c +++ b/system-linux.c @@ -772,7 +772,7 @@ int
>>> system_bridge_addbr(struct device *bridge, struct bridge_config
>>> *cfg) bridge->ifname, cfg->igmp_snoop ? "1" : "0");
>>> 
>>> system_set_dev_sysctl("/sys/devices/virtual/net/%s/bridge/multicast_querier",
>>>
>>> 
-		bridge->ifname, cfg->igmp_snoop ? "1" : "0");
>>> +		bridge->ifname, cfg->multicast_querier ? "1" : "0");
>>> 
>> 
>> change this to
>> 
>> bridge->ifname, (cfg->igmp_snoop & cfg->multicast_querier) ? "1"
>> : "0");
>> 
>> this should not break anything with multiple ubus calls. the
>> default behavior also wont change as we set
>> cfg->multicast_querier = true;
>> 
>> Please make those changes to the patch and resend it.
> I just re-read the whole function and noticed why I made my change
> like this in the first place: all values in bridge_config are
> always reset to their defaults at the top of
> bridge_apply_settings() anyways, not regarding if the blobmsg
> contains a new value for the options or not.
> 

yep, i will cook up a patch tomorrow ...

> Doesn't this mean that all option's values are lost in the case of 
> multiple ubus calls anyways? Is this intended for bridge devices,
> or should this be fixed as well?
> 

probably not

> I hope I don't misunderstand how this is supposed to work, I'm not 
> really familar with the way dynamic reconfiguration works in
> netifd...
> 

me neither :)

> 
> 
>> 
>> 
>> 
>> 
>> 
>>> args[0] = BRCTL_SET_BRIDGE_PRIORITY; args[1] = cfg->priority; 
>>> diff --git a/system.h b/system.h index 9a2326b..94e0dd9 100644 
>>> --- a/system.h +++ b/system.h @@ -50,6 +50,7 @@ struct
>>> bridge_config { enum bridge_opt flags; bool stp; bool
>>> igmp_snoop; +	bool multicast_querier; unsigned short priority; 
>>> int forward_delay; bool bridge_empty;
>>> 
> 
> 
> 
> 
> _______________________________________________ openwrt-devel
> mailing list openwrt-devel at lists.openwrt.org 
> https://lists.openwrt.org/cgi-bin/mailman/listinfo/openwrt-devel
> 
_______________________________________________
openwrt-devel mailing list
openwrt-devel at lists.openwrt.org
https://lists.openwrt.org/cgi-bin/mailman/listinfo/openwrt-devel