Fix cleanup in lib/libc/gen/setdomainname_test

[asomers]

Summary

Fix cleanup in lib/libc/gen/setdomainname_test

ATF cleanup routines run in separate processes from the tests themselves, so
they can't share global variables.

Also, setdomainname_test needs to be is_exclusive because the test cases
access a global resource.

PR: 219967

[ngie]

I'm ok with what's being offered fix-wise. Just nitpicking bits to be honest.. The only part I'm curious about is the separation of ATF_REQUIRE calls.

contrib/netbsd-tests/lib/libc/gen/t_setdomainname.c

58 ↗

(On Diff #29581)

The return value of memset doesn't need to be checked.

75 ↗

(On Diff #29581)

n should be ssize_t.

76–78 ↗

(On Diff #29581)

Why separate this out?

83 ↗

(On Diff #29581)

Please add a #ifdef __NetBSD__ around this. I need to upstream this change to NetBSD.

[asomers]

contrib/netbsd-tests/lib/libc/gen/t_setdomainname.c

76–78 ↗

(On Diff #29581)

What do you mean?

83 ↗

(On Diff #29581)

Do you want me to restore all of the deleted lines with #ifdef __NETBSD__?

And BTW, I've opened an upstream bug report at https://gnats.netbsd.org/cgi-bin/query-pr-single.pl?number=52297

[ngie]

contrib/netbsd-tests/lib/libc/gen/t_setdomainname.c

76–78 ↗

(On Diff #29581)

Oh.... I see. If it's a zero-length file, don't wipe out the domain.

Why not ensure that the size of the file read is matches the read amount, and set the domain name to that read value?

83 ↗

(On Diff #29581)

Ok. If you're handling it upstream, then it doesn't need the annotation. I usually ask for the annotation when pulling things back in just so I can keep track of what all is going on/what needs to be upstreamed.

[asomers]

• Don't call ATF_REQUIRE in a cleanup routine. On failure, it asserts.
• Deal with short reads and writes in backup_domain and restore_domain
• Stripe trailing nulls in backup_domain
• Always restore the domain in restore_domain, even if it's zero-length. That logic only worked by accident before, since backup_domain didn't strip.
• Remove unused paramters.

[ngie]

contrib/netbsd-tests/lib/libc/gen/t_setdomainname.c

80–87 ↗

(On Diff #29689)

I think warn + exit(0) makes more sense here (apart from scenarios where you absolutely, must trickle up the error) because cleanup should exit successfully in normal circumstances (which includes incomplete cleanups). Otherwise kyua says something like "cleanup failed. this isn't supposed to happen! -> coredump ala abort())".

[asomers]

contrib/netbsd-tests/lib/libc/gen/t_setdomainname.c

80–87 ↗

(On Diff #29689)

My reasoning was that if any of these steps failed, then there must be an error in the test. Under normal circumstances, none of this stuff can fail. If it exits 0, then the user will never know about the problem, and a developer will never fix it. These are basically assertions, but apparently I can't use ATF_REQUIRE in a cleanup routine. Would you like me to replace exit(3) with assert(3) ?

[ngie]

contrib/netbsd-tests/lib/libc/gen/t_setdomainname.c

80–87 ↗

(On Diff #29689)

If the test exits early, it'll get flagged as an erroneous error. This is what I'm hoping to avoid.

I don't care for the kyua/atf integration in this regard, but it's kind of the way it is today.

[asomers]

contrib/netbsd-tests/lib/libc/gen/t_setdomainname.c

80–87 ↗

(On Diff #29689)

Can you explain why you think it would be a false error? It seems like it would be a genuine error to me.

[ngie]

contrib/netbsd-tests/lib/libc/gen/t_setdomainname.c

66–67 ↗

(On Diff #29689)

I think a one-shot write attempt would be ok.