Use fts functions for scanning directories

[rafal-krypa]

The BSD functions fts_open() and fts_read() are a nice alternative to ftw/ntfw and manual directory reading. This commit replaces existing readdir() and nftw() usage to get:

• applying CIPSO rules in one shot, just like regular rules
  • [jsakkine: Regular rules are not yet applied in one shot. #53 has not yet been merged. I would merge that first and even that after profiling. You can propose something similar for CIPSO but is there large CIPSO data sets even used?]
• sort input files alphabetically to load them in a defined order (readdir() order is file system dependant)
  • [jsakkine: This not a benefit.]
• loading rules works on file systems not setting d_type on readdir() (fix 233dc30 regression)
  • [jsakkine: I would first consider fixing this regression.]
• better performance compared to nftw() due to fewer calls to stat() internally
  • [jsakkine: As far as I know nftw() is not used anymore to load access rules, only CIPSO uses it. I'm willing to take a patch to fix that.]
• shorter code due to unified handling of load from file and from directory
  • [jsakkine: I'm not a fan of callbacks or unnecessary function pointers. At least I would propose that part in a separate patch.]

[rafal-krypa]

This patch also fixes issue #39, point 3.

[jarkkojs]

• This contains multiple logical changes:
  • Migrate rule loading to fts_*.
  • Migrate CIPSO loading to fts_*.
  • Merge these into single multi-purpose function.
  • fputs -> fprintf
  • Reading -> Parsing
  • One shot loading of access rules.
  • One shot loading of CIPSO.

I think I could consider taking use of fts_* functions after the release but I would not take them in this form. I want to first merge #53 (also that after the release).

[jarkkojs]

Don't take me wrong, I'm not shooting this down but I'm not ready to take this amount of stuff either in one shot.

[jarkkojs]

I thought about this and my view is that instead CIPSO should use opendir() and readdir(). They are widely used and mature way to read directories. Also, most of the relevant file systems understand DT_TYPE. Code based on them is easy to read and debug.

If you think that this is good for performance I would all the other performance optimizations first and then see if the used workloads are too slow and if fts_* makes any significant difference. I agree that using nftw() was horrible idea.

My most probable comment for this is NAK.

One more note. I have my doubts about superiority of this interface to the simpler opendir/readdir/closedir interface. Both are wrappers to the same system call interface provided by GNU libc. It's just factors more complex wrapper interface and produces factors more complex to trace code for simple use case like we have (reading files from a single directory). It does much more than is needed. It makes sense to use here thinnest available GNU libc interface to the system calls.

[jarkkojs]

Better way to fix #39 would to just use scandir together with alphasort IMHO. It would be very small change to the current loop. Then you get your guaranteed order.

[rafal-krypa]

Don't get me wrong, I don't claim that there is something wrong with readdir() or scandir(). They are surely used internally in glibc for nftw and fts implementation. scandir() looks just fine for the purpose of reading files in guaranteed order. But their usage in libsmack requires more logic than present atm. For d_type = DT_UNKNOWN problem, there are at least two important file systems not supporting it: XFS and ReiserFS. For now smackctl is silently broken on those and IMHO it's a release critical bug. It should probably call stat() in such cases and get the file type there. Also symbolic links are silently ignored - was that intentional? If you prefer to have additional code, reimplementing logic parts of already existing library functions, let's go that way. But what do you dislike about fts?

If you see a future of this patch, let me know how do I proceed. In that case I should probably retarget it for different branch, maybe split to smaller (but interleaving) changes?

[jarkkojs]

The choice of abstraction level is always a tradeoff. I chose the simplest library functions available with least features because use case is fairly simple. Using something like fts_* is over-engineering from my point of view. I'm a more fan of thin and simple functions than frameworks. Productivity in code writing is overrated. Easiness of reading the code, tracing and debugging are more important.

Symbolic links are ignored on purpose. If you need such support create issue on that. I haven't seen such workload yet.

If you have a use case for XFS or ReiserFS that doesn't work, report a bug on that. I highly doubt it. I also don't really see why you couldn't solve possible problems without using fts_*. Have you tested fts_* with all file systems and all major libc implementations (Bionic, uClibc, GNU libc)?

I think right bug fix for these issues for the release would not to ignore silently but return -1. You're right that silent ignorance is a critical issue but I don't think ignoring XFS and ReiserFS is.

[jarkkojs]

I think there are multiple issues that you reported. I would be, for example, happy to take in a patch that would traverse files in alphanumeric order. I can already integrate patch in #53 to next release. I think also returning -1 if dt_type is not DT_REG makes also sense.

[jarkkojs]

You can use function pointer when if you want to use the same function for traversing CIPSO and access rules like you do in that patch (add_func). It works. There's enough common code to it make sense (assuming that you want to make improvements to CIPSO code).

[rafal-krypa]

Closing, I will proceed with smaller changes which we agreed on.