Page MenuHomeFreeBSD

Add posix_spawn_file_actions_{addchdir,addfchdir,addclosefrom}_np
ClosedPublic

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

Diff Detail

Repository
R10 FreeBSD src repository
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

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

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

ngie accepted this revision.EditedNov 28 2021, 4:51 PM

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 โ†—(On Diff #99095)

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 โ†—(On Diff #99095)

Nit: space between np and (.

lib/libc/gen/posix_spawn_file_actions_addopen.3
260 โ†—(On Diff #99095)

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 โ†—(On Diff #99095)

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 โ†—(On Diff #99095)

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 โ†—(On Diff #99095)
207 โ†—(On Diff #99095)
259 โ†—(On Diff #99095)
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 โ†—(On Diff #99095)

In which way it breaks? The directory is used to chdir() into it, nothing more. There are no state stored in /tmp

420โ€“428 โ†—(On Diff #99095)

I do not understand what you are proposing to do there.

lib/libc/gen/posix_spawn_file_actions_addopen.3
260 โ†—(On Diff #99095)

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.

kib marked an inline comment as done.

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 โ†—(On Diff #99095)

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 โ†—(On Diff #99095)

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 โ†—(On Diff #99095)

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.

kevans added inline comments.
lib/libc/gen/Symbol.map
439 โ†—(On Diff #99124)

Maybe re-sort FBSD_1.7 pre-commit? I note, e.g.. FBSD_1.2 is sorted despite splitting up *utx*symbols. I don't feel strongly about it, though.

This revision is now accepted and ready to land.Nov 30 2021, 1:09 AM
kib marked 2 inline comments as done.Nov 30 2021, 1:16 AM