Page MenuHomeFreeBSD

atf: Don't be deterred by weird umasks.
ClosedPublic

Authored by des on May 9 2025, 10:42 AM.
Tags
None
Referenced Files
Unknown Object (File)
Wed, Jul 9, 4:56 AM
Unknown Object (File)
Tue, Jul 8, 11:56 PM
Unknown Object (File)
Tue, Jul 8, 8:53 PM
Unknown Object (File)
Tue, Jul 8, 2:14 PM
Unknown Object (File)
Tue, Jul 8, 3:37 AM
Unknown Object (File)
Tue, Jul 8, 2:30 AM
Unknown Object (File)
Sun, Jul 6, 6:29 AM
Unknown Object (File)
Sat, Jul 5, 9:21 PM

Details

Summary

If the current umask is weird, ATF goes to great lengths to tell us that
it can't possibly work in these conditions, instead of just dealing with
it. This makes it unreasonably hard to use ATF to test how our own code
handles unusual umasks.

MFC after: 1 week
Sponsored by: Klara, Inc.

Diff Detail

Repository
rG FreeBSD src repository
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

des requested review of this revision.May 9 2025, 10:42 AM
igoro added a subscriber: igoro.

It looks fine that atf-check wants to verify that its temporary dir has enough access rights as it needs to store output files beneath. Why does it add so many code to handle it explicitly? Probably, a usual noperm file system error does not propagate nicely and leaves a test result reader guessing about the actual root cause. Why doesn't it "escape" the current umask instead (as this patch proposes)? Maybe, there was another reason to obey the current umask when ATF was just a separate project before Kyua introduction and it was invoked directly. I do not see answers in the code parts I've read.

In our case (the FreeBSD project) it is expected to be run with kyua which also creates its own tmp dir, i.e. if kyua succeeds to do that then atf-check is expected to succeed as well, as the latter ends up working within the kyua tmp dir, e.g. /tmp/kyua.XXXXXX/2/work/check.XXXXXX. The detail/fs.c code is a part of atf-c lib which is part of atf-c++ lib as well, and I see only two entry points here: atf_fs_mkdtemp() and atf_fs_mkstemp(). The first one seems to be used solely for atf check temporary directory creation, and the second one seems to be not used at all (just for self-testing). Therefore, it feels fine to remove this umask checking business and ignore "user" part of the current umask during tmp dir creation.

By the way, just in case if we have doubts removing this, or there are debates, or if we want to keep more parity with the upstream then as Plan B the whole patch could be replaced with this alternative:

diff --git a/bin/cp/tests/cp_test.sh b/bin/cp/tests/cp_test.sh
index 29dce783ffe2..11d2bad82743 100755
--- a/bin/cp/tests/cp_test.sh
+++ b/bin/cp/tests/cp_test.sh
@@ -562,18 +562,7 @@ dstmode_body()
 {
        mkdir -m 0755 dir
        echo "foo" >dir/file
-       umask 0177
-       #atf_check cp -R dir dst
-#begin
-       # atf-check stupidly refuses to work if the current umask is
-       # weird, instead of just dealing with the situation
-       cp -R dir dst >stdout 2>stderr
-       rc=$?
-       umask 022
-       atf_check_equal 0 $rc
-       atf_check cat stdout
-       atf_check cat stderr
-#end
+       atf_check -x 'umask 0177 && cp -R dir dst'
        atf_check -o inline:"40600\n" stat -f%p dst
        atf_check chmod 0750 dst
        atf_check cmp dir/file dst/file
This revision is now accepted and ready to land.May 10 2025, 11:47 PM
kevans added subscribers: jmmv, kevans.

I do wonder if @jmmv has an opinion, if he still gets/reads phab e-mail there, but I don't see off-hand why atf shouldn't try to function in these kinds of environments.

None of this would be needed if atf didn't insist on creating a temporary directory to contain temporary files to store the output of the program it's testing. It could simply create temporary files in $TMPDIR, or store the output in memory. This is what you get when you put someone's master's thesis into production without a reality check...

I'd still like someone from tests to weigh in, @ngie perhaps?

In D50267#1147410, @des wrote:

I'd still like someone from tests to weigh in, @ngie perhaps?

Macro surewhynot:

This revision was automatically updated to reflect the committed changes.