prplmesh: add package - #30071
Conversation
7eae958 to
e33f272
Compare
openwrt-ai
left a comment
openwrt-ai
left a comment
There was a problem hiding this comment.
Reviewed 1 new commit; 1 inline issue flagged. The OpenWrt packaging (Makefile deps libnl-core/libnl-genl/libnl-route etc. all resolve, init/config look consistent) is fine; the flagged issue is in the bundled upstream patch. CI on the head commit is still pending, so nothing to ground CI-side.
Generated by Claude Code
cc7506b to
365abdd
Compare
365abdd to
6f40907
Compare
67aff22 to
737a174
Compare
| HOSTAPD_SOURCE_DATE:=2025.08.26 | ||
| HOSTAPD_SOURCE_VERSION:=ca266cc24d8705eb1a2a0857ad326e48b1408b20 | ||
| HOSTAPD_SOURCE:=hostapd-$(HOSTAPD_SOURCE_DATE)~$(call version_abbrev,$(HOSTAPD_SOURCE_VERSION)).tar.zst | ||
| HOSTAPD_SOURCE_URL:=https://w1.fi/hostap.git | ||
| HOSTAPD_SOURCE_PROTO:=git | ||
| HOSTAPD_MIRROR_HASH:=59ac677093f524ff98588abd9f33805a336a6e929d6814222f0d784c854f2343 | ||
| HOSTAPD_BUILD_ROOT:=$(BUILD_DIR)/$(PKG_NAME)-hostapd-source | ||
| HOSTAPD_BUILD_DIR:=$(HOSTAPD_BUILD_ROOT)/hostapd-$(HOSTAPD_SOURCE_DATE)~$(call version_abbrev,$(HOSTAPD_SOURCE_VERSION)) | ||
| HOSTAPD_OPENWRT_CTRL_PATCH:=$(CURDIR)/files/610-hostapd_cli_ujail_permission.patch | ||
| HOSTAPD_WPA_CTRL_HASHER:=$(STAGING_DIR_HOST)/bin/mkhash | ||
| HOSTAPD_WPA_CTRL_RAW_SHA256:=482839d354b9c53e19e6a766ff5be054980607dd94e53efee9762dbda40f0f26 | ||
| HOSTAPD_WPA_CTRL_PATCHED_SHA256:=ad925b710901b3f2f2188afdab5a7f181913e48b159a824c30e0c19d2acd53fa |
There was a problem hiding this comment.
I have strong concerns regarding bundling/vendoring a custom, frozen copy of hostapd inside this package. As we can see, the version used here is already old and missing critical security fixes that are present in the core OpenWrt repo (see hostapd commits), where it is actively updated.
Additionally, it introduces unnecessary custom variables that shouldn't be there and just make the code harder to maintain.
Could you please consider another solution that fits your needs while reusing the system hostapd?
There was a problem hiding this comment.
Yes. To clarify, the package does not install or run the downloaded hostapd; that source is only used at build time for the control client, while runtime connects to the existing wpad. Still, I agree that freezing a second copy, duplicating OpenWrt's 610 patch, and installing under /opt are not acceptable maintenance choices here. I'll rework the integration to avoid the vendored hostapd source and use standard OpenWrt paths before requesting another review.
737a174 to
8ee5c37
Compare
8ee5c37 to
f30a23d
Compare
|
@BKPepe thanks for the review — reworked as requested, all points addressed:
Could you take another look? CI status: everything is green on the current head � FormalityCheck and the full multi-arch test build, including the container install/runtime checks. |
109477a to
ac913e0
Compare
|
|
||
| include $(INCLUDE_DIR)/package.mk | ||
| include $(INCLUDE_DIR)/cmake.mk | ||
| include ../../lang/python/python3-host.mk |
There was a problem hiding this comment.
Unfortunately Python cannot be avoided here: prplMesh's tlvf code generator (framework/tlvf/tlvf.py) runs at build time and produces the IEEE 1905 / EasyMesh TLV C++ classes from 225 YAML definitions (two generator trees, framework/tlvf and common/beerocks/tlvf), and it does import yaml — so a host Python with PyYAML is a hard build requirement. The only way around it would be committing hundreds of generated sources as patches, which would be far worse to maintain.
PKG_BUILD_DEPENDS:=python-yaml/host stages PyYAML, and python3-host.mk provides HOST_PYTHON3_BIN, which is passed to CMake as Python3_EXECUTABLE so the build uses the staged interpreter instead of whatever python happens to be on the build host — the same pattern samba4, openvswitch, ovn, meshtasticd and fwupd use in this feed.
ac913e0 to
436095f
Compare
openwrt-ai
left a comment
openwrt-ai
left a comment
There was a problem hiding this comment.
Reviewed the rework since 07357a8 (branch is a single squashed commit again, so I diffed 07357a8..7f960c6 restricted to the package directory).
The rename to plain prplmesh, the switch to INSTALL_BIN/INSTALL_DATA/INSTALL_CONF and the flattened /usr/share/prplmesh layout all look right to me. I checked the upstream install rules against a prplMesh tree: share/ receives exactly one regular file (prplmesh_platform_db, installed unconditionally from framework/platform/bpl/platform_db), and config/ only receives regular files here because the two install(DIRECTORY … DESTINATION config) rules that would add config/odl/ sit behind ENABLE_NBAPI, which this package disables — so neither INSTALL_DATA … /* glob can hit a directory and fail. The /usr/libexec/prplmesh/{share,config} relative symlinks still resolve. config/version is generated from version.in as prplmesh_version=@prplmesh_VERSION@ and -DPRPLMESH_VERSION=$(PKG_VERSION) feeds it, so the new test-version.sh is checking a string that really is in the shipped file. diagnostics_measurements is a real UCI key read by cfg_get_diagnostics_measurements() (upstream default 1), and the init-script reordering does now complete every preflight before the first register_process, which resolves the earlier half-registration issue.
One real finding plus two nits inline; nothing here looks like a merge blocker to me. Dropping EXTRA_CXXFLAGS += -Wno-error=deprecated-declarations and not installing the prplOS helper scripts both check out.
Generated by Claude Code
|
You're right: the archived link is not useful review evidence, and I don't have a defensible timeline for upstream acceptance yet. I'll submit the downstream patches upstream first (draft MRs are enough to start that review), replace the dead reference with public links, and only ask for another OpenWrt review once those links exist. |
91d9c94 to
9d2439f
Compare
466a18f to
d314827
Compare
82af2b3 to
a65b72f
Compare
Add the prplMesh 6.0.1 EasyMesh controller and agent for stock OpenWrt. The package keeps the installed hostapd/wpad stack and drives it over the existing UCI, ubus, nl80211 and hostapd control sockets. The hostapd control interface client is a small self-contained implementation added by patch 120, so no hostapd source tree is downloaded, bundled or duplicated at build time and the system hostapd stays the only hostapd. Binaries install under /usr/libexec/prplmesh, read-only data under /usr/share/prplmesh, shared libraries under /usr/lib, configuration under /etc/config/prplmesh. The prplOS helper scripts are not installed: their platform detection does not know stock OpenWrt and nothing on this platform invokes them. Stock builds run without NBAPI, so the controller skips the CurrentOperatingClassProfile data-model cleanup for radios without a data model path, and legacy vendor diagnostic measurements default to off while standard EasyMesh metrics stay enabled. The controller also registers radios that appear only in Topology Responses: some stock agents never run WSC onboarding for a band (Wi-Fi 7 units and their 6 GHz radio), and such radios, their BSSes and their clients stayed invisible otherwise. The init preflights every binary the selected management mode needs before registering any procd instance, and the example credentials ship empty so an enabled but unconfigured service fails validation loudly instead of deploying a published passphrase. A test-version.sh verifies the installed version from the shipped version file instead of probing daemons. An earlier revision of this integration, differing only in the wpa_ctrl client origin and the install prefix, runs on OpenWrt 25.12.5 mediatek/mt7622 with two Ethernet-connected EasyMesh agents. Signed-off-by: Andrei Ovcharenko <a@krot.name>
4a8f175 to
a44a8e7
Compare
|
@BKPepe upstream status: prpl moved to Jira Cloud and their GitLab does not accept merge requests from non-members, so the series is filed as PCI-107 and is publicly reviewable at https://gitlab.com/kreout/prpl-mesh-mercusys/-/merge_requests/1. Every patch header now carries that link instead of the dead archived one. CI is green on the current head — could you take another look? |
openwrt-ai
left a comment
openwrt-ai
left a comment
There was a problem hiding this comment.
Reviewed the rework since 7f960c6 (the branch is a single squashed commit again, so I diffed 7f960c6..a44a8e7 restricted to net/prplmesh/). New since then: the upstream-status paragraphs in 100/110/120/130, the extra encryption-must-be-explicit guard in 110, the grep -q fix in test-version.sh, and two new patches, 140 and 150.
Everything from the previous round checks out: test-version.sh now leaves the matched version line in the CI log as CONTRIBUTING.md suggests, and all five patch headers have the blank line before Signed-off-by:. The 110 guard is a real fail-closed improvement — an explicit bss_profile with no encryption option is now rejected before authentication_from_uci() can map it to something permissive, and it fails the whole profile set the same way the neighbouring validation errors do. Patch 140 reads correctly to me: the if (!radio) block in handle_topology_response now falls through to radio->bsses.keep_new_prepare() instead of continue, and the FREQ_UNKNOWN branch in handle_current_op_class only fires for radios created that way, since WSC-onboarded radios always carry a band.
Patch 150 is where my comments are: one encoding defect that ships a non-UTF-8 byte into the patched source, one behavioural issue in the new stale-disconnect reconciliation pass, and one nit about the two near-duplicate sweep loops. Nothing here looks like a merge blocker to me except possibly the reconciliation false-disconnect, which is worth a look because the new 60 s timer makes it recur rather than happen once at startup. CI is green on a44a8e70 across all ten target builds plus the formality checks, so nothing CI-grounded.
Commit checks
a44a8e7"prplmesh: add package" — the body enumerates the behavioural patches individually (the NBAPI data-model cleanup from 130, the Topology-Response radio registration from 140, the init preflight, the empty example credentials) but never mentions patch 150, which is the largest single piece of new code in the diff at 677 lines and changes agent runtime behaviour (a periodic 60 s client-reconciliation sweep on the nl80211 AP and monitor HALs). Worth a sentence in the commit body. The PR description has the matching slip in the other direction — "Two stock-build fixes added as patches 130/140/150" introduces three patches.
Generated by Claude Code
Have you double check what AI does for you? It leads to page not found - 404. |
|
You're right. I rechecked the link in a signed-out browser and it returns 404, so it is not publicly reviewable. I shouldn't have presented it as public evidence. I won't ask for another review until I can provide a genuinely public upstream reference. |
908dc9d to
d1c26af
Compare
Reject incomplete nl80211 client snapshots on transient backend failures. Preserve valid topology state and invalidate stale events before reconciliation. Signed-off-by: Andrei Ovcharenko <a@krot.name>
d1c26af to
c8fb83c
Compare
openwrt-ai
left a comment
openwrt-ai
left a comment
There was a problem hiding this comment.
Reviewed the one new commit since a44a8e7 (c8fb83c), which touches only 150-nl80211-connected-clients-import.patch.
The reconciliation rework is the right shape: the pre-sweep snapshot invalidated by runtime connect/disconnect events closes the concurrent-association window, m_enumerated_vaps gates stale disconnects on VAPs that actually reached end-of-list, and splitting UNKNOWN out of the end-of-list branch matches hostapd's ctrl_iface semantics. Hoisting looks_like_mac() into base_wlan_hal_nl80211 gives the monitor sweep the same validation as the AP sweep, and the three stray 0x97 bytes are gone — the file is pure ASCII now.
Three findings inline: the patch is no longer quilt-refreshed, which is what all ten CI builds are failing on; the looks_like_mac() hoist looks like it breaks the build in ap_wlan_hal_nl80211.cpp, which CI has not reached yet; and one remaining path where a partially enumerated VAP is still treated as clean. The first two look like merge blockers to me — the third is a rare-path correctness issue with a one-line fix.
Commit checks
c8fb83c"prplmesh: make client reconciliation failure-safe" — the body covers the two behavioural changes (reject incomplete snapshots, invalidate stale events before reconciliation) but not two other things the diff does: it moveslooks_like_mac()out ofap_wlan_hal_nl80211.cppintobase_wlan_hal_nl80211so both HALs validate MACs identically, and it regenerates the whole patch file in git format (ninediff --git/indexblocks, reflowed context), which accounts for most of the 250 changed lines and is what CI is failing on. Both are worth a line in the message. Separately, the PR description still says the branch is "still squashed to one commit" while it now carries two — since this commit only fixes a patch introduced bya44a8e7in the same PR, squashing it back before merge would keep the history to the single "prplmesh: add package" commit the description promises.
Generated by Claude Code
| @@ -0,0 +1,747 @@ | |||
| From 0000000000000000000000000000000000000000 Mon Sep 17 00:00:00 2001 | |||
There was a problem hiding this comment.
This revision regenerated the patch with git rather than refreshing it, and the result no longer round-trips through quilt. That is what all ten build jobs on c8fb83cf fail on, before a single source file is compiled (mips_24kc log, the other nine are identical):
Applying patch 150-nl80211-connected-clients-import.patch
...
Now at patch 150-nl80211-connected-clients-import.patch
Refreshed patch 150-nl80211-connected-clients-import.patch
Dirty patches detected, please refresh and review the diff
##[error]Process completed with exit code 1.
100–140 all report is unchanged in the same log; only 150 is rewritten by the refresh. Three differences from the other five patches in the series, all introduced by this commit: the file now carries nine diff --git + index header pairs, its 33 empty context lines lost the leading space a unified diff needs (siblings have 25–45 lines of the ^ $ form and zero bare-empty ones), and this From line is a placeholder all-zero SHA where 100–140 carry real hashes.
make package/prplmesh/refresh regenerates the diff body in the tree's quilt style and drops the git-only headers. It preserves everything above the diff, though, so the placeholder SHA on this line needs fixing by hand.
Generated by Claude Code
| + if (!line.empty() && line.back() == '\r') { | ||
| + line.pop_back(); | ||
| + } | ||
| + if (!looks_like_mac(line)) { |
There was a problem hiding this comment.
This call site no longer has a visible looks_like_mac. The commit deleted the file-local static bool looks_like_mac() from ap_wlan_hal_nl80211.cpp and re-added it as a protected static member of base_wlan_hal_nl80211 (declaration at line 564, [definition at line 535](https://github.com/openwrt/packages/blob/c8fb83cf0bbabc8b76f47ef11a795252acb1d8d4/net/prplmesh/patches/150-nl80211-connected-clients-import.patch#L535``)) — but parse_sta_block() is a file-scope static free function, not a member. Unqualified lookup from namespace scope never finds a class member, and the member is protected, so even spelling it base_wlan_hal_nl80211::looks_like_mac(line) would be rejected from a non-member. This should fail to compile with 'looks_like_mac' was not declared in this scope. The monitor-side call at line 658 is fine — it sits in a member function of a derived class.
CI stops at the dirty-patch check above and never reaches the compile step, so nothing has exercised this yet.
Options, depending on how you want to share the helper: keep a file-local copy in ap_wlan_hal_nl80211.cpp and only share it if both HALs really need one implementation; or declare it as a free function in base_wlan_hal_nl80211.h (inside namespace bwl::nl80211) so both a free function and a member can call it; or make the member public and turn parse_sta_block() into a private static member of ap_wlan_hal_nl80211.
Generated by Claude Code
| + m_prev_client_mac = beerocks::net::network_utils::ZERO_MAC; | ||
| + continue; | ||
| + } | ||
| + vap_enumerated = true; |
There was a problem hiding this comment.
One path still marks a partially walked VAP as cleanly enumerated. Reaching this line means either the reply was empty (real end of list) or it was FAIL with the restart budget already spent — m_vap_restart_count at 2 after two restarts, so the guard above falls through on the third consecutive FAIL. hostapd returns FAIL from STA-NEXT only when the cursor address is no longer a known station; genuine end-of-list from both STA-FIRST and STA-NEXT is an empty reply, which is exactly what the restart comment above assumes. So in the FAIL case the VAP enters m_enumerated_vaps with only a prefix of its stations in m_handled_clients, and the reconciliation loop below then pushes a stale STA_Disconnected for every client past the failing cursor — the same false disconnect this commit sets out to remove, just on the rarer path.
| + vap_enumerated = true; | |
| + vap_enumerated = text.empty(); |
Generated by Claude Code
There was a problem hiding this comment.
Pull request overview
Adds a new prplmesh package (prplMesh 6.0.1) to OpenWrt packages, targeting “stock OpenWrt” integration by driving the existing hostapd/wpad stack via UCI/ubus/nl80211 and hostapd control sockets, plus a set of upstream patches to make this mode work reliably.
Changes:
- Introduces
net/prplmeshpackaging (CMake-based build, install layout, init script, default UCI config). - Adds a
test-version.shthat verifies the installed version via/usr/share/prplmesh/config/version. - Carries a patch series to support stock OpenWrt UCI/nl80211 operation and fixes around topology handling and connected-client import/reconciliation.
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| net/prplmesh/Makefile | New package definition for prplMesh 6.0.1 (CMake build options, install layout, deps). |
| net/prplmesh/files/prplmesh.init | procd init script to start transport/controller/agent with preflight checks and hostapd socket wait loop. |
| net/prplmesh/files/prplmesh.config | Default UCI config including explicit BSS profiles with blank SSID/key placeholders. |
| net/prplmesh/test-version.sh | Package version verification script for CI. |
| net/prplmesh/patches/100-stock-platform.patch | Enables a stock OpenWrt platform mode (UCI/ubus without Ambiorix/WHM) and related glue. |
| net/prplmesh/patches/110-stock-wifi-hardening.patch | Hardens/extends stock UCI Wi-Fi profile parsing and adds helper utilities + unit tests. |
| net/prplmesh/patches/120-bwl-bundled-wpa-ctrl-client.patch | Adds a bundled minimal wpa_ctrl-compatible client for hostapd control sockets. |
| net/prplmesh/patches/130-skip-op-class-dm-cleanup-without-nbapi.patch | Avoids data-model cleanup spam when NBAPI is disabled. |
| net/prplmesh/patches/140-register-foreign-radios-from-topology.patch | Registers radios from Topology Response when WSC onboarding never occurs for a band. |
| net/prplmesh/patches/150-nl80211-connected-clients-import.patch | Implements nl80211 connected-client sweep/import + periodic reconciliation behavior. |
Suppressed comments (1)
net/prplmesh/patches/110-stock-wifi-hardening.patch:510
- The
#endifcomment should match the updated include-guard macro name to avoid confusion during future edits/searches.
+#endif // _BPL_CFG_WIFI_UTILS_H_
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| + | ||
| + uint32_t ht_mcs = 0; | ||
| + for (uint8_t i = 0; i < 4; i++) { | ||
| + ht_mcs |= HT_MCS[i] << (8 * i); |
| + /* Group first so restricted processes still hand over what they can; | ||
| + * when even that fails the socket keeps working for a root daemon. */ | ||
| + if (lchown(path, (uid_t)-1, gid) != 0 || lchown(path, uid, gid) != 0) { | ||
| + /* best effort by design */ | ||
| + } |
| +#ifndef _BPL_CFG_WIFI_UTILS_H_ | ||
| +#define _BPL_CFG_WIFI_UTILS_H_ |
| + counter++; | ||
| + | ||
| + wpa_ctrl_grant_daemon_access(ctrl->local.sun_path); | ||
| + |
📦 Package Details
Maintainer: @krotname
Description:
Adds the prplMesh 6.0.1 EasyMesh controller and agent for stock OpenWrt
(package
prplmesh, no-stocksuffix — no variants exist yet). It keepsthe installed hostapd/wpad stack and drives it over the existing UCI, ubus,
nl80211 and hostapd control sockets.
Changes in this revision (review follow-up, still squashed to one commit):
prplmesh-stockto plainprplmesh; install usesthe
INSTALL_BIN/INSTALL_DATA/INSTALL_CONFmacros instead of rawcp/install, and theEXTRA_CXXFLAGS += -Wno-error=deprecated-declarationsflag is gone (it only guarded the now-removed vendored hostapd).
/usr/libexec/prplmesh; read-only data flattened directlyunder
/usr/share/prplmeshwith a single symlink back from libexec soprplMesh's own install-path discovery keeps working; shared libraries in
/usr/lib; UCI configuration in/etc/config/prplmesh. The prplOS helperscripts/directory is not installed at all — its platform detectiondoesn't know stock OpenWrt and nothing on this platform calls it.
test-version.shverifies the installed version from the shippedconfig/versionfile instead of the generic daemon-probe test thatproduced spurious
[warn]output againstieee1905_transportin CI.prplmesh.init: every mode-specific preflight check (required binariespresent, mandatory interfaces set) now runs and logs before the first
procdservice registration, so a failed preflight can no longer leave ahalf-registered service running.
inert-but-real passphrase, so enabling the service without deliberate
credentials fails validation instead of silently deploying a published PSK.
cleanup when NBAPI is disabled (was flooding the log), register radios a
stock agent never runs WSC onboarding for (seen with Wi-Fi 7 6 GHz radios)
from their Topology Response instead of dropping every report about them,
and import/reconcile already-connected clients on the nl80211 HAL at
startup instead of only picking up clients that (re)associate afterwards.
series: https://gitlab.com/kreout/prpl-mesh-mercusys/-/merge_requests/1
(a merge request on a public fork of upstream prplMesh, replacing the
dead archived link). A direct upstream merge request is not possible for
non-members —
prpl-foundation/prplmesh/prplMeshhasmerge_requests_access_level: private, so the upstream submissionitself goes through the prpl Foundation Jira, as noted in review:
filed as PCI-107.
🧪 Run Testing Details
aarch64_cortex-a53)Validation:
verify at the current head.
make package/prplmesh/refreshidempotence and a snapshot x86_64 SDKbuild verified locally.
both Active/Reachable; four radios visible across channels 2/40/11/36.