uneighbord: add uneighbord - #30359
Conversation
Formality Check: FailedWe checked this pull request against the contribution guidelines. Here is what needs your attention: 🛑 CRITICAL ERRORS
Tip Do not close this pull request to make corrections. Instead, modify your existing commits (e.g. Something broken? Consider reporting an issue. |
468599e to
bab9bbe
Compare
openwrt-ai
left a comment
There was a problem hiding this comment.
Reviewed the single commit adding net/uneighbord. The packaging is close to the feed's conventions and the ucode is careful about validation; the findings below are mostly about packaging metadata, the CI test harness, and the trust model of the multicast protocol.
Two things that look like they need action before this leaves WIP: the missing PKG_VERSION, and the missing test-version.sh that the forced generic CI test needs for a daemon. The rest are judgement calls for you — in particular the unauthenticated-peer question, where I have flagged the mechanism rather than prescribing a design.
One note on the formality bot's CRITICAL error: it claims net/uneighbord/files/usr/sbin/uneighbord is destined for /etc/config/ and is not valid UCI. The install block sends that file to /usr/sbin/, and ./files/etc/config/uneighbord is a proper UCI file — all three installed files just share the basename uneighbord, which appears to be what confused the check. Looks like a false positive rather than something to fix here.
Generated by Claude Code
| const VERSION = 1; | ||
| const GROUP = "ff12::6e65:6967:6862:6f72:64"; | ||
| const PORT = 32027; | ||
| const MAX_PACKET = 8192; |
There was a problem hiding this comment.
An 8192-byte datagram is well past the 1280-byte IPv6 minimum MTU, so any announcement over ~1200 bytes has to be fragmented, and fragmented IPv6 multicast is exactly the traffic class that bridges, APs and IGMP/MLD-snooping switches are most likely to drop or fail to reassemble. That is not a rare case here: with 64 reports of [bssid(17) + ssid(≤32) + nr hex(26…1024)] the JSON reaches several kilobytes, and even a handful of BSSes with a normal ~70-char NR element already crosses 1280 bytes — so the packets most worth propagating are the ones most likely to be lost, silently and only on some networks.
Capping the announcement at the IPv6 minimum MTU (and either splitting across datagrams or logging a clear "too many reports to announce" once) would keep the failure mode deterministic. MAX_PACKET can stay large on the receive side; it is the send path that needs the tighter bound.
Generated by Claude Code
| if (last_installed[object] == signature) | ||
| continue; | ||
|
|
||
| bus.call(object, "rrm_nr_set", { list }, "ignore"); |
There was a problem hiding this comment.
Is "ignore" actually honoured as a positional fourth argument? Every in-tree use of that option passes it through the named-object form instead, e.g. wireless-device.uc:22-27:``
ubus.call({
object: "wpa_supplicant",
method: "mld_start",
return: "ignore",
data: { },
});
I could not find a single positional four-argument .call() in openwrt or luci to confirm the shorthand works. If it silently falls through to a different parameter, the return value is not being ignored and the libubus.error() check below may be reading the wrong state. Worth either switching to the object form here or confirming the shorthand is supported.
Generated by Claude Code
b46ce98 to
3e01d19
Compare
openwrt-ai
left a comment
There was a problem hiding this comment.
Re-reviewed the force-pushed 3e01d19 against bab9bbe. Every finding from the last round was picked up, and in a couple of cases the fix is better than what I suggested: neighbor_list() now builds the list locals-first with an explicit MAX_REPORTS cut instead of sorting candidates by BSSID, so a peer flooding low BSSIDs can no longer displace the AP's own reports — which is the part of that finding that actually mattered. MAX_SEND at 1232 (1280 − 40 − 8) is the right bound, and the deduplicated warning on oversize means the drop is at least visible. ulog() via ucode-mod-log is a cleaner answer than the warn() I proposed, and it makes the logread -e uneighbord recipe in README.md accurate. The "ignore" positional argument I could not verify is simply gone.
Two of the changes go beyond what was asked and are worth a second pair of eyes, since neither is a straight bug fix:
node_idmoved from lowest local BSSID to the LAN device MAC. The reasoning in the comment is sound — identity should outlive individual BSSes coming and going — but it also makes announcing depend on a second ubus object that can fail independently. See the inline comment onensure_socket().announce_local_reports()no longer bails on an emptylocal_bss. This is what lets peers clear a reloading AP's reports immediately instead of afterPEER_TIMEOUT_S, and it works becausediscover_hostapd()reports the N→0 transition as a change. Note it only covers the reload case: on SIGTERM nothing is announced, so peers still wait out the 90 s timeout. If that is intentional, it may be worth saying so next to the comment at lines 365–367.
test-version.sh matches the shape used by admin/syslog-ng and admin/zabbix, and all twelve Feeds Package Test Build jobs now pass — the generic version check that failed across every architecture last round is resolved. The only red check left is FormalityCheck / OpenWrt Makefiles, one half of which is the PKG_LICENSE_FILES removal I asked for; see the inline comment on the Makefile for where that leaves things. The other half is still the /etc/config/ false positive on files/usr/sbin/uneighbord, unchanged from last time.
Generated by Claude Code
| let mac = device_node_id(device); | ||
| if (mac) | ||
| node_id = mac; |
There was a problem hiding this comment.
node_id now comes exclusively from network.device status, and this is the only place that assigns it. If device_node_id() returns null — the device status carries no macaddr, or one the regex rejects — node_id stays unset and announce_local_reports() returns at line 369 on every announce tick, forever.
The resulting state is a receive-only node: remote reports keep getting installed, ubus call hostapd.<if> rrm_nr_list looks populated, hostapd is happy, and this AP simply never appears in any peer's list — with nothing in logread -e uneighbord saying why. That is a harder failure to diagnose than the socket ones, because everything else about the daemon still works.
Every other failure path in the file announces itself exactly once and remembers that it did: socket_failure in socket_fail(), and the new send_failure in announce_local_reports(). This one is the only silent one. Could it get the same treatment — a deduplicated ulog(LOG_WARNING, ...) when device_node_id(device) returns null and node_id is still unset?
Generated by Claude Code
|
|
||
| ## Requirements | ||
|
|
||
| `ucode`, `ucode-mod-ubus`, `ucode-mod-uloop`, `ucode-mod-socket`, `ucode-mod-uci`. |
There was a problem hiding this comment.
nit: ucode-mod-log is missing here now that the switch to ulog() made it a hard requirement — the Makefile lists it in DEPENDS at line 16 and the script imports it unconditionally at line 5, so a manual install without it fails at startup rather than degrading.
| `ucode`, `ucode-mod-ubus`, `ucode-mod-uloop`, `ucode-mod-socket`, `ucode-mod-uci`. | |
| `ucode`, `ucode-mod-ubus`, `ucode-mod-uloop`, `ucode-mod-socket`, `ucode-mod-uci`, `ucode-mod-log`. |
Generated by Claude Code
| PKG_VERSION:=1 | ||
| PKG_RELEASE:=1 | ||
|
|
||
| PKG_LICENSE:=GPL-2.0-only |
There was a problem hiding this comment.
Correcting myself: my previous comment asked you to drop PKG_LICENSE_FILES and the vendored LICENSE, and that is what turned the formality check red — it treats PKG_LICENSE_FILES as mandatory for a new package. Sorry for the round trip.
Both positions have a point, and the reason they collide is specific to this package: PKG_LICENSE_FILES names a path inside $(PKG_BUILD_DIR), and with no PKG_SOURCE there is no build dir to find LICENSE in, so the value is inert either way. That is presumably why the closest in-tree sibling, net/static-neighbor-reports/Makefile, sets PKG_LICENSE alone.
So it is a bot question rather than a code one, and your call: restoring LICENSE + PKG_LICENSE_FILES:=LICENSE clears the check at the cost of 340 lines of vendored GPL text, or the check gets an exemption for source-less packages. Either way the SPDX header in the script remains the authoritative statement.
Generated by Claude Code
uneighbord synchronizes hostapd 802.11k Neighbor Reports between local `hostapd.*` BSSes and other OpenWrt access points on the same LAN, over IPv6 link-local UDP multicast. Reports are only shared between BSSes with the same SSID, and installed through hostapd's ubus API. Signed-off-by: Nick Hainke <vincent@systemli.org>
3e01d19 to
36bf7af
Compare
📦 Package Details
Maintainer: me
Description:
uneighbord synchronizes hostapd 802.11k Neighbor Reports between local
hostapd.*BSSes and other OpenWrt access points on the same LAN, over IPv6 link-local UDP multicast. Reports are only shared between BSSes with the same SSID, and installed through hostapd's ubus API.🧪 Run Testing Details
✅ Formalities
If your PR contains a patch:
git am(e.g., subject line, commit description, etc.)
We must try to upstream patches to reduce maintenance burden.