lib/libc/tests: add fts symlink traversal tests

[iyan005_outlook.com]

Summary

Add regression tests for fts_open, with FTS_LOGICAL, verifying correct
handling of symbolic links during directory traversal

Test Plan

Test with kyua the new generated test in /usr/obj/usr/src/[your arch]/lib/libc/tests/gen/,
specificallym the fts_follow_symlink test

[asomers]

This is a good start. In addition to the above comments, though, there needs to be a paired test for FTS_PHYSICAL. It isn't worth submitting a test for one without the other.

lib/libc/tests/gen/Makefile

35 ↗

(On Diff #174081)

These should be ordered alphabetically, so this test program should up up with the other fts test program.

lib/libc/tests/gen/fts_symlink_test.c

10

Why this commented-out line?

30

You should make found_link a bool.

33

34

Better to split this up. Otherwise the error message will be very confusing if open fails.

35

50–53

This cleanup isn't actually necessary. Kyua will take care of that. And even if it didn't, this isn't the right place for cleanup. If you need cleanup, it should go in a separate cleanup function so that it will be executed even if the test fails.

[iyan005_outlook.com]

Ok, I just went over your suggestions, implemented them, and also followed proper fts testing "etiquette" by using the designated structures for the job.

Thank you Mr Somers for the review.

P.S. This is so fun, I'm loving looking at the code and learning about the proper way to do stuff, I was giggling a bit while submitting this patch not gonna lie.

[asomers]

This looks a lot better. In addition to the inline comments, I have two general ones:

• Are these tests redundant with those in fts_options_test.c ? Specifically fts_options_logical and fts_options_physical ? [e-mail] wrote those, and the other existing fts tests, after I planned this GSoC project (but before you or anybody else had started to work on it). If you make use of des's work, then that would be less work for you.

• fts needs dozens of test cases in total. Only a few involve symlinks. IMHO one test program just for a handful of symlink cases is awkward. It will lead to too many files on disk. I would prefer a smaller number of test programs with more test cases each. Apparently des@ disagrees with me. But then I'm the mentor here ;) .

lib/libc/tests/gen/fts_symlink_test.c

4

There's supposed to be a license name after SPDX-License-Identifier, for example, BSD-2-Clause

7–16

C header files have a particular order:

#include <sys/types.h>           // If types.h or param.h, it comes first
#include <sys/other_stuff.h>     // Other things in sys/ come next
// Then a space
#include <net/if.h>              // Anything in net/ , if necessary
// Then a space
#include <stdio.h>               // The C standard library comes next
// Then a space
#include "local_stuff.h"         // then local includes

See style(9) for details

81

Too many blank lines here

[iyan005_outlook.com]

addressed inline comments.

• After looking now at fts_options_test.c, it does indeed cover symlink testing in much more extent than my tests. I'll definitely take those into consideration for the roadmap of the test suite.

• From what I undestand, your approach doesn't cost anything additional over des@'s approach right ? I mean less programs and more tests just sounds like a better deal, but since I'm uninformed on this, I'm not sure.

Thank you for the review !

P.S. Just replied to your email !

[iyan005_outlook.com]

Hello, I'm still figuring out the proper way to do things in phabricator, so i was just wondering :

• Are my inline comments visible ? since at the bottom it page it lists them with a "Unsubmitted" label, as in this patch needs revision ? or as in other people can't see them ?

Other than, I've seen this draft https://reviews.freebsd.org/D19407 stuck mainly on fts(3) not being capsicumized right ? Maybe I could try to finish after this project ? But this is more of a note to remind myself later on, than to actually discuss it, I still need to get accepted here first hahaha.

lib/libc/tests/gen/fts_symlink_test.c

4

got it !

7–16

style(9) needs a good read, thank you for mentioning it. Those are now fixed.

Quick question : Inside of style(9) :

"[...] The remaining Kernel headers should be sorted alphabetically.

#include <sys/types.h> /* [...] */
#include <sys/systm.h>
#include <sys/endian.h>
#include <sys/lock.h>
#include <sys/queue.h>
"

Are the header inclusions listed above supposed to demonstrate the statement about alphabetical ordering ? because I can't see how they are. I surely misunderstood something.

10

because I thought that I'd submit a patch first without using the fts_test() and struct fts_testcase, and then i forgot to remove it.

As I fix all of the issues you addressed, I'll also go with using the fts_test function and struct fts_testcase.

50–53

Alright, that's noted !

81

good catch, fixed them all.