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.

[nightt5879]

Thanks for the review. I trust your judgment here.

The global list was added because once popen() returns the real fdopen() stream, pclose(FILE *stream) still needs a way to recover the shell pid for waitpid().

The alternatives I can see are:

1. keep external stream/fd -> pid state, as this PR does;
2. store popen metadata in NuttX FILE/stdio internals;
3. keep the old copied-FILE approach;
4. make a broader libc/helper API change.

I chose (1) to avoid copying FILE or modifying libc internals, but I understand this may not be the tradeoff you want for this issue.

What approach would you suggest here?

[xiaoxiang781216]

@michallenc added fopencookie before, you may attach the private data to FILE by this function:
https://github.com/apache/nuttx/pulls?q=fopencookie

[nightt5879]

Thanks for the pointer. I followed this direction and reworked the implementation to use fopencookie(): popen now stores the pipe fd and shell pid in the stream's private cookie instead of a global FILE-to-pid list. The cookie callbacks forward read/write/close to the fd, and pclose() retrieves the pid from the cookie before fclose()/waitpid(). The popen test remains in the second commit and passes on sim:nsh.