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

2015-02-18 Thread John Crispin 


On 17/02/2015 18:16, Matthias Schiffer wrote:
 On 01/28/2015 11:00 PM, John Crispin wrote:
  
  
  On 28/01/2015 21:31, Matthias Schiffer wrote:
  On 01/28/2015 11:54 AM, John Crispin wrote:
 [...]
  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 ...
 Hi, any news on this?
 
 

i did make a patch, apparently i never pushed it. will go searching
during the day
___
openwrt-devel mailing list
openwrt-devel@lists.openwrt.org
https://lists.openwrt.org/cgi-bin/mailman/listinfo/openwrt-devel

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

2015-02-17 Thread Matthias Schiffer 
On 01/28/2015 11:00 PM, John Crispin wrote:
 
 
 On 28/01/2015 21:31, Matthias Schiffer wrote:
 On 01/28/2015 11:54 AM, John Crispin wrote:
[...]
 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 ...

Hi, any news on this?


 
 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 :)
 




signature.asc
Description: OpenPGP digital signature
___
openwrt-devel mailing list
openwrt-devel@lists.openwrt.org
https://lists.openwrt.org/cgi-bin/mailman/listinfo/openwrt-devel

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

2015-01-28 Thread John Crispin 
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 mschif...@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.





   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@lists.openwrt.org
https://lists.openwrt.org/cgi-bin/mailman/listinfo/openwrt-devel

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

2015-01-28 Thread Matthias Schiffer 
On 01/28/2015 01:59 PM, John Crispin wrote:
 
 
 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.

 Signed-off-by: Matthias Schiffer mschif...@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
 
 [...]
 
 
 @@ -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);
 +
 +if ((cur = tb[BRIDGE_ATTR_MULTICAST_QUERIER]))
 +cfg-multicast_querier = blobmsg_get_bool(cur);
  
 
 how about this ?
 
   if ((cur = tb[BRIDGE_ATTR_IGMP_SNOOP]))
   cfg-igmp_snoop = blobmsg_get_bool(cur);
 
   if ((cur = tb[BRIDGE_ATTR_MULTICAST_QUERIER]))
   cfg-multicast_querier = blobmsg_get_bool(cur);
   else
 cfg-multicast_querier = cfg-igmp_snoop;
 
 
 
 
 
This will break when neither igmp_snoop and multicast_querier is set in
a ubus request, it will overwrite the multicast_querier setting with the
previous igmp_snoop setting. I realized that it is not possible to store
all needed cases in two booleans in the bridge_config (in my first
version I didn't think about dynamic reconfiguration at all...)


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





signature.asc
Description: OpenPGP digital signature
___
openwrt-devel mailing list
openwrt-devel@lists.openwrt.org
https://lists.openwrt.org/cgi-bin/mailman/listinfo/openwrt-devel

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

2015-01-28 Thread Matthias Schiffer 
On 01/28/2015 01:34 PM, John Crispin wrote:
 
 
 On 28/01/2015 13:30, Matthias Schiffer wrote:
 On 01/28/2015 11:54 AM, John Crispin wrote:
 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.
 Wouldn't it be better to change the possible values of
 multicast_querier to yes, no and unset? That way it could
 always default to the igmp_snoop value when it is unset, but it
 would be possible to override it in both ways.

 
 do you call this by hand or via webui/rpc ?
At the moment, this is compile-tested only. My only usecase is setting
it in /etc/config/network.

Your suggestion would make it impossible to enable the multicast querier
without also enabling snooping. My intention was to allow all four
combinations of snooping on/off and querier on/off, while having the
querier setting default to the snooping setting (so nothing changes for
people who have set igmp_snoop, but not multicast_querier). While I
don't need the case querier on/snooping off, I don't see why we
shouldn't allow it.



signature.asc
Description: OpenPGP digital signature
___
openwrt-devel mailing list
openwrt-devel@lists.openwrt.org
https://lists.openwrt.org/cgi-bin/mailman/listinfo/openwrt-devel

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

2015-01-28 Thread 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 mschif...@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.
Wouldn't it be better to change the possible values of multicast_querier
to yes, no and unset? That way it could always default to the
igmp_snoop value when it is unset, but it would be possible to override
it in both ways.

 
 
 
 
 
  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;





signature.asc
Description: OpenPGP digital signature
___
openwrt-devel mailing list
openwrt-devel@lists.openwrt.org
https://lists.openwrt.org/cgi-bin/mailman/listinfo/openwrt-devel

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

2015-01-28 Thread John Crispin 


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.
 
 Signed-off-by: Matthias Schiffer mschif...@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

[...]


 @@ -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);
 +
 + if ((cur = tb[BRIDGE_ATTR_MULTICAST_QUERIER]))
 + cfg-multicast_querier = blobmsg_get_bool(cur);
  

how about this ?

if ((cur = tb[BRIDGE_ATTR_IGMP_SNOOP]))
cfg-igmp_snoop = blobmsg_get_bool(cur);

if ((cur = tb[BRIDGE_ATTR_MULTICAST_QUERIER]))
cfg-multicast_querier = blobmsg_get_bool(cur);
else
  cfg-multicast_querier = cfg-igmp_snoop;






   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);
  
   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@lists.openwrt.org
https://lists.openwrt.org/cgi-bin/mailman/listinfo/openwrt-devel

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

2015-01-28 Thread John Crispin 


On 28/01/2015 13:30, 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
 mschif...@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.
 Wouldn't it be better to change the possible values of
 multicast_querier to yes, no and unset? That way it could
 always default to the igmp_snoop value when it is unset, but it
 would be possible to override it in both ways.
 

do you call this by hand or via webui/rpc ?





 
 
 
 
 
 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@lists.openwrt.org 
 https://lists.openwrt.org/cgi-bin/mailman/listinfo/openwrt-devel
 
___
openwrt-devel mailing list
openwrt-devel@lists.openwrt.org
https://lists.openwrt.org/cgi-bin/mailman/listinfo/openwrt-devel

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

2015-01-28 Thread 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 mschif...@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;





signature.asc
Description: OpenPGP digital signature
___
openwrt-devel mailing list
openwrt-devel@lists.openwrt.org
https://lists.openwrt.org/cgi-bin/mailman/listinfo/openwrt-devel

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

2015-01-28 Thread 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
 mschif...@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@lists.openwrt.org 
 https://lists.openwrt.org/cgi-bin/mailman/listinfo/openwrt-devel
 
___
openwrt-devel mailing list
openwrt-devel@lists.openwrt.org
https://lists.openwrt.org/cgi-bin/mailman/listinfo/openwrt-devel

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

2015-01-26 Thread Matthias Schiffer 
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.

Signed-off-by: Matthias Schiffer mschif...@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);
+
+   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);
 
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;
-- 
2.2.2
___
openwrt-devel mailing list
openwrt-devel@lists.openwrt.org
https://lists.openwrt.org/cgi-bin/mailman/listinfo/openwrt-devel