The overall idea seems ok to me for what it's worth, my comments are about cosmetic issues.

My comments have been addressed and I think it makes sense to go ahead with the proposed patches.

In D40377#922101, @nyan_myuji.xyz wrote:

In D40377#922083, @crest_freebsd_rlwinm.de wrote:

The jail_name variable must be initialised to NULL. This should be done through an explicit char * jail_name = NULL; in line 101 of route.c.

Static storage are always initialized to 0/NULL by C standard.

In D40377#922083, @crest_freebsd_rlwinm.de wrote:

The jail_name variable must be initialised to NULL. This should be done through an explicit char * jail_name = NULL; in line 101 of route.c.

The jail_name variable must be initialised to NULL. This should be done through an explicit char * jail_name = NULL; in line 101 of route.c.

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

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.

IMHO this is superseded by https://reviews.freebsd.org/D40188

In D40188#919081, @crest_freebsd_rlwinm.de wrote:

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

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.

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

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.

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

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.

In D40188#915159, @otis wrote:

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

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

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.

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)

New and improved diff :-)

@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)

I suggest we start with a switch which enables the new solution. That means it does not break stuff for people who upgrade.

In D38826#894218, @guest-patmaddox wrote:

In D38826#887279, @dvl wrote:

All jails.conf jail (in /etc/jail.conf, /etc/jail.*.conf and /etc/jail.conf.d/*.conf) start automatically, without the need to define them in jail_list in rc.conf

That will require an UPDATING notice.

AND: perhap a switch[es] to preserve the old behavior and if set, enable the new behavior.

What is the motivation for changing the default behavior in the first place?

I have a bunch of “utility” jails that I don’t want starting up on boot. If it does change to start all jails, there either needs to be a way to maintain old behavior with an inclusion list, or at least provide an exclusion list to prevent some jails from starting.

I prefer the current behavior with inclusion lists. You have to enable most services - starting all jails by default conflicts with that general principle. Starting all jails - with no way to exclude them - would cause a big problem for me.

In D38826#887279, @dvl wrote:

All jails.conf jail (in /etc/jail.conf, /etc/jail.*.conf and /etc/jail.conf.d/*.conf) start automatically, without the need to define them in jail_list in rc.conf

That will require an UPDATING notice.

AND: perhap a switch[es] to preserve the old behavior and if set, enable the new behavior.

This change conflicts with https://reviews.freebsd.org/D39011
@antranigv_freebsd.am and @meka_tilda.center need to hash this out

All jails.conf jail (in /etc/jail.conf, /etc/jail.*.conf and /etc/jail.conf.d/*.conf) start automatically, without the need to define them in jail_list in rc.conf