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

[Matthias Schiffer]

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.

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?

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...



> 
> 
> 
> 
> 
>>  	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;
>>


-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: OpenPGP digital signature
URL: <http://lists.infradead.org/pipermail/openwrt-devel/attachments/20150128/fd8a7738/attachment.sig>
-------------- next part --------------
_______________________________________________
openwrt-devel mailing list
openwrt-devel at lists.openwrt.org
https://lists.openwrt.org/cgi-bin/mailman/listinfo/openwrt-devel