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!

[xiaoxiang781216]

but it isn't good (or even worse) to add a global list to fix a minor issue.