Add conf.d support #88
Conversation
LaakkonenJussi
force-pushed
the
jb61966
branch
6 times, most recently
from
a472224 to
69ce9d2
Compare
LaakkonenJussi
force-pushed
the
jb61966
branch
from
69ce9d2 to
b49d2eb
Compare
| int fd; | ||
|
|
||
| fd = open(filename, O_WRONLY | O_NOFOLLOW | O_CLOEXEC); | ||
| if (fd == -1 && errno == ELOOP) |
There was a problem hiding this comment.
I guess this nofollow + eloop check works but... out of curiosity: why not lstat()? Or g_file_test(G_FILE_TEST_IS_SYMLINK) since it is used anyway for
G_FILE_TEST_IS_REGULAR?
Also, is there some reason to open for write if this is used for probing config files to read?
nit: if open fails for other than eloop reason, close(-1) is made. harmless but still
There was a problem hiding this comment.
Cannot remember because this is quite old code. Well I can look up this part to improve it as now this seems to work according to unit tests.
There was a problem hiding this comment.
Actually, this is where I apparently got the idea from in the first place long time ago: https://docs.gtk.org/glib/func.file_test.html
// DON'T DO THIS
if (!g_file_test (filename, G_FILE_TEST_IS_SYMLINK))
{
fd = g_open (filename, O_WRONLY);
// write to fd
}
// DO THIS INSTEAD
fd = g_open (filename, O_WRONLY | O_NOFOLLOW | O_CLOEXEC);
if (fd == -1)
{
// check error
if (errno == ELOOP)
// file is a symlink and can be ignored
else
// handle errors as before
}
else
{
// write to fd
}
There was a problem hiding this comment.
Not sure why changing this to g_file_test(filename, G_FILE_TEST_IS_SYMLINK) breaks iptables unit tests and because it is quite laborious and boring to work with that I'd rather not go back there...
| case -EACCES: | ||
| connman_warn("%s: %s, continue reloading firewall", | ||
| FIREWALLCONFIGDIR, | ||
| strerror(-err)); |
There was a problem hiding this comment.
after seeing N of these: Is there some profound reason why it would be preferable to have this kind of code duplication everywhere instead of doing such logging from within util_read_config_files_from()?
There was a problem hiding this comment.
Well yeah the warnings can be moved, however this is the reload case that is different to normal loading here. Maybe that upper warning needs a better description. Not sure what to change, the different cases here kind of justify this..
connman/src/shared/util.c
Outdated
| for (iter = files; iter; iter = iter->next) { | ||
| filepath = iter->data; | ||
|
|
||
| err = cb(filepath); |
There was a problem hiding this comment.
Just wondering, not knowing what happens in existing config reader functionality: if a value is set in multiple files, it gets now "executed" several times. Any chance that such thing could trigger adverse things?
There was a problem hiding this comment.
I think this is up to the processing function how to handle. There are no += indicators for basic ini-style file processing so the value defined in the last config read overrides anything defined before. Each processing function must handle the cases of overriding, and later appending on some, like on lists. It is hard to make foolproof out of this, not sure if dropping of the file from the list in case of certain error should be done...
LaakkonenJussi
force-pushed
the
jb61966
branch
from
ae3dcbd to
892894c
Compare
|
I have some bigger fish to fry with this still, the main.c config is just so difficult to work with this kind of change I rewrote all of it and need to check if it works, so, maybe this was too early out of WIP. |
LaakkonenJussi
force-pushed
the
jb61966
branch
2 times, most recently
from
ffa346c to
560c9a7
Compare
LaakkonenJussi
force-pushed
the
jb61966
branch
4 times, most recently
from
0427f07 to
5f197ce
Compare
[util] Add conf.d support for connmand and vpnd. JB#61966 Add support for reading and processing of additional configuration files. Util is best suited for both daemons to avoid adding more internal dependencies to vpnd. The files are saved into the list with the full path if it is given as a parameter. Callback can be omitted for only getting the list of configuration files with the desired suffix in the given directory. Callback gets the full path of the config file for convenience.
[main] Support main.conf.d/ files for overriding configs. JB#61966 Use the util conf.d support to read additional configs. The values in these additional configs will override any existing configuration set in the main.conf or any previous conf.d files.
[vpn-settings] Add callback for processing configs. JB#61966 Reformat init to allow processing of separate config files with a separate callback for the conf.d support.
[vpn] Use util conf.d to read additional configs. JB#61966
[firewall-iptables] Read additional firewall configs with conf.d support. JB#61966 Read the configurations when starting and reloading the firewall. When starting read the files into a list and process each with the callback. When reloading use the internal processing of files to first check which files are added and removed and then process the content accordingly.
The config change made the device specific rules to be loaded differently (correctly), so these needed to be adapted into the tests. Also the access failure on g_dir_open() needed to be reported back as a proper error from the test stub.
[main] Reformat processing of configuration values. JB#61966
[setting] Move connmand setting from main to setting. JB#61966 Move all setting from main.c to setting.c. Add internal and external getters for new content. Put the defines into setting.h for other components to access. Modify main.c to support new way of using settings, initialize settings early so the startup options can be set as well.
LaakkonenJussi
force-pushed
the
jb61966
branch
2 times, most recently
from
253f52a to
34dfb80
Compare
[util] Add GFileError to errno converter. JB#61966 Map the errors in G_FILE_ERROR domain to matching errnos. Return -EBADMSG if error is NULL and -ENOTSUP when domain is not G_FILE_ERROR.
LaakkonenJussi
force-pushed
the
jb61966
branch
from
34dfb80 to
2b304d5
Compare
No description provided.