Add posix_spawn_file_actions_{addchdir,addfchdir,addclosefrom}_np
ClosedPublic
Actions

Authored by kib on Nov 28 2021, 2:35 AM.

Details

Reviewers

Commits

Summary

Same as in glibc

Repository

rG FreeBSD src repository

Lint

Lint Skipped

Unit

Tests Skipped

kib requested review of this revision.Nov 28 2021, 2:35 AM

Also test addfchdir, it is trivial to generalize the test..

Tests look good! Thank you 💚!

All of my comments are about best practices with atf(4) and other related questions/nits.

contrib/netbsd-tests/lib/libc/gen/posix_spawn/t_fileactions.c
437	Non-blocking nit: make /bin/pwd a constant, e.g., `PWD`. I looked up `_PATH_PWD` and it seems that it's already taken by pwd.h .
include/spawn.h
95	Nit: space between `np` and `(`.
lib/libc/gen/posix_spawn_file_actions_addopen.3
260	Non-blocking comment: is there a macro that could be used along with mentioning glibc? Also, should it glibc or GNU libc?

This revision is now accepted and ready to land.Nov 28 2021, 4:51 PM

contrib/netbsd-tests/lib/libc/gen/posix_spawn/t_fileactions.c
398	This breaks the ATF sandbox: it's better to use the current test directory and nest underneath it to ensure that atf can clean up all files leftover after a test run.
420–428	This could be done with `atf_utils_wait` instead.

Implementation looks good, just some manpage nits

lib/libc/gen/posix_spawn_file_actions_addopen.3
204
207
259

kib marked 6 inline comments as done.Nov 29 2021, 1:29 AM

kib added inline comments.

contrib/netbsd-tests/lib/libc/gen/posix_spawn/t_fileactions.c
398	In which way it breaks? The directory is used to chdir() into it, nothing more. There are no state stored in /tmp
420–428	I do not understand what you are proposing to do there.
lib/libc/gen/posix_spawn_file_actions_addopen.3
260	I do not think we have or would ever have such macro. Official GNU project is called `The GNU C Library (glibc)`, and they call themself `glibc`.

I handled all comments that I can understand.

This revision now requires review to proceed.Nov 29 2021, 1:30 AM

contrib/netbsd-tests/lib/libc/gen/posix_spawn/t_fileactions.c
398	Ah, got it. Yeah, it's not an issue then (only if files, etc, were created there). The whole thing about the ATF sandbox is to make sure that ATF can clean up all files/directories created by test cases. If a test case creates something outside that hierarchy, then there will be stale data kicking around the test host which can result in non-deterministic test behavior or resource utilization over time that could cause minor grief for the host running the test, requiring manual intervention from a sysadmin.
420–428	`atf_utils_wait` would eliminate the need for explicitly calling waitpid, fread, etc.

kib marked 3 inline comments as done.Nov 29 2021, 7:28 AM

kib added inline comments.

contrib/netbsd-tests/lib/libc/gen/posix_spawn/t_fileactions.c
420–428	atf_utils_wait() requires atf_utils_fork(), from what I see. It cannot be used to get stdout from arbitrary spawned child. And the whole point of this test is that it spawns the process using specific posix_spawn() invocation.