system/popen: Avoid copying FILE

[nightt5879]

Summary

Fixes #2937.

system/popen was embedding a copied FILE object in its private tracking container and copying it back in pclose(). That depends on libc FILE layout details.

This PR makes popen() return the actual stream from fdopen() and keeps a small private FILE * -> pid tracking list so pclose() can find the spawned shell process without copying FILE.

Scope:

• Does not change supported popen() modes.
• Does not change the spawn/file-action setup.
• Keeps pclose() responsible for closing the stream and waiting for the shell pid.
• Returns EINVAL if pclose() cannot find the stream in the private popen list.

Impact

popen()/pclose() no longer rely on libc FILE internals.

• New feature: NO
• User adaptation required: NO
• Build process change: NO
• Hardware/architecture/board change: NO
• Documentation update required: NO
• Security impact: NO
• Compatibility impact: NO intended compatibility impact

Testing

Host:

• Windows with WSL Ubuntu 24.04
• Target: sim:nsh

Checks:

• git diff --check: pass
• checkpatch.sh -g HEAD~1..HEAD: pass
• sim:nsh build with CONFIG_SYSTEM_POPEN=y and CONFIG_EXAMPLES_POPEN=y: pass
  • confirmed CC: popen.c
  • result: SIM elf with dynamic libs archive in nuttx.tgz
• printf 'popen\npoweroff\n' | timeout 30s ./nuttx: pass
  • confirmed popen("help") output is read
  • confirmed execution reaches pclose()

[acassis]

@nightt5879 it would be nice to have some test coverage to test it too.

[nightt5879]

Thanks, I added a small testing/libc/popen test in commit 23a698f. @acassis

The test opens two popen(..., "r") streams, verifies the output read from both streams, and closes the first stream before the second. That covers reading from the returned popen() stream and also exercises the private stream-to-pid lookup/removal path added by this PR when the stream is not the first entry in the tracking list.

I verified the added test with:

• checkpatch.sh -g HEAD~1..HEAD
• sim:nsh build with CONFIG_SYSTEM_POPEN=y and CONFIG_TESTING_POPEN_TEST=y
• printf 'popen_test\npoweroff\n' | timeout 30s ./nuttx

The simulator run prints popen_test: successful.

[acassis]

Thanks, I added a small testing/libc/popen test in commit 23a698f. @acassis

The test opens two popen(..., "r") streams, verifies the output read from both streams, and closes the first stream before the second. That covers reading from the returned popen() stream and also exercises the private stream-to-pid lookup/removal path added by this PR when the stream is not the first entry in the tracking list.

I verified the added test with:

* `checkpatch.sh -g HEAD~1..HEAD`

* `sim:nsh` build with `CONFIG_SYSTEM_POPEN=y` and `CONFIG_TESTING_POPEN_TEST=y`

* `printf 'popen_test\npoweroff\n' | timeout 30s ./nuttx`

The simulator run prints popen_test: successful.

My bad, I looked only the first commit. So, everything is fine!

[nightt5879]

Thanks, I added a small testing/libc/popen test in commit 23a698f. @acassis
The test opens two popen(..., "r") streams, verifies the output read from both streams, and closes the first stream before the second. That covers reading from the returned popen() stream and also exercises the private stream-to-pid lookup/removal path added by this PR when the stream is not the first entry in the tracking list.
I verified the added test with:

* `checkpatch.sh -g HEAD~1..HEAD`

* `sim:nsh` build with `CONFIG_SYSTEM_POPEN=y` and `CONFIG_TESTING_POPEN_TEST=y`

* `printf 'popen_test\npoweroff\n' | timeout 30s ./nuttx`

The simulator run prints popen_test: successful.

My bad, I looked only the first commit. So, everything is fine!

Just to clarify: I added the additional commit after seeing your suggestion.
Thanks for reviewing it again!

[nightt5879]

@xiaoxiang781216 Thanks, I addressed the latest review comments in commit 6a16a4d.

The follow-up changes:

• changed the invalid-mode path to jump to errout_with_container, since no pipe fd exists there;
• avoided closing the stream fd twice after fopencookie() has taken ownership;
• removed the redundant NULL check before free();
• moved fd close, waitpid(), and cookie free into popen_file_close(), so pclose() now calls fclose() directly.

One caveat I noticed while moving waitpid() into the fopencookie close callback: fclose() treats any non-OK close callback return as an EOF/error result. Because of that, popen_file_close() now returns OK after close() and waitpid() themselves succeed, and reports only close()/waitpid() failures as errors. Otherwise a normal nonzero shell termination status could be converted into an fclose() failure.

I re-tested with:

• checkpatch.sh -f apps/system/popen/popen.c
• sim:nsh build with CONFIG_SYSTEM_POPEN=y and CONFIG_TESTING_POPEN_TEST=y
• popen_test, which prints successful

[xiaoxiang781216]

@nightt5879 please rebase your patch to fix the conflict, thanks.