Skip to content

uneighbord: add uneighbord - #30359

Open
PolynomialDivision wants to merge 1 commit into
openwrt:masterfrom
PolynomialDivision:add-uneighbord
Open

uneighbord: add uneighbord#30359
PolynomialDivision wants to merge 1 commit into
openwrt:masterfrom
PolynomialDivision:add-uneighbord

Conversation

@PolynomialDivision

@PolynomialDivision PolynomialDivision commented Aug 22, 2026

Copy link
Copy Markdown
Member

📦 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

  • OpenWrt Version:
  • OpenWrt Target/Subtarget:
  • OpenWrt Device:

✅ Formalities

  • I have reviewed the CONTRIBUTING.md file for detailed contributing guidelines.

If your PR contains a patch:

  • It can be applied using git am
  • It has been refreshed to avoid offsets, fuzzes, etc., using
    make package/<your-package>/refresh V=s
  • It is structured in a way that it is potentially upstreamable
    (e.g., subject line, commit description, etc.)
    We must try to upstream patches to reduce maintenance burden.

@openwrt

openwrt Bot commented Aug 22, 2026

Copy link
Copy Markdown

Formality Check: Failed

We checked this pull request against the contribution guidelines. Here is what needs your attention:

🛑 CRITICAL ERRORS

Commit 36bf7af - uneighbord: add uneighbord:

  • New OpenWrt package is missing the mandatory parameter: 'PKG_LICENSE_FILES'
  • File 'net/uneighbord/files/usr/sbin/uneighbord' is destined for '/etc/config/' but is not a valid UCI configuration file. In OpenWrt, '/etc/config/' is reserved for UCI-formatted configuration files. Raw files (such as TOML, JSON, or YAML) are not allowed at this path. Invalid line 3: '/* SPDX-License-Identifier: GPL-2.0-only */'

Tip

Do not close this pull request to make corrections. Instead, modify your existing commits (e.g. git commit --amend) and update the branch using git push --force-with-lease --force-if-includes. The checks will re-run automatically.


Something broken? Consider reporting an issue.
Running version 3eeda58 deployed on 2026-08-18 11:06:29 CEST

@openwrt-ai openwrt-ai left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Comment thread net/uneighbord/Makefile
Comment thread net/uneighbord/Makefile
Comment thread net/uneighbord/Makefile Outdated
Comment thread net/uneighbord/files/usr/sbin/uneighbord Outdated
Comment thread net/uneighbord/files/usr/sbin/uneighbord
const VERSION = 1;
const GROUP = "ff12::6e65:6967:6862:6f72:64";
const PORT = 32027;
const MAX_PACKET = 8192;

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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");

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@PolynomialDivision
PolynomialDivision force-pushed the add-uneighbord branch 2 times, most recently from b46ce98 to 3e01d19 Compare August 23, 2026 10:23

@openwrt-ai openwrt-ai left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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_id moved 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 on ensure_socket().
  • announce_local_reports() no longer bails on an empty local_bss. This is what lets peers clear a reloading AP's reports immediately instead of after PEER_TIMEOUT_S, and it works because discover_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

Comment on lines +284 to +286
let mac = device_node_id(device);
if (mac)
node_id = mac;

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Comment thread net/uneighbord/README.md Outdated

## Requirements

`ucode`, `ucode-mod-ubus`, `ucode-mod-uloop`, `ucode-mod-socket`, `ucode-mod-uci`.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Suggested change
`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

Comment thread net/uneighbord/Makefile
PKG_VERSION:=1
PKG_RELEASE:=1

PKG_LICENSE:=GPL-2.0-only

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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>
@PolynomialDivision PolynomialDivision changed the title [WIP] uneighbord: add uneighbord uneighbord: add uneighbord Aug 29, 2026
@PolynomialDivision
PolynomialDivision marked this pull request as ready for review August 29, 2026 16:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants