testing, system, netutils: do not depend on fork() where it is absent or unneeded - #3673
testing, system, netutils: do not depend on fork() where it is absent or unneeded#3673casaroli wants to merge 2 commits into
Conversation
94d1583 to
e32964b
Compare
jerpelea
left a comment
There was a problem hiding this comment.
please replace
Co-Authored-By: Claude Opus 5 (1M context) noreply@anthropic.com
with
Assisted-by: Claude Opus 5 (1M context) noreply@anthropic.com
536349e to
2a694fe
Compare
2a694fe to
7aac6c5
Compare
|
Follow up pr #3685 |
7aac6c5 to
e295115
Compare
|
@xiaoxiang781216 I rewrote the PR so most of the complexity is moved to #3685 |
| CSRCS += $(wildcard libuv/test/test-*.c) | ||
| # test-fork.c and test-pipe-close-stdout-read-stdin.c call fork(). Every | ||
| # test they define is already excluded from the task list on NuttX by | ||
| # 0001-libuv-port-for-nuttx.patch, so they are dead code here. |
There was a problem hiding this comment.
should we keep fork related code if the arch support the fork
There was a problem hiding this comment.
I think we should try to enable this after we have real fork() working. We would need to fix the patch.
There was a problem hiding this comment.
if so, why not check CONFIG_ARCH_HAVE_FORK before removing them from build.
There was a problem hiding this comment.
Done — both the Makefile and CMakeLists.txt now drop the two files only when CONFIG_ARCH_HAVE_FORK is unset, so on every architecture today they are built exactly as before.
On the first question, though: keeping them where the arch supports fork() does not actually recover any coverage, because the exclusion is not keyed on fork availability. 0001-libuv-port-for-nuttx.patch widens the two #ifndef _WIN32 guards in test/test-list.h to #if !defined(_WIN32) && !defined(__NuttX__), and those guards cover the complete set of tests the two files define — all nine fork_* entries (fork_timer, fork_socketpair, fork_socketpair_started, fork_signal_to_child, fork_signal_to_child_closed, fork_fs_events_child, fork_fs_events_child_dir, fork_fs_events_file_parent_child, fork_threadpool_queue_work_simple) plus pipe_close_stdout_read_stdin. On NuttX they compile and link but are never entered.
I have taken the suggestion anyway, because gating is the better shape for two reasons that do not depend on that argument: it keeps this PR a strict no-op on master rather than nearly one, and it matches what testing/ltp does a few lines away instead of asking the reader to accept a claim about a vendored patch.
Verified on sim:citest under Linux GCC, building the same tree twice against a NuttX with and without the symbol: with CONFIG_ARCH_HAVE_FORK=y 170 libuv test files are compiled including both of these, and with it unset 168 are compiled and these two are the only ones missing.
3cab7a1 to
065682a
Compare
Two places call fork() from code that is compiled unconditionally, which is fine only for as long as every architecture provides it. NuttX is splitting fork() into three primitives -- see apache/nuttx#19562 -- after which ARCH_HAVE_FORK announces POSIX fork() specifically, and is off until an architecture implements it. Both then fail to link. Each is dropped only where ARCH_HAVE_FORK is unset, so builds that have fork() are unaffected. system/libuv: test-fork.c and test-pipe-close-stdout-read-stdin.c are filtered out of the test-*.c glob. Nothing is lost even where they are dropped: every test they define is already excluded from the task list on NuttX by 0001-libuv-port-for-nuttx.patch, which extends the _WIN32 guards around them to __NuttX__ -- all nine fork_* entries and pipe_close_stdout_read_stdin. They are compiled today but never run. testing/ltp: the open_posix_testsuite is filtered through LTP's existing BLACKWORDS mechanism, which already drops tests for absent features and is already conditioned on configuration symbols. The pattern spares vfork() and task_fork(). Where fork() is absent this drops 278 of 1943 test files; those tests exercise fork() and cannot link without it, and they return per architecture as fork() lands. Against today's master this is a no-op: ARCH_HAVE_FORK is set everywhere, so neither filter drops anything. It is part of what lets the NuttX side build against apps master. Assisted-by: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Marco Casaroli <marco.casaroli@gmail.com>
…ground. Neither of these wants fork() semantics. Both reach for fork() only to put work in the background, and each has a NuttX-native way to do that, so neither needs a fork primitive at all -- which matters once apache/nuttx#19562 makes ARCH_HAVE_FORK conditional on the architecture implementing POSIX fork(). netutils/dropbear: the port already routes every fork-then-exec through vfork(), because sysoptions.h selects DROPBEAR_VFORK when HAVE_FORK is undefined and the port leaves it undefined. spawn_command() in dbutil.c and both call sites in scp.c follow that switch. The one exception is the daemon() fallback that compat.c compiles under #ifndef HAVE_DAEMON, which calls fork() directly and bypasses it. NuttX provides daemon() in libs/libc/unistd/lib_daemon.c and declares it in unistd.h, so the fallback is redundant; define HAVE_DAEMON alongside the HAVE_STRLCAT and HAVE_STRLCPY entries that are there for exactly the same reason. The code was unreachable in any case -- the port hands svr_getopts() an argv containing -F, so svr_opts.forkbg is always zero and dropbear never calls daemon() at all. testing/drivers/nand_sim: forked so that the parent could return to the shell while the child registered the MTD device and slept forever. Nothing from before the fork is used after it, so the child is a self-contained entry point, and task_create() expresses that directly. The emulator body moves into nand_sim_daemon() unchanged. TESTING_NAND_SIM therefore needs no fork dependency, and the two sim configurations that enable it keep working whatever ARCH_HAVE_FORK is set to. Assisted-by: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Marco Casaroli <marco.casaroli@gmail.com>
065682a to
63b94b4
Compare
Summary
Four places in
appscallfork()from code that is compiled unconditionally. That is fine only for as long as every architecture provides it, and NuttX is about to stop doing so: apache/nuttx#19562 splitsfork()into three primitives, after whichCONFIG_ARCH_HAVE_FORKannounces POSIXfork()specifically and is off until an architecture implements it. Those four then fail to build, and the NuttX PR's CI cannot go green, because NuttX PRs build againstappsmaster.This PR removes that blockage and nothing else. It introduces no new configuration symbols, no compatibility layer, and no test changes.
It splits along a real line, one commit each. Two of the four are tests that genuinely exercise
fork()and so must stop being built where it is absent. The other two never wantedfork()at all — they reached for it only to put work in the background, and each has a NuttX-native way to do that, so they need no fork primitive and no new dependency.Tests that really do need
fork()system/libuv—test-fork.candtest-pipe-close-stdout-read-stdin.care filtered out of thetest-*.cglob whereCONFIG_ARCH_HAVE_FORKis unset, in both the Makefile and the CMake build. Nothing is lost even then, because every test the two files define is already excluded from the task list on NuttX:0001-libuv-port-for-nuttx.patchwidens the two#ifndef _WIN32guards intest/test-list.hto#if !defined(_WIN32) && !defined(__NuttX__), and those guards cover all ninefork_*entries andpipe_close_stdout_read_stdin— the complete set both files define. They are compiled today but never run.testing/ltp— theopen_posix_testsuiteis filtered through LTP's existingBLACKWORDSmechanism, which already drops tests for absent features and is already conditioned on configuration symbols. The pattern[^v_]fork(sparesvfork()andtask_fork(). Wherefork()is not provided this drops 278 of 1943 test files; where it is provided — everywhere, today — nothing is dropped. That loss is the honest price of the split: those tests exercisefork()and cannot link without it. They return per architecture as realfork()lands.Callers that only wanted to run in the background
netutils/dropbear— the port already routes every fork-then-exec throughvfork(), and has done since it was written:sysoptions.hselectsDROPBEAR_VFORKwhenHAVE_FORKis undefined, and the port leaves it undefined, sospawn_command()indbutil.cand both call sites inscp.ctake thevfork()branch. The single exception is thedaemon()fallback thatcompat.ccompiles under#ifndef HAVE_DAEMON, which callsfork()directly and bypasses that switch. NuttX hasdaemon()inlibs/libc/unistd/lib_daemon.cand declares it inunistd.h, so the fallback is redundant — this definesHAVE_DAEMONalongside theHAVE_STRLCATandHAVE_STRLCPYentries that are innuttx_config.hfor exactly the same reason. The code was unreachable in any case: the port handssvr_getopts()an argv containing-F, sosvr_opts.forkbgis always zero and dropbear never callsdaemon()at all.testing/drivers/nand_sim— forked so the parent could return to the shell while the child registered the MTD device and slept forever. Nothing from before thefork()is used after it, so the child is a self-contained entry point andtask_create()expresses that directly; the emulator body moves intonand_sim_daemon()unchanged.TESTING_NAND_SIMtherefore needs no fork dependency and itsKconfigis untouched, sosim:nandandsim:mnemofskeep working whateverARCH_HAVE_FORKis set to — no defconfig edits are needed on either side, and no coverage is lost at any point in the merge order.Two other
fork()mentions need nothing:games/NXDoom's is inside#if 0 /* UNUSED */, andsystem/syslogdalready usesposix_spawn().Impact
Against today's master this is a no-op, with nothing left over.
CONFIG_ARCH_HAVE_FORKis set on every architecture, so neither the LTP filter nor the libuv filter drops anything;nand_simand dropbear behave identically either way, sincetask_create()anddaemon()do not depend on the symbol at all. Every configuration builds exactly the same set of files it does today.After apache/nuttx#19562,
ARCH_HAVE_FORKgoes off until a per-architecture PR turns it back on. The two test suites stop being built rather than failing to link;nand_simand dropbear are unaffected.interpreters/basis deliberately left alone. ItsSHELLandEDITstatements want the same treatment, butcheckpatch.shchecks the whole of any file a patch touches, andbas_statement.cproduces 1681 pre-existing findings before this patch is applied at all — a one-newline commit against master fails CI identically. Migrating BAS has to follow a style cleanup of that file, and neither belongs here. The consequence is small:CONFIG_EXAMPLES_BAS_SHELLisEXPERIMENTALand alreadydepends on ARCH_HAVE_FORK, so it becomes unselectable rather than misbehaving.The companion test work — splitting
ostest's fork test intotask_fork,vforkandfork— is #3685, which depends on apache/nuttx#19562 and must merge after it.Testing
Host: macOS 15 (Darwin 25.5.0) on Apple Silicon. QEMU 11.0.3, xPack
riscv-none-elf-gcc14.2.0-3, Arm GNUarm-none-eabi-gcc14.2.Rel1.Style
../nuttx/tools/checkpatch.sh -c -u -m -g <base>..HEAD, the exact command.github/workflows/check.ymlruns — ✔️ All checks pass, withcodespell,cvt2utf,cmake-formatandnxstyleall installed.Verification
This PR was restructured after review: the test work it originally carried has moved to #3685, leaving only the changes needed to unblock apache/nuttx#19562, and the branch has been rebased onto current master. The functional matrix is being re-run against the restructured branch and will be posted here; the previous revision's results are in this PR's history.
The three configurations CI reported failing on apache/nuttx#19562 —
sim:nand,sim:mnemofsandsim:dropbear— were rebuilt against that PR's branch, whereCONFIG_ARCH_HAVE_FORKis genuinely off, under Linux GCC rather than the development host's compiler.At this PR's base the two reported diagnostics reproduce at exactly the locations CI gives:
With this PR applied both are gone — zero occurrences across all three configurations — and
sim:dropbearbuilds and links clean with no warnings at all. So the change is measured against the real failure rather than an approximation of it.By inspection the change is a no-op against today's master:
CONFIG_ARCH_HAVE_FORKis set on every architecture, so the LTP filter drops nothing, and the two libuv files removed from the glob are already absent from the task list on NuttX.