Page MenuHomeFreeBSD

ctl.conf(5): add support for an 'include' directive.
AbandonedPublic

Authored by rew on Oct 6 2020, 9:18 PM.
Tags
None
Referenced Files
Unknown Object (File)
Dec 20 2023, 1:39 AM
Unknown Object (File)
Dec 13 2023, 12:47 PM
Unknown Object (File)
Dec 10 2023, 12:49 PM
Unknown Object (File)
Nov 19 2023, 4:13 AM
Unknown Object (File)
Nov 19 2023, 1:51 AM
Unknown Object (File)
Nov 19 2023, 1:39 AM
Unknown Object (File)
Nov 11 2023, 12:33 PM
Unknown Object (File)
Nov 9 2023, 3:41 PM
Subscribers

Details

Reviewers
asomers
kevans
allanjude
trasz
Group Reviewers
manpages
Summary

Add support to allow an 'include' directive in ctl.conf(5).

This directive expects a directory and will source all configuration files
within that directory ending in '.conf' - similar to syslog.conf(5).

All configuration files sourced in the given directory are processed after
the first level configuration file.

All included configuration files still share the same namespace.

Works with the general syntax and UCL format.

Example ctl.conf(5):

include /path/to/confs

This review is for PR, https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=249239

Test Plan

I've tested on local machine with basic configs spread among multiple directories.

If there's specific edge-cases to test for, would like to hear it.

Diff Detail

Repository
rS FreeBSD src repository - subversion
Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 34030
Build 31217: arc lint + arc unit

Event Timeline

rew requested review of this revision.Oct 6 2020, 9:18 PM
asomers requested changes to this revision.Oct 6 2020, 9:40 PM

The change summary says that it's ucl compatible. However, it's not compatible with libucl's 'include directive. That one expects either a file or a glob pattern, not a directory. See https://github.com/vstakhov/libucl . I think libucl's version is better.

usr.sbin/ctld/ctl.conf.5
89

Don't forget to bump the man page's .Dd date.

usr.sbin/ctld/uclparse.c
298

The } and else { should be on the same line

1000

Why did you change this to a printf?

1004

Is that trailing whitespace I see?

This revision now requires changes to proceed.Oct 6 2020, 9:40 PM

Updating D26700: ctl.conf(5): add support for an 'include' directive.

fixed whitespace

style(9) fix

printf back to log_warn, my mistake.

add a missed free call, free the parser if ucl_parser_add_file fails.

remove two header files from parse.y, leftover from testing.

The include directive works with the general syntax version or the UCL format version for ctl.conf(5). It doesn't interfere
nor is it compatible with the .include macro offered by libucl - the .include macro is handy though and has more functionality.

I can see how the include keyword could be confusing when a configuration file is in UCL format...especially if the .include macro
is known - both have the same name and do different, but similar things..

In D26700#595013, @rew wrote:

The include directive works with the general syntax version or the UCL format version for ctl.conf(5). It doesn't interfere
nor is it compatible with the .include macro offered by libucl - the .include macro is handy though and has more functionality.

I can see how the include keyword could be confusing when a configuration file is in UCL format...especially if the .include macro
is known - both have the same name and do different, but similar things..

Oh, .include already worked? I had no idea. Given that, do we even need this feature at all?

In D26700#595013, @rew wrote:

The include directive works with the general syntax version or the UCL format version for ctl.conf(5). It doesn't interfere
nor is it compatible with the .include macro offered by libucl - the .include macro is handy though and has more functionality.

I can see how the include keyword could be confusing when a configuration file is in UCL format...especially if the .include macro
is known - both have the same name and do different, but similar things..

Oh, .include already worked? I had no idea. Given that, do we even need this feature at all?

It only works if ctld is started with -u, which switches from this config syntax to ucl. This seems like a kind of bizarre design, tbh. I would remove the ucl counterpart so that we don't encourage non-standard convention in ucl; the user should really learn about the .include macro.

That said, I would be tempted to match the precedent set by newsyslog of making it a glob by default. This would be a closer match to the flexibility provided by libucl (which can actually switch between glob and non-glob), and allows the user to use non-standard filename patterns if they want to. Rather, the user can explicitly see at the time of writing how we're expecting the files to be named.

This wouldn't be significantly harder, it's still substantially the same approach except you have to split the pattern up (probably basename(3) and null-terminate the character just before if it's not the start of the string) and fnmatch(3) against the pattern.

@asomers, good question...if your configuration is in UCL format, then probably not.

I hesitate to add a directive that isn't consistent between the standard format and UCL format configurations. I may be
more inclined to add a man page hint for the .include macro provided by ucl - which will encourage the use of the
UCL format style.

In D26700#595719, @rew wrote:

@asomers, good question...if your configuration is in UCL format, then probably not.

I hesitate to add a directive that isn't consistent between the standard format and UCL format configurations. I may be
more inclined to add a man page hint for the .include macro provided by ucl - which will encourage the use of the
UCL format style.

+1. Encouraging UCL sounds like the best option to me. I wouldn't have opened the bug if I'd know about that option.

gbe added a subscriber: gbe.

OK from manpages.