| Message ID | 1438729208-3556-3-git-send-email-linus.luessing@c0d3.blue |
|---|---|
| State | Changes Requested |
| Headers | show |
On 2015-08-05 01:00, Linus Lüssing wrote: > With this patch the multicast_to_unicast feature can be disabled for all > wireless interfaces via an according option on the uci bridge interface. > > This patch also exports the setting information to wireless handler > scripts. The hostapd script will need that information to determine > whether to enable or disable ap-isolation, for instance. > > Signed-off-by: Linus Lüssing <linus.luessing@c0d3.blue> I think it would be better to store these flags in the bridge config instead of the generic device config, and either add a blob_buf argument in struct device_hotplug_ops -> prepare, or add a new callback get_info, which adds the bridge info for use in the wireless json. I would like to avoid polluting the generic device options with bridge specific stuff. This patch should probably be merged with the previous one afterwards, since the first one without any other changes will cause regressions. - Felix
On Wed, Aug 05, 2015 at 01:38:28PM +0200, Felix Fietkau wrote: > On 2015-08-05 01:00, Linus Lüssing wrote: > > With this patch the multicast_to_unicast feature can be disabled for all > > wireless interfaces via an according option on the uci bridge interface. > > > > This patch also exports the setting information to wireless handler > > scripts. The hostapd script will need that information to determine > > whether to enable or disable ap-isolation, for instance. > > > > Signed-off-by: Linus Lüssing <linus.luessing@c0d3.blue> > I think it would be better to store these flags in the bridge config > instead of the generic device config, and either add a blob_buf argument > in struct device_hotplug_ops -> prepare, or add a new callback get_info, > which adds the bridge info for use in the wireless json. These would then be options per bridge, but not per bridge port, wouldn't they? For the multicast_to_unicast feature, okay (currently this patch only allows setting it "globally" for a bridge but not on a bridge port basis - though I was thinking about maybe making it bridge port specific once someone needs that). For the multicast_router option I'd need that on a bridge port basis. > > I would like to avoid polluting the generic device options with bridge > specific stuff. > > This patch should probably be merged with the previous one afterwards, > since the first one without any other changes will cause regressions. Regressions, where/why? If this patch were ommitted from this patchset then no harm should be done (unless I'm missing something?). Cheers, Linus
On 2015-08-06 22:10, Linus Lüssing wrote: > On Wed, Aug 05, 2015 at 01:38:28PM +0200, Felix Fietkau wrote: >> On 2015-08-05 01:00, Linus Lüssing wrote: >> > With this patch the multicast_to_unicast feature can be disabled for all >> > wireless interfaces via an according option on the uci bridge interface. >> > >> > This patch also exports the setting information to wireless handler >> > scripts. The hostapd script will need that information to determine >> > whether to enable or disable ap-isolation, for instance. >> > >> > Signed-off-by: Linus Lüssing <linus.luessing@c0d3.blue> >> I think it would be better to store these flags in the bridge config >> instead of the generic device config, and either add a blob_buf argument >> in struct device_hotplug_ops -> prepare, or add a new callback get_info, >> which adds the bridge info for use in the wireless json. > > These would then be options per bridge, but not per bridge port, > wouldn't they? Right. > For the multicast_to_unicast feature, okay (currently this patch > only allows setting it "globally" for a bridge but not on a bridge > port basis - though I was thinking about maybe making it bridge > port specific once someone needs that). For the multicast_router > option I'd need that on a bridge port basis. OK, then leave it as device options for now until we have a better way of specifiying bridge port settings. Same for multicast_to_unicast. >> I would like to avoid polluting the generic device options with bridge >> specific stuff. >> >> This patch should probably be merged with the previous one afterwards, >> since the first one without any other changes will cause regressions. > > Regressions, where/why? If this patch were ommitted from this > patchset then no harm should be done (unless I'm missing > something?). The regression is: patch 1 enables hairpin mode for wireless interfaces, with the assumption that the wireless script enables isolate mode for multicast-to-unicast enable configurations. The wireless script can only do that if patch 2 is applies, because that one passes the required options. The regression disappears after you rework patch 1 as discussed. - Felix
On Thu, Aug 06, 2015 at 11:09:02PM +0200, Felix Fietkau wrote: > On 2015-08-06 22:10, Linus Lüssing wrote: > > On Wed, Aug 05, 2015 at 01:38:28PM +0200, Felix Fietkau wrote: > >> On 2015-08-05 01:00, Linus Lüssing wrote: > >> > With this patch the multicast_to_unicast feature can be disabled for all > >> > wireless interfaces via an according option on the uci bridge interface. > >> > > >> > This patch also exports the setting information to wireless handler > >> > scripts. The hostapd script will need that information to determine > >> > whether to enable or disable ap-isolation, for instance. > >> > > >> > Signed-off-by: Linus Lüssing <linus.luessing@c0d3.blue> > >> I think it would be better to store these flags in the bridge config > >> instead of the generic device config, and either add a blob_buf argument > >> in struct device_hotplug_ops -> prepare, or add a new callback get_info, > >> which adds the bridge info for use in the wireless json. > > > > These would then be options per bridge, but not per bridge port, > > wouldn't they? > Right. > > > For the multicast_to_unicast feature, okay (currently this patch > > only allows setting it "globally" for a bridge but not on a bridge > > port basis - though I was thinking about maybe making it bridge > > port specific once someone needs that). For the multicast_router > > option I'd need that on a bridge port basis. > OK, then leave it as device options for now until we have a better way > of specifiying bridge port settings. Same for multicast_to_unicast. Ok, thanks :). (we want to fix and use bridge snooping + multicast-to-unicast here as soon as possible so I'm quite fond of simple solutions for now :) ) > > >> I would like to avoid polluting the generic device options with bridge > >> specific stuff. > >> > >> This patch should probably be merged with the previous one afterwards, > >> since the first one without any other changes will cause regressions. > > > > Regressions, where/why? If this patch were ommitted from this > > patchset then no harm should be done (unless I'm missing > > something?). > The regression is: patch 1 enables hairpin mode for wireless interfaces, > with the assumption that the wireless script enables isolate mode for > multicast-to-unicast enable configurations. > The wireless script can only do that if patch 2 is applies, because that > one passes the required options. Not passing the multicast_to_unicast option from netifd to hostapd.sh is not a regression. Actually even with patch 2 there are cases where no multicast_to_unicast json option is passed - that's when no multicast_to_unicast option was set in uci. With netifd patch 1, without netifd patch 2 and with openwrt patch 1 things will work fine without a regression because multicast_to_unicast is then empty and hostapd.sh will assume the default which is that multicast_to_unicast was enabled. However, netifd patch 1 without openwrt patch 1 is a regression, yes (which is independant of netifd patch 2). Which I couldn't do with a single patch as it's in two packages. But I like your suggestion for netifd patch 1 which will also get rid of an "intermediate regression" :). So with that change I think I can leave netifd patch 2 and netifd 3 as is, without any regressions. Cheers, Linus
diff --git a/device.c b/device.c index b5bee11..b44ccb8 100644 --- a/device.c +++ b/device.c @@ -48,6 +48,7 @@ static const struct blobmsg_policy dev_attrs[__DEV_ATTR_MAX] = { [DEV_ATTR_RPS] = { .name = "rps", .type = BLOBMSG_TYPE_BOOL }, [DEV_ATTR_XPS] = { .name = "xps", .type = BLOBMSG_TYPE_BOOL }, [DEV_ATTR_DADTRANSMITS] = { .name = "dadtransmits", .type = BLOBMSG_TYPE_INT32 }, + [DEV_ATTR_MULTICAST_TO_UNICAST] = { .name = "multicast_to_unicast", .type = BLOBMSG_TYPE_BOOL }, }; const struct uci_blob_param_list device_attr_list = { @@ -174,6 +175,7 @@ device_merge_settings(struct device *dev, struct device_settings *n) s->neigh6reachabletime : os->neigh6reachabletime; n->dadtransmits = s->flags & DEV_OPT_DADTRANSMITS ? s->dadtransmits : os->dadtransmits; + n->multicast_to_unicast = s->multicast_to_unicast; n->flags = s->flags | os->flags; } @@ -274,6 +276,11 @@ device_init_settings(struct device *dev, struct blob_attr **tb) s->flags |= DEV_OPT_DADTRANSMITS; } + if ((cur = tb[DEV_ATTR_MULTICAST_TO_UNICAST])) { + s->multicast_to_unicast = blobmsg_get_bool(cur); + s->flags |= DEV_OPT_MULTICAST_TO_UNICAST; + } + device_set_disabled(dev, disabled); } @@ -884,6 +891,8 @@ device_dump_status(struct blob_buf *b, struct device *dev) } if (st.flags & DEV_OPT_DADTRANSMITS) blobmsg_add_u32(b, "dadtransmits", st.dadtransmits); + if (st.flags & DEV_OPT_MULTICAST_TO_UNICAST) + blobmsg_add_u8(b, "multicast_to_unicast", st.multicast_to_unicast); } s = blobmsg_open_table(b, "statistics"); diff --git a/device.h b/device.h index 373445c..4344d8a 100644 --- a/device.h +++ b/device.h @@ -42,6 +42,7 @@ enum { DEV_ATTR_RPS, DEV_ATTR_XPS, DEV_ATTR_DADTRANSMITS, + DEV_ATTR_MULTICAST_TO_UNICAST, __DEV_ATTR_MAX, }; @@ -84,6 +85,7 @@ enum { DEV_OPT_XPS = (1 << 11), DEV_OPT_MTU6 = (1 << 12), DEV_OPT_DADTRANSMITS = (1 << 13), + DEV_OPT_MULTICAST_TO_UNICAST = (1 << 14), }; /* events broadcasted to all users of a device */ @@ -141,6 +143,7 @@ struct device_settings { bool rps; bool xps; unsigned int dadtransmits; + bool multicast_to_unicast; }; /* diff --git a/scripts/netifd-wireless.sh b/scripts/netifd-wireless.sh index c5d8a96..4d81929 100644 --- a/scripts/netifd-wireless.sh +++ b/scripts/netifd-wireless.sh @@ -260,6 +260,7 @@ for_each_interface() { json_select "$_w_iface" if [ -n "$_w_types" ]; then json_get_var network_bridge bridge + json_get_var multicast_to_unicast multicast_to_unicast json_select config json_get_var _w_type mode json_select .. diff --git a/system-linux.c b/system-linux.c index 9c4bea6..944245c 100644 --- a/system-linux.c +++ b/system-linux.c @@ -567,14 +567,19 @@ static char *system_get_bridge(const char *name, char *buf, int buflen) } static void -system_bridge_set_wireless(struct device *dev) +system_bridge_set_wireless(struct device *bridge, struct device *dev) { + bool mcast_to_ucast = true; bool hairpin = true; - if (dev->wireless_isolate) + if (bridge->settings.flags & DEV_OPT_MULTICAST_TO_UNICAST && + !bridge->settings.multicast_to_unicast) + mcast_to_ucast = false; + + if (!mcast_to_ucast || dev->wireless_isolate) hairpin = false; - system_bridge_set_multicast_to_unicast(dev, "1"); + system_bridge_set_multicast_to_unicast(dev, mcast_to_ucast ? "1" : "0"); system_bridge_set_hairpin_mode(dev, hairpin ? "1" : "0"); } @@ -588,7 +593,7 @@ int system_bridge_addif(struct device *bridge, struct device *dev) ret = system_bridge_if(bridge->ifname, dev, SIOCBRADDIF, NULL); if (dev->wireless) - system_bridge_set_wireless(dev); + system_bridge_set_wireless(bridge, dev); return ret; } diff --git a/wireless.c b/wireless.c index 337f563..d0d2942 100644 --- a/wireless.c +++ b/wireless.c @@ -92,6 +92,10 @@ vif_config_add_bridge(struct blob_buf *buf, struct blob_attr *networks, bool pre dev->hotplug_ops->prepare(dev); blobmsg_add_string(buf, "bridge", dev->ifname); + + if (dev->settings.flags & DEV_OPT_MULTICAST_TO_UNICAST) + blobmsg_add_u8(buf, "multicast_to_unicast", + dev->settings.multicast_to_unicast); } static void
With this patch the multicast_to_unicast feature can be disabled for all wireless interfaces via an according option on the uci bridge interface. This patch also exports the setting information to wireless handler scripts. The hostapd script will need that information to determine whether to enable or disable ap-isolation, for instance. Signed-off-by: Linus Lüssing <linus.luessing@c0d3.blue> --- device.c | 9 +++++++++ device.h | 3 +++ scripts/netifd-wireless.sh | 1 + system-linux.c | 13 +++++++++---- wireless.c | 4 ++++ 5 files changed, 26 insertions(+), 4 deletions(-)