| Message ID | b4fac27152a34dd39d59ca0b83de0157a8f7805f.1422326992.git.mschiffer@universe-factory.net |
|---|---|
| State | Accepted |
| Headers | show |
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@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; >
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@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; >>
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 >>> <mschiffer@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 >
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 <mschiffer@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; >
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.
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 <mschiffer@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; >>
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@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; >>
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@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 >
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 :) >
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
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;
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 <mschiffer@universe-factory.net> --- bridge.c | 8 +++++++- system-linux.c | 2 +- system.h | 1 + 3 files changed, 9 insertions(+), 2 deletions(-)