Port wolfSSH Zephyr test sample to Zephyr 4.4.0#1065
Conversation
- ClientFreeBuffers declared 'entry' under TERM && KBI but used it under KBI alone, so KBI-without-TERM failed to compile - widen the declaration guard to match the use
Zephyr 3.5 moved the RAM disk driver from Kconfig to devicetree and 4.1 replaced CONFIG_NET_SOCKETS_POSIX_NAMES with CONFIG_POSIX_API. Port the sample to the new APIs while keeping it buildable on older Zephyr. - add ramdisk.overlay (zephyr,ram-disk node) for the FS scenarios; drop CONFIG_DISK_RAM_VOLUME_SIZE - switch to CONFIG_POSIX_API; include the POSIX time/select/socket/netdb headers in test.h and port.h, gated on ZEPHYR_VERSION_CODE >= 4.1.0 so older Zephyr keeps the BSD names from <zephyr/net/socket.h> - select RAM disk (overlay >= 3.5, else CONFIG_DISK_RAM_VOLUME_SIZE) and POSIX (CONFIG_POSIX_API >= 4.1, else the legacy symbols) in CMakeLists.txt, with ramdisk_legacy.conf / posix_legacy.conf fallbacks - tests, nofs and kbi scenarios pass on qemu_x86 with Zephyr 3.4.0 (SDK 0.16.1) and 4.4.0 (SDK 1.0.0)
- add a v4.4.0 matrix leg alongside v3.4.0 - run each leg in the Zephyr CI image with the matching SDK (ci:v0.26.4/SDK 0.16.1, ci:v0.29.0/SDK 1.0.0); drops the manual deps, west, pip and SDK install steps - scope twister by --testsuite-root and drop --test: the scenario id is path-prefixed on 3.4.0 but bare on 4.x - replace zip with tar caf logs.tar.xz for the failure-log artifact
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #1065
Scan targets checked: wolfssh-bugs, wolfssh-src
No new issues found in the changed files. ✅
aidangarske
left a comment
There was a problem hiding this comment.
Skoll Multi-Scan Review
Modes: review + review-security + bugsOverall recommendation: REQUEST_CHANGES
Findings: 4 total — 4 posted, 0 skipped
4 finding(s) posted as inline comments (see file-level comments below)
Posted findings
- [High] [bugs] Unsupported job container entrypoint option breaks Zephyr CI —
.github/workflows/zephyr.yml:24-26 - [Low] [review] Version matrix lacks fail-fast: false, so one Zephyr leg failing cancels the other —
.github/workflows/zephyr.yml:13-22 - [Low] [review] RAM-disk selection keys off a substring match of the CONF_FILE filename —
zephyr/samples/tests/CMakeLists.txt:24 - [Low] [review] New config/overlay files open with multi-line description blocks contrary to repo comment convention —
zephyr/samples/tests/posix_legacy.conf:1-10
Review generated by Skoll
| - zephyr-ref: v4.4.0 | ||
| docker-image: v0.29.0 | ||
| runs-on: ubuntu-22.04 | ||
| container: |
There was a problem hiding this comment.
🔴 [High] Unsupported job container entrypoint option breaks Zephyr CI · API Contract Violations
The PR introduces a GitHub Actions job container and sets container.options to --entrypoint /bin/bash. GitHub's workflow syntax for jobs.\<job_id>.container.options explicitly documents that the --entrypoint (and --network) options are not supported. GitHub manages the job-container entrypoint itself to keep the container alive and exec each step into it, so overriding it via container options is unsupported and can prevent the container from starting or being kept alive before any west/twister step runs. The base workflow used no job container, so this regression is introduced by the PR and gates every Zephyr matrix leg. Verified against the referenced code: lines 24-26 add the container with the unsupported option.
Fix: Remove the unsupported option. If the image needs a neutral entrypoint, publish/use an image with that entrypoint baked in instead of overriding it through job-container options.
container:
image: ghcr.io/zephyrproject-rtos/ci:${{ matrix.config.docker-image }}
| @@ -13,34 +13,23 @@ jobs: | |||
| strategy: | |||
There was a problem hiding this comment.
🔵 [Low] Version matrix lacks fail-fast: false, so one Zephyr leg failing cancels the other · convention
The stated goal of the PR is to validate the sample on BOTH Zephyr 3.4.0 and 4.4.0. GitHub Actions matrix strategy defaults to fail-fast: true, which cancels all in-progress matrix legs as soon as one fails. If the 3.4.0 leg (the harder-to-keep-working legacy port) fails to build, the 4.4.0 leg will be cancelled before it finishes, hiding whether 4.4.0 still passes (and vice-versa). For a version-compatibility matrix you almost always want both results independently. Verified: the strategy: block contains only matrix: with no fail-fast: key.
Fix: Add fail-fast: false under strategy: so each Zephyr version reports its own pass/fail independently.
| # RAM disk: configured via the devicetree "zephyr,ram-disk" node on Zephyr | ||
| # >= 3.5, via Kconfig before that. Only the filesystem-backed scenarios need | ||
| # it; the nofs build (CONF_FILE="prj_nofs.conf") has no disk. | ||
| if(NOT "${CONF_FILE}" MATCHES "nofs") |
There was a problem hiding this comment.
🔵 [Low] RAM-disk selection keys off a substring match of the CONF_FILE filename · convention
if(NOT "${CONF_FILE}" MATCHES "nofs") decides whether to add the RAM disk purely from the presence of the substring "nofs" in the CONF_FILE filename. This works for today's three scenarios (prj.conf, prj_kbi.conf, prj_nofs.conf), but it is an implicit coupling: a future filesystem-less config that does not happen to contain "nofs" in its name would silently pull in the RAM disk overlay, and a filesystem config that happened to contain "nofs" would lose it. The intent ("this scenario has no filesystem") is not self-documenting from the filename convention alone. Verified present in CMakeLists.txt.
Fix: Optional: drive the RAM disk decision off an explicit toggle (e.g. a -DWOLFSSH_WITH_FS=y/n extra_arg per scenario in sample.yaml) rather than a filename substring, so the coupling is explicit. Not blocking given the current fixed scenario set.
| @@ -0,0 +1,14 @@ | |||
| # Pre-4.1 POSIX layer. | |||
There was a problem hiding this comment.
🔵 [Low] New config/overlay files open with multi-line description blocks contrary to repo comment convention · style
The new posix_legacy.conf, ramdisk_legacy.conf, and ramdisk.overlay each open with a multi-line file-header comment block, and CMakeLists.txt adds several multi-line comment blocks. The wolfSSL-ecosystem convention favors minimal comments — a one-line WHY when it would surprise a reader — rather than multi-line file-header/explanation blocks. The WHYs here (version boundaries, POSIX reorg) are genuinely worth a note, but could be condensed to a single line each, with the longer rationale living in the PR/commit description. Verified: posix_legacy.conf opens with a 10-line comment block, and CMakeLists.txt carries multiple multi-line explanatory blocks.
Fix: Optionally collapse the multi-line header blocks in the new .conf/.overlay files (and CMakeLists comments) to a single WHY line each; move the extended rationale to the PR description. Purely stylistic.
Brings the Zephyr test sample and its CI up to date with Zephyr 4.x APIs while keeping it buildable on the older 3.4.0 release.