| Message ID | 20201211070511.3515348-2-john@phrozen.org |
|---|---|
| State | Under Review |
| Delegated to: | John Crispin |
| Headers | show |
| Series | [1/2] lldpd: do not start lldpd on the loopback device | expand |
John Crispin <john@phrozen.org> writes: > iff --git a/package/network/services/lldpd/files/lldpd.init b/package/network/services/lldpd/files/lldpd.init > index 7a5b25e016..8200556786 100644 > --- a/package/network/services/lldpd/files/lldpd.init > +++ b/package/network/services/lldpd/files/lldpd.init > @@ -10,6 +10,10 @@ LLDPSOCKET=/var/run/lldpd.socket > LLDPD_CONF=/tmp/lldpd.conf > LLDPD_CONFS_DIR=/tmp/lldpd.d > > +service_triggers() { > + procd_add_reload_trigger lldpd > +} > + > find_release_info() > { > [ -s /etc/os-release ] && . /etc/os-release > @@ -38,6 +42,10 @@ write_lldpd_conf() > for iface in $ifaces; do > local ifname="" > if network_get_device ifname "$iface" || [ -e "/sys/class/net/$iface" ]; then > + if [ -e "/sys/class/net/$ifname/bridge" -o -e "/sys/class/net/$ifname/lower_bridge" ] ; then > + local ports=$(jsonfilter -i /etc/board.json -e "@.network.$iface.ifname") > + [ "${ports// /,}" ] && ifname="${ports// /,}" > + fi > append ifnames "${ifname:-$iface}" "," > fi > done Doesn't this assume too much about /etc/config/network names always matching /etc/board.json? How about just getting the bridge ports from /sys/class/net/switch/brif/* if the configured interface resolves to a bridge? Note BTW: The "bridge" in "lower_bridge" is the name of the symlink target. It's not a static name. Try creating a subinterface under something that isn't named "brigde" :-) Bjørn
On 11.12.20 09:30, Bjørn Mork wrote: > John Crispin <john@phrozen.org> writes: > >> iff --git a/package/network/services/lldpd/files/lldpd.init b/package/network/services/lldpd/files/lldpd.init >> index 7a5b25e016..8200556786 100644 >> --- a/package/network/services/lldpd/files/lldpd.init >> +++ b/package/network/services/lldpd/files/lldpd.init >> @@ -10,6 +10,10 @@ LLDPSOCKET=/var/run/lldpd.socket >> LLDPD_CONF=/tmp/lldpd.conf >> LLDPD_CONFS_DIR=/tmp/lldpd.d >> >> +service_triggers() { >> + procd_add_reload_trigger lldpd >> +} >> + >> find_release_info() >> { >> [ -s /etc/os-release ] && . /etc/os-release >> @@ -38,6 +42,10 @@ write_lldpd_conf() >> for iface in $ifaces; do >> local ifname="" >> if network_get_device ifname "$iface" || [ -e "/sys/class/net/$iface" ]; then >> + if [ -e "/sys/class/net/$ifname/bridge" -o -e "/sys/class/net/$ifname/lower_bridge" ] ; then >> + local ports=$(jsonfilter -i /etc/board.json -e "@.network.$iface.ifname") >> + [ "${ports// /,}" ] && ifname="${ports// /,}" >> + fi >> append ifnames "${ifname:-$iface}" "," >> fi >> done > > Doesn't this assume too much about /etc/config/network names always > matching /etc/board.json? How about just getting the bridge ports from > > /sys/class/net/switch/brif/* > > if the configured interface resolves to a bridge? > > Note BTW: The "bridge" in "lower_bridge" is the name of the symlink > target. It's not a static name. Try creating a subinterface under > something that isn't named "brigde" :-) > > > > Bjørn Hi Bjørn, correct, this is not the ideal solution right now. using /sys/class/net/switch/brif/* is also quirky as devices might later be added or removed (wifi). Right now the code is totally de-funct and this patch will fix it for a vast number of std use cases. I plan to pay some attention to lldpd post release and also add a small ubus interface. This would then be usable to listen to netifd notifications, thus fixing the problem properly. This patch does however improve the current situation a lot. John
On 11.12.20 09:30, Bjørn Mork wrote: > John Crispin <john@phrozen.org> writes: > >> iff --git a/package/network/services/lldpd/files/lldpd.init b/package/network/services/lldpd/files/lldpd.init >> index 7a5b25e016..8200556786 100644 >> --- a/package/network/services/lldpd/files/lldpd.init >> +++ b/package/network/services/lldpd/files/lldpd.init >> @@ -10,6 +10,10 @@ LLDPSOCKET=/var/run/lldpd.socket >> LLDPD_CONF=/tmp/lldpd.conf >> LLDPD_CONFS_DIR=/tmp/lldpd.d >> >> +service_triggers() { >> + procd_add_reload_trigger lldpd >> +} >> + >> find_release_info() >> { >> [ -s /etc/os-release ] && . /etc/os-release >> @@ -38,6 +42,10 @@ write_lldpd_conf() >> for iface in $ifaces; do >> local ifname="" >> if network_get_device ifname "$iface" || [ -e "/sys/class/net/$iface" ]; then >> + if [ -e "/sys/class/net/$ifname/bridge" -o -e "/sys/class/net/$ifname/lower_bridge" ] ; then >> + local ports=$(jsonfilter -i /etc/board.json -e "@.network.$iface.ifname") >> + [ "${ports// /,}" ] && ifname="${ports// /,}" >> + fi >> append ifnames "${ifname:-$iface}" "," >> fi >> done > > Doesn't this assume too much about /etc/config/network names always > matching /etc/board.json? How about just getting the bridge ports from > > /sys/class/net/switch/brif/* > > if the configured interface resolves to a bridge? > > Note BTW: The "bridge" in "lower_bridge" is the name of the symlink > target. It's not a static name. Try creating a subinterface under > something that isn't named "brigde" :-) > > > > Bjørn > > _______________________________________________ > openwrt-devel mailing list > openwrt-devel@lists.openwrt.org > https://lists.openwrt.org/mailman/listinfo/openwrt-devel Hi Bjørn, correct, this is not the ideal solution right now. using /sys/class/net/switch/brif/* is also quirky as devices might later be added or removed (wifi). Right now the code is totally de-funct and this patch will fix it for a vast number of std use cases. I plan to pay some attention to lldpd post release and also add a small ubus interface. This would then be usable to listen to netifd notifications, thus fixing the problem properly. This patch does however improve the current situation a lot. For all other cases you will have to manually add the interfaces as is required right now. John
diff --git a/package/network/services/lldpd/files/lldpd.init b/package/network/services/lldpd/files/lldpd.init index 7a5b25e016..8200556786 100644 --- a/package/network/services/lldpd/files/lldpd.init +++ b/package/network/services/lldpd/files/lldpd.init @@ -10,6 +10,10 @@ LLDPSOCKET=/var/run/lldpd.socket LLDPD_CONF=/tmp/lldpd.conf LLDPD_CONFS_DIR=/tmp/lldpd.d +service_triggers() { + procd_add_reload_trigger lldpd +} + find_release_info() { [ -s /etc/os-release ] && . /etc/os-release @@ -38,6 +42,10 @@ write_lldpd_conf() for iface in $ifaces; do local ifname="" if network_get_device ifname "$iface" || [ -e "/sys/class/net/$iface" ]; then + if [ -e "/sys/class/net/$ifname/bridge" -o -e "/sys/class/net/$ifname/lower_bridge" ] ; then + local ports=$(jsonfilter -i /etc/board.json -e "@.network.$iface.ifname") + [ "${ports// /,}" ] && ifname="${ports// /,}" + fi append ifnames "${ifname:-$iface}" "," fi done
The script was missing the reload trigger. Additionally running lldpid on a bridge is not correct. Rather than running lldpd on the L3 bridge, it needs to be run on all the L2 members. Signed-off-by: John Crispin <john@phrozen.org> --- package/network/services/lldpd/files/lldpd.init | 8 ++++++++ 1 file changed, 8 insertions(+)