| Message ID | 20240402130221.58706-1-newtwen+github@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | [1/7] lldpd: fixed interface(s) parsing | expand |
Hi, > For interface type parameters, the man page documents patterns: > ``` > *,!eth*,!!eth1 > > uses all interfaces, except interfaces starting with "eth", > but including "eth1". > ``` at some point, uci configuration was meant to provide a somewhat sane config abstraction over various damon specific native configurations, now I see the recurring trend to expose every last native config idiosyncrasy as-is in uci. Is there really a need to support these weird micro formats in uci? The uci config for lldpd should deal with logical interface names and translate them into layer 2 ones as needed. People requiring complex, hand-tuned settings probably want to bypass uci entirly and simply start lldpd with a static native config file. ~ Jo
Thanks for the insight, Jo. Inline: On 2024-04-03 01:12, Jo-Philipp Wich wrote: > Hi, > >> For interface type parameters, the man page documents patterns: >> ``` >> *,!eth*,!!eth1 >> >> uses all interfaces, except interfaces starting with "eth", >> but including "eth1". >> ``` > > at some point, uci configuration was meant to provide a somewhat sane config abstraction over various damon specific native configurations, now I see the recurring trend to expose every last native config idiosyncrasy as-is in uci. I guess that's what people want. More power, and ease, out of the box. Adapt or die. It would seem you have not yet read the man page for lldpd... > > Is there really a need to support these weird micro formats in uci? The uci config for lldpd should deal with logical interface names and translate them into layer 2 ones as needed. People requiring complex, hand-tuned settings probably want to bypass uci entirly and simply start lldpd with a static native config file. > Try saying the same thing about the kernel. With this philosophy, it's like we aim for the 50th percentile. Everybody's needs differ, which is how openwrt has acquired endless patches and grown. I do not see this is as an idiosyncrasy, it's standard behaviour in a number of other systems. The cost to handle it is, as we see from the code, low, while still retaining the original behaviour ("deal with logical interface names and translate them into layer 2 ones as needed.").
diff --git a/package/network/services/lldpd/files/lldpd.init b/package/network/services/lldpd/files/lldpd.init index 67ee011ae2..fc53520c5b 100644 --- a/package/network/services/lldpd/files/lldpd.init +++ b/package/network/services/lldpd/files/lldpd.init @@ -76,18 +76,32 @@ get_config_restart_hash() { } get_config_cid_ifaces() { + set -o noglob local _ifaces config_get _ifaces 'config' "$2" local _iface _ifnames="" for _iface in $_ifaces; do - local _ifname="" - if network_get_device _ifname "$_iface" || [ -e "/sys/class/net/$_iface" ]; then - append _ifnames "${_ifname:-$_iface}" "," + local _l3dev="" + # save any "!" or "!!" prefix from the interface name + _suffix=${_iface##*"!"} + _prefix=${_iface%%"$_suffix"} + + if network_get_device _l3dev "$_suffix" || [ -e "/sys/class/net/$_suffix" ]; then + # prepend the stripped "!" or "!!" prefix here if we had one + append _ifnames "$_prefix${_l3dev:-$_suffix}" "," + else + case $_iface in + *"*"*) + # Append any interface names including a glob '*' pattern + append _ifnames "$_iface" "," + ;; + esac fi done export -n "${1}=$_ifnames" + set +o noglob } write_lldpd_conf()
For interface type parameters, the man page documents patterns: ``` *,!eth*,!!eth1 uses all interfaces, except interfaces starting with "eth", but including "eth1". ``` While we must check that interfaces exist, first strip any prefixed "!" then pass the original string (with `!` prefix) again, to command lines. * Renamed `_ifname` to `_l3dev`. * Glob pattern `*` is also valid - pass those verbatim. The net result is that now interface 'names' including globs '*' and '!' inversions now are included in the generated lldpd configs. We must also `set -o noglob` and `set +o noglob` to disable and enable globbing respectively, because when we pass `*` as an interface choice everything goes to hell without them. Tested extensively on: 22.03.6 Signed-off-by: Paul Donald <newtwen+github@gmail.com> --- .../network/services/lldpd/files/lldpd.init | 20 ++++++++++++++++--- 1 file changed, 17 insertions(+), 3 deletions(-)