Add ".include" directive to jail.conf

[jamie]

Summary

Allow jail config files to include other files, via the ".include" directive (the leading dot is to ensure it doesn't clash with jail or parameter names). The include filename may be a glob, which allowed for config dirfectories such as /etc/jail.conf.d.

[ihor_antonovs.family]

@jamie please re-generate the diff with -U9999 and re-upload it. This is necessary to have context available. (Annoying Phab limitation when diffs are uploaded manually)

Example:

git diff -U99999 | wl-copy

[jamie]

New and improved diff :-)

[ihor_antonovs.family]

I like this approach.
It blurs the line between UCL and jail format (IMHO making future transition to UCL smoother), and makes include more explicit (and less magical comparing to D40188)

The I have not read the code thoughtfully yet, will do so over the next day or two.

[meka_tilda.center]

I you use git format-patch -1 -U9999 and apply it with git am <patch>, you get the whole commit with the message. Not strictly needed, but makes life easier.

usr.sbin/jail/config.c

274

Initialize all glob_t vars to zero/NULL.

[otis]

Haven't looked closely yet, but: are circular includes handled correctly?

[dvl]

In D40188#915159, @otis wrote:

Haven't looked closely yet, but: are circular includes handled correctly?

This is what I came here to ask.

[ihor_antonovs.family]

In D40188#915660, @dvl wrote:

In D40188#915159, @otis wrote:

Haven't looked closely yet, but: are circular includes handled correctly?

This is what I came here to ask.

Theoretically this is easy to handle. All it takes is a map/list of absolute file paths that the parser has already seen. If a new path encountered - include, if a known one encountered - skip.

[ihor_antonovs.family]

It doesn't look like the patch in it's current state handles this circular includes.

usr.sbin/jail/config.c

295

Checking if the file was already included would need to happen somewhere here.

[jamie]

True, they're not handled. I took my include inspiration from newsyslog (which has includes that also support globbing), and there it's also just a simple matter or running whatever it's told to include. It's kind of a footgun situation, where it's generally good enough to trust the administrator not to make such a loop. I did it for depend loops, but only because that's kind of elemental in building an acyclic directed graph.

I imagine it wouldn't be incredibly hard to handle, though it's a bit more than just tracking filenames, since I do at least a cursory job of resolving relative names, and because the same file may be included in different contexts (within different jails). But hashing a jail/dev/inode tuple might do the trick.

[jamie]

Simple include-loop prevention with via a maximum depth counter.

[crest_freebsd_rlwinm.de]

Just a small nitpick: I would prefer a macro #define MAX_INCLUDE_DEPTH 32 or constant static const unsigned int max_include_depth = 32; somewhere above the include_config() in config.c instead of the literal to improve readability.

[jamie]

In D40188#919081, @crest_freebsd_rlwinm.de wrote:

Just a small nitpick: I would prefer a macro #define MAX_INCLUDE_DEPTH 32

True, let's not let bad habits creep in.

[jamie]

I've committed the "jails can include jails" and "use the recursive parser" bits separately. This new diff is just the part that handles the includes.

[jamie]

Commited in eb5bfdd06565. I forgot to add the review to the commit message :-/

[antranigv_freebsd.am]

In D40188#920799, @jamie wrote:

Commited in eb5bfdd06565. I forgot to add the review to the commit message :-/

In that case, I guess we have to mark it as "Accept Revision"?

[antranigv_freebsd.am]

Oh and we also have to close it, because it did land! ugh Phabricator is very Project-Management-y :)

[emaste]

In D40188#920799, @jamie wrote:

Commited in eb5bfdd06565. I forgot to add the review to the commit message :-/

eb5bfdd06565 is tcp: Add and update cubic module variable names

This should be e82a62943529d1a7c1fcec39aec13eba69c671d6 I think?