Add support for config drop-ins (rudimentary includes as a bonus) #933

WhitePeter · 2021-10-16T00:54:23Z

This PR introduces support for drop-ins--those files (or snippets) usually residing in a *.d directory to override (some or all settings of) the main/base config in the parent directory.

There is also some very rudimentary support for including additional files, inspired by #796. But it is more of an "anti-sale" I am trying here, since I believe that drop-ins are a sufficient alternative. They can be implemented in a cleaner way and do not have problems that might arise with including files, i.e. include loops as mention here. Things like that are simply impossible with drop-ins. So, with the right care one should be able to achieve the same (or similar) results by leveraging drop-ins. See FILES section in 'dunst(1)' for details on the drop-in support. There is, intentionally, no further documentation on including files, because I believe it is more trouble to get right than it is worth, given the rather robust alternative, which is a straight forward evolution of #917.

Technically, one would need to really preprocess all files and keep track of already included files, if you want to get include support right. At first glance, hooking this into 'load_ini_file()' does look nice, until you think about all the potential problems. From my perspective the right place to do this would probably be in 'load_settings()' where the file handling takes place now, because not only do you need to keep track of all files opened by include rules but the corresponding file containing the first include as well. That information is not available in 'load_ini_file()' unless you want to push file handling there. But if you do that there is one big gain from NOT supporting includes and solely relying on drop-ins: the key-value file parser provided by glib, which I stumbled upon while browsing the documentation, could be used to do the dirty work in 'load_ini_file()'. While there is no way to make that work with includes, it seems trivial to replace the cumbersome string parsing and drop includes altogether.

~~Oh, one last thing, I forgot to mention in the commit messages: 'load_settings()' now has a safeguard to not set defaults, if called again, with #719 in mind.~~ Edit: This wasn't actually true, since set_defaults() ran unconditionally, had overlooked that I moved it because of a failing test, and on second thought it does not make sense to not set defaults on subsequent calls of load_settings()

P.S.: Also dialed back a little on the prefixes for debug messages etc. and made them work with clang as well. ~~I just realized that the commit message suggests otherwise, but only debug, error, and critical messages get this additional prefix.~~

codecov-commenter · 2021-10-16T00:55:32Z

Codecov Report

Merging #933 (c5fbdbc) into master (3b36f79) will decrease coverage by 0.15%.
The diff coverage is 47.05%.

@@            Coverage Diff             @@
##           master     #933      +/-   ##
==========================================
- Coverage   60.33%   60.17%   -0.16%     
==========================================
  Files          42       42              
  Lines        6416     6456      +40     
==========================================
+ Hits         3871     3885      +14     
- Misses       2545     2571      +26

Flag	Coverage Δ
unittests	`60.17% <47.05%> (-0.16%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
src/icon.c	`68.82% <ø> (-0.37%)`	⬇️
src/option_parser.c	`79.83% <ø> (ø)`
src/settings.c	`50.00% <36.76%> (-13.42%)`	⬇️
src/utils.c	`90.09% <87.50%> (-0.22%)`	⬇️
src/ini.c	`72.00% <100.00%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3b36f79...c5fbdbc. Read the comment docs.

WhitePeter · 2021-10-16T15:23:29Z

Commit messages and code should now be in sync. Especially ini.c was missing the fix for section names surrounded by spaces, which fell by the wayside when resolving a conflict while rebasing against upstream.

WhitePeter · 2021-10-16T15:43:45Z

Added a minor fix for the doxygen comment of get_config_files().

WhitePeter · 2021-10-17T21:23:46Z

load_settings() and get_config_files() received some final tweaks. This should be it, though; not touching the code anymore.

fwsmit

I didn't go through everything of the last commit yet, but here are my initial thoughts.
I think that a drop-in directory would be an OK alternative to @include. I'm not sure if it's much easier to implement, though. If we implement @include support, you don't need to do anything to implement a drop-in dir, since you can just write
@INCLUDE ./conf.d/*.conf.

About the drop-in directory location: I believe that it should be basedir/dunst/conf.d/. Most programs I see on my system are putting the conf.d directory in the same directory as their main config (in our case the config is in the directory dunst, so conf.d should be there as well).

In the current review comments I looked a bit too much at the implementation details, but I'll think some more about the big picture and get back to you about that. So don't look too much into the the small comments yet.

src/settings.c

fwsmit · 2021-10-18T21:09:17Z

src/settings.c

+#define G_DROPIN_DIR(basedir) G_STRCONCAT(G_BASE_RC(basedir), ".d")
+
+/** @brief Match pattern for drop-in file names */
+#define DROPIN_PAT "*.conf"


spelling: DROPIN_PAT -> DROPIN_PATH. I would maybe call it DROP_IN_PATH, though, since dropin isn't a word

spelling: DROPIN_PAT -> DROPIN_PATH.

Nope, it is a pattern, not a path. ;) I thought it was obvious and wanted to save some line space and keystrokes.

I would maybe call it DROP_IN_PATH, though, since dropin isn't a word

Fair point, I had similar thoughts while I was at it. I will rename it. Should I use ..._PATTERN?

This macro isn't neccesary anyways. It's only used in 1 place. Please move the string to is_drop_in.

src/settings.c

WhitePeter · 2021-10-19T14:25:14Z

I think that a drop-in directory would be an OK alternative to @include. I'm not sure if it's much easier to implement, though.

Yes, that is technically true, in a way, but the drop-in code hooks nicely into the already present implementation of #917, which is kind of the point. And, as I said, you cannot get headaches about loops because we rely on filesystem features which protect against those--thinking about symlink loops here.

If we implement @include support, you don't need to do anything to implement a drop-in dir, since you can just write @INCLUDE ./conf.d/*.conf.

But it is kind of a side effect of #917 to get an easy in for drop-ins. Includes are somewhat orthogonal to that, because you need to open and preprocess/parse the files to make them work. Hence my remark on robustness: drop-ins are basically just some filesystem 'magic'.

About the drop-in directory location: I believe that it should be basedir/dunst/conf.d/. Most programs I see on my system are putting the conf.d directory in the same directory as their main config (in our case the config is in the directory dunst, so conf.d should be there as well).

My experience is the opposite. Most of the time I see something like <base>.d/ beside <base>, see this excerpt from my system's /etc. /etc/apt/{apt.conf.d,sources.list.d} are prime examples and some of the earliest I can think of--Debian played a big part in the conception of the drop-in concept, if not to say it was invented there. IMO the naming (and the derivation in the code) is straight forward and very easy to grasp: <base>.d(rop-ins)/. Especially, since <base> need not exist at all, you always know, just by looking at the name of the drop-ins directory, what it is supposed to be.

In the current review comments I looked a bit too much at the implementation details, but I'll think some more about the big picture and get back to you about that. So don't look too much into the the small comments yet.

👍

fwsmit · 2021-10-21T15:07:12Z

Yes, that is technically true, in a way, but the drop-in code hooks nicely into the already present implementation of #917, which is kind of the point. And, as I said, you cannot get headaches about loops because we rely on filesystem features which protect against those--thinking about symlink loops here.

If you think it's easier to implement drop-in dirs that's fine. We can always add @INCLUDE later in a backward-compatible way.

My experience is the opposite. Most of the time I see something like <base>.d/ beside <base>, see this excerpt from my system's /etc. /etc/apt/{apt.conf.d,sources.list.d} are prime examples and some of the earliest I can think of--Debian played a big part in the conception of the drop-in concept, if not to say it was invented there. IMO the naming (and the derivation in the code) is straight forward and very easy to grasp: <base>.d(rop-ins)/. Especially, since <base> need not exist at all, you always know, just by looking at the name of the drop-ins directory, what it is supposed to be.

I'm sorry, I was under the impression that the location in your code was for example /etc/dunstrc.d, but /etc/dunst/dunstrc.d seems good to me.

fwsmit

Here's the rest of my review. In short, please don't use macros when not neccesary. You may also drop the @INCLUDE code. Once you've addressed my comments, I think this PR is in a pretty good state.

docs/dunst.1.pod

src/ini.c

src/settings.c

fwsmit · 2021-10-24T15:26:18Z

src/settings.c

+#define G_DROPIN_DIR(basedir) G_STRCONCAT(G_BASE_RC(basedir), ".d")
+
+/** @brief Match pattern for drop-in file names */
+#define DROPIN_PAT "*.conf"


This macro isn't neccesary anyways. It's only used in 1 place. Please move the string to is_drop_in.

fwsmit · 2021-10-24T15:27:05Z

src/settings.c

-        FILE *config_file;
+static int is_dropin(const struct dirent *dent)
+{
+        return 0 == fnmatch(DROPIN_PAT, dent->d_name, FNM_PATHNAME | FNM_PERIOD)


Suggested change

return 0 == fnmatch(DROPIN_PAT, dent->d_name, FNM_PATHNAME | FNM_PERIOD)

return 0 == fnmatch("*.conf", dent->d_name, FNM_PATHNAME | FNM_PERIOD)

This macro isn't neccesary anyways. It's only used in 1 place.

Technically, that is not true because it is also used in the doxygen comment. ;) I was just about to make the change you requested, when I realized that there are two places that need changing. I'd really rather use symbolic names wherever possible. Using a hardcoded string constant "buried" in a function just doesn't feel right.

Edit
Let me quote from the old K&R to drive this home ;)

C makes the use of functions easy, convenient and efficient; you will often see a short function defined
and called only once, just because it clarifies some piece of code.

The same goes for this symbolic name (not a macro, BTW), which only serves to hide the gritty details which are irrelevant in is_drop_in(). So the fact that it is only used once, does not make it useless.

If it helps I renamed it to DROP_IN_PATTERN in my local repo, already. :)

Edit2

It's bad practice to bury "magic numbers" like 300 and 20 in a program; they convey little information to someone who might have to read the program later, and they are hard to change In a systematic way. One way to deal with magic numbers is to give them meaningful names. A #define line defines a symbolic name or symbolic constant to be a particular string of characters:

#define name replacement text

In this case, the macro makes the function harder to read, since you have to scroll up to the definition of the macro to see what the actual search path is.

It's bad practice to bury "magic numbers" like 300 and 20 in a program; they convey little information to someone who might have to read the program later, and they are hard to change In a systematic way. One way to deal with magic numbers is to give them meaningful names. A #define line defines a symbolic name or symbolic constant to be a particular string of characters:

#define name replacement text

To use your example. In the case of numbers it's much less clear what's happening, since a number like 20 doesn't say much. The string "*.conf" that's passed to the function fnmatch makes it completely clear what's happening: you're trying to see if the filename ends with .conf. When you replace it with DROP_IN_PATTERN, the reader might think 2 things:

the function matches "*.conf"

the function matches "dunstrc.d/*.conf"

In the same line there is an example of good usage of a macro (gcc calls them macros), namely the flags given to fnmatch: FNM_PATHNAME | FNM_PERIOD. You could also use some number instead, but that wouldn't say anything and you would have to look at some other place to see what those numbers mean.

In this case, the macro makes the function harder to read, since you have to scroll up to the definition of the macro to see what the actual search path is.

Again, not a path. ;)

And what you describe is kind of the point, actually. You should not need to look because it is irrelevant. All you need to know is that it is looking for the drop-in pattern (DROP_IN_PATTERN). The real value of that pattern is unimportant. You do not go searching for the the real values of symbolic constants like TRUE, FALSE, or EINVAL, or whatever, either, do you?

It's bad practice to bury "magic numbers" like 300 and 20 in a program; they convey little information to someone who might have to read the program later, and they are hard to change In a systematic way. One way to deal with magic numbers is to give them meaningful names. A #define line defines a symbolic name or symbolic constant to be a particular string of characters:

#define name replacement text

To use your example. In the case of numbers it's much less clear what's happening, since a number like 20 doesn't say much.

The example only talks about numbers because it is a verbatim copy from Kernighan's & Ritchie's "The C Programming Language".

The string "*.conf" that's passed to the function fnmatch makes it completely clear what's happening: you're trying to see if the filename ends with .conf. When you replace it with DROP_IN_PATTERN, the reader might think 2 things:

* the function matches "*.conf" * the function matches "dunstrc.d/*.conf"

Why? This is a filter for scandir which cannot return relative paths but only the entries of a directory listing--it cannot look into sub directories. Again, that is nothing that the reader should be concerned about. Think of it in terms of encapsulation: you do not need to know. The function name and its documentation says it all, hopefully--if not, the documentation should be fixed, not the macro. Needing to know about the inner workings of a function is a symptom of bad encapsulation and smells. ;)

And if you had to go looking it up, try the documentation produced by doxygen, which neatly links to DROP_IN_PATTERN, which it couldn't if it wasn't defined this way. I know that you do not use it, but maybe you should consider doing so. At least take a look at it to see what I mean, please.

In the same line there is an example of good usage of a macro (gcc calls them macros)

OK, 'tomato tomahto'. ;)

namely the flags given to fnmatch: FNM_PATHNAME | FNM_PERIOD. You could also use some number instead, but that wouldn't say anything and you would have to look at some other place to see what those numbers mean.

Yes, but why is my macro any less useful?

fwsmit · 2021-10-24T15:38:42Z

src/settings.c

+ * explicitly setting XDG_CONFIG_DIRS to their liking at runtime or by
+ * setting SYSCONFDIR=/etc/xdg at compile time.
+ */
+#define XDG_CONFIG_DIRS_DEFAULT SYSCONFDIR


This variable doesn't need to be defined file-wide. Please move it to the place where it's used.

A similar issue here. If I move this to get_xdg_basedirs(), the doxygen comment then becomes part of that function's comment. While I was writing that code it just seemed out of place and not necessary to have there, which is why I made it a symbolic constant.

Also, this being a preprocessor directive makes it file-wide anyways at least from the line of its definition onwards.

P.S.: vim also wants to indent the comment, since it is inside a function. :/

src/settings.c

config.mk

WhitePeter · 2021-10-25T20:21:02Z

About #947:

But, when implementing #947, I had to also had to work with xdg basedir variables and I've slightly improved upon your code. Could you consider copying that code to utils.c and use that instead?

Did you mean this?

Did you mean to copy get_xdg_basedirs() verbatim or to generalize it, so it can be used for other kinds of XDG base directories? I am asking because I haven't found the time to study that code in-depth yet. (read your comment wrong)

BTW, there is g_get_system_data_dirs() in Glib, much like g_get_system_config_dirs(). It is only for the default of the latter, that I wrote get_xdg_basedirs(). According to the spec XDG_DATA_DIRS defaults to a sensible /usr/local/share:/usr/share, so maybe there is no need for custom code.

P.S.: One thing just sprang to mind, which I thought of while writing fexists() (not the best name, given that it is more picky). That could also be made available in utils.c and maybe replace is_readable_file(). Or meld the two together to be more general. As I pointed out before access() also returns success when pointed to directories since those are just "special" files in UNIX terms. I think a stat() call is unavoidable, if you really want to check if the path at hand points to a regular file. Also, access() uses the real UID to check--I don't know if privilege dropping is in the pipeline, which could render that test misleading. And only the result of (f)open() is relevant, after all, which is why I did not even attempt further checks in fexists(). See for instance this comment about TOCTOU in the stackoverflow answer you linked to above.

fwsmit · 2021-10-27T12:34:14Z

src/settings.c

- * Tries to open all existing config files in *descending* order of importance.
- * If cmdline_config_path is not NULL return early after trying to open the
- * referenced file.
+static const char * const *get_xdg_basedirs(void);


This definition isn't needed as far as I can tell

fwsmit · 2021-10-27T12:35:15Z

src/settings.c

+
+static int is_dropin(const struct dirent *dent);
+
+static bool fexists(char * const path);


These definitions aren't needed either

Those are just the prototypes, and I took another file from Dunst's codebase as an example--can't remember which, though. Having these in there makes for more flexibility below, since one can write the actual function definitions in arbitrary order, as opposed to "define before call".

I'd argue that, since load_settings() is the only real public function and somewhat the main() of this file, it should be at the top, after the prototypes that is. But it is at the bottom now, because someone didn't bother to copy its signature to define the prototype. ;)

fwsmit · 2021-10-27T12:54:03Z

Did you mean this?

yes

BTW, there is g_get_system_data_dirs() in Glib, much like g_get_system_config_dirs(). It is only for the default of the latter, that I wrote get_xdg_basedirs().
Edit: note that I accidentally hardcoded XDG_DATA_DIRS in that function. I'll update that soon to be more generic.

So what I'm asking is, is to replace most of the get_xdg_basedirs with add_paths_from_env. This is to prevent code duplication.

According to the spec XDG_DATA_DIRS defaults to a sensible /usr/local/share:/usr/share, so maybe there is no need for custom code.

The environment variable itself doesn't default to some value. When it's empty, that value should indeed be used. To make it more confusing, when the spec says a file is located in XDG_DATA_DIRS/somedir you should look at XDG_DATA_HOME/somedir before looking at XDG_DATA_DIRS/somedir.

P.S.: One thing just sprang to mind, which I thought of while writing fexists() (not the best name, given that it is more picky). That could also be made available in utils.c and maybe replace is_readable_file(). Or meld the two together to be more general.

Yep that's definitely a good option. The only place where that's used (in master) is in icon.c to check if the icon file is readable. I don't expect many icon themes to include FIFO's, but I don't think it would hurt to allow them.

As I pointed out before access() also returns success when pointed to directories since those are just "special" files in UNIX terms. I think a stat() call is unavoidable, if you really want to check if the path at hand points to a regular file.

Yeah that's a good point. It isn't too big of a problem in the icon code for a few reasons, but it's better to not allow directories in the first place.

Also, access() uses the real UID to check--I don't know if privilege dropping is in the pipeline, which could render that test misleading. And only the result of (f)open() is relevant, after all, which is why I did not even attempt further checks in fexists(). See for instance this comment about TOCTOU in the stackoverflow answer you linked to above.

There isn't much of a security problem in this case, since dunst just runs as a regular user. It doesn't need to do privilege dropping/escalation either (although it might provide a very small security benefit). No privilege dropping is in the pipeline.

WhitePeter · 2021-10-27T20:13:11Z

So what I'm asking is, is to replace most of the get_xdg_basedirs with add_paths_from_env. This is to prevent code duplication.

I think you should reconsider, since you are basically reinventing the wheel with add_paths_from_env, because g_get_system_data_dirs and g_get_user_data_dir do most of the work you are doing in there. get_xdg_basedirs only exists because of that unfortunate default for $XDG_CONFIG_DIRS. I don't see any value in arbitrarily choosing defaults for environment variables that have a spec that defines them. Basically, you are duplicating code you don't see, unless you study the source of Glib. ;)

get_xdg_basedirs, or more precisely the alternative default, is an exception for reasons discussed in the corresponding PR. Maybe it should be renamed, though, to make clear that is only concerned with config dirs.

According to the spec XDG_DATA_DIRS defaults to a sensible /usr/local/share:/usr/share, so maybe there is no need for custom code.

The environment variable itself doesn't default to some value. When it's empty, that value should indeed be used.

No, not if you want to be compliant with the XDG BD spec. It states a recommended default which you are, in fact, using here. What's the point, if there are readily available functions for doing that? And what would you expect to find, if you were to actually use an empty XDG_DATA_DIRS? ;)

To make it more confusing, when the spec says a file is located in XDG_DATA_DIRS/somedir you should look at XDG_DATA_HOME/somedir before looking at XDG_DATA_DIRS/somedir.

Not confusing at all, to me. XDG_DATA_HOME is the most important directory, after all. In this context (looking for an icon file) you can just stop looking if it is in there, otherwise look in the less important directories designated by XDG_DATA_DIRS.

WhitePeter · 2021-10-27T20:39:06Z

Look at the old code WRT add_paths_from_env. Basically you just need to replace g_get_system_config_dirs with g_get_system_data_dirs and g_get_user_config_dir with g_get_user_data_dir and you should have a good starting point.

Edit: corrected the URL to the old code

fwsmit · 2021-10-29T15:59:03Z

The problem is that I need a subdirectory of g_get_system_data_dirs. The old code you point to makes a temporary variable with the subdirectory path concatenated to it, but I don't want to recreate it every time. You'll also have to make custom code for adding more directories (for example ~/.icons that is still recommended by the spec). The glib library function is just annoying to deal with, so I opted not to use it.

I'm also considering contributing the icon lookup code to mako, so I want to stay as independent from glib as possible in that part (the list is easy enough to replace).

WhitePeter · 2021-10-29T18:15:38Z

The problem is that I need a subdirectory of g_get_system_data_dirs.

Yes, of course but you don't need to, and most probably should not, do that in one go. The spec is called "XDG Base Directory Specification" for a reason. You get the base directory and append the appropriate relative path(s) after. That is what get_conf_files() does as well inside the for loop.

The old code you point to makes a temporary variable with the subdirectory path concatenated to it, but I don't want to recreate it every time.

It was only meant as an example of what g_get_system_*_dirs() and g_get_uses_*_dir() can do for you.

You'll also have to make custom code for adding more directories (for example ~/.icons that is still recommended by the spec).

Yes, that is one special case for backwards compatibility. You can just prepend it to the array and drop that part of the code, once backwards compatibility is no longer required, or whatever. All the other directories are specified in the XDG BD spec.

I have cloned your fork of Dunst and was about to prepare a PR with a PoC but, for the time being, let me try some quick and dirty pseudo code to demonstrate:

static array all_xdg_data_bd_arr; /* so as to only do this once */
if (all_xdg_data_bd_arr)
        goto BUILD_SEARCH_PATH_ARR;

all_xdg_data_bd_arr[0] = g_get_user_data_dir();
extend_array(all_xdg_data_bd_arr, g_get_system_data_dirs());
/*...*/

BUILD_SEARCH_PATH_ARR:
static array all_icon_theme_basedirs;
if (all_icon_theme_basedirs)
         goto FIND_ICON_THEME;

all_icon_theme_basedirs[0] = g_build_filename(g_get_home_dir(), ".icons");
foreach xdg_data_dir in all_xdg_data_dirs do {
        icons_dir = g_build_filename(xdg_data_dir, "icons");
        append_to_array(all_icon_theme_basedirs, icons_dir);
        pixmaps_dir = g_build_filename(xdg_data_dir, "pixmaps");
        append_to_array(all_icon_theme_basedirs, pixmaps_dir);
}
/*...*/

FIND_ICON_THEME:
foreach icon_theme_basedir in all_icon_theme_basedirs do {
       find(icon_theme_base_dir, "index.theme"); /* find() walks all subdirectories */
}

(or something along those lines, haven't had time to flesh out the details and have not fully understood the spec yet, but you should get the gist of it, hopefully. The goto labels only serve as replacement for proper function declarations.)

This way you can reuse all_xdg_bd_arr for other stuff as well, only need to append the appropriate relative paths similarly to BUILD_SEARCH_PATH_ARR, or even parametrize that as well to be reusable.

The glib library function is just annoying to deal with, so I opted not to use it.

Why is it annoying? It gets used in other places as well. I find it quite convenient, so why not use it when it is a requirement for Dunst, anyway? Even your code for #947 uses it for other stuff, i.e. GPtrArray. Cannot have it both ways. ;)

I'm also considering contributing the icon lookup code to mako, so I want to stay as independent from glib as possible in that part (the list is easy enough to replace).

That is a good point, but then you would also need to rewrite any code that uses Glib, to make it portable to non-Glib projects. It's not the worst idea, though. Let me think a little more about it.

On a related note: is there another channel for communication? This does not seem like the right place. A mailing list would be great, or maybe IRC, even though I am not a frequent IRC user. ~~and we seem to be in different time zones--judging by the mail timestamps in my notifications you seem to be in GMT-7 and I am in GMT+2, so email seems more appropriate.~~ Edit: The timestamps are obviously those of githubs mail server, I just realized.

fwsmit · 2021-10-30T16:40:07Z

I'll play around with the code some more, while finishing #947.

On a related note: is there another channel for communication? This does not seem like the right place. A mailing list would be great, or maybe IRC, even though I am not a frequent IRC user. ~~and we seem to be in different time zones--judging by the mail timestamps in my notifications you seem to be in GMT-7 and I am in GMT+2, so email seems more appropriate.~~ Edit: The timestamps are obviously those of githubs mail server, I just realized.

I'm about in the same timezone as you, so IRC could be an option for real-time communication. I'm not very often online on there, though, so you would have to message me beforehand. Important conclusions should be posted to github, however.
Edit: I've discovered there is already a #dunst channel in libera.chat. I'll try to keep that open while working on dunst, but don't expect someone to be there all the time.

fwsmit

Please drop commits 05cc9fd and 46deb08 with git rebase -i HEAD~9. Make sure to save that commit somewhere in a different branch if we want to use it later.

fwsmit · 2021-11-02T12:48:14Z

src/settings.c

-                        // debug level because of low relevance
-                        LOG_D(MSG_FOPEN_FAILURE(path));
-                g_free(path);
+                        g_free(base_rc);


style: when the if statement uses brackets the else statement should as well

fwsmit · 2021-11-02T13:03:47Z

src/settings.c

+        save_settings(ini);
+
+        LOG_D("Checking/correcting settings");
+        check_and_correct_settings(&settings);


This function should only be done once, after all the config files are loaded

fwsmit · 2021-11-02T13:07:02Z

src/settings.c

+        fclose(f);
+
+        LOG_D("Loading settings");
+        save_settings(ini);


load_ini_file may return NULL. You should handle that case correctly. I'd make save_settings return immediately when the ini file is NULL.

fwsmit · 2021-11-02T13:41:57Z

src/settings.c

+ * non-empty queue.
+ */
+static void get_conf_files(GQueue *config_files) {
+        struct dirent **drop_ins;


This should be initialized to NULL. Otherwise my linter (rightfully) complains about the free statement below.

fwsmit · 2021-11-02T16:02:27Z

I've reconsidered the decision to read every config file and apply them one after the other. Apart from if it conforms better to the standards, let's consider the user benefits.
Benefits of applying all configs:

One could change something in the global dunstrc and it will be applied for all users, even if they have a user-specific dunstrc.

Benefits of applying only the last config (let's ignore drop-ins for now):

We can change the default dunstrc without breaking existing configs. This comes in for the new icon lookup, which isn't compatible with the old one. We could enable it in the default dunstrc, while still allowing other people to use the old mechanism until the next major release. Of course, this can also be used for any other incompatible change.
Rules will be in the right order. If the system-wide dunstrc defines rule A before section B and the user-specific config defines them the other way around, rule A will still be applied before rule B.

To me the benefits of easy backward compatibility, while still providing the best defaults for new users is very nice, while I don't think there are many benefits with applying all configs. What are your thoughts?

Rules will be in the right order. If the system-wide dunstrc defines rule A before section B and the user-specific config defines them the other way around, rule A will still be applied before rule B.

Also, we need to think about this either way, since the drop-ins have this problem as well. We could make all rules from other files independent by adding something like a filename attribute to each rule. I think this is necessary to avoid confusion from users.

fwsmit · 2021-11-02T16:12:31Z

I've also taken a look at the doxygen output and I do agree that the separate macros might make the doxygen output a bit clearer. But I don't think this should go over the code readability. I found myself scrolling back to the macro definitions way too often, even just while reviewing this PR. So I do stand by my point of removing at least the macros XDG_CONFIG_DIRS_DEFAULT, DROP_IN_DIR and DROP_IN_PATTERN.

XDG_CONFIG_DIRS_DEFAULT can be replaced with SYSCONFDIR where it's used. The comment can be moved to SYSCONFDIR.
The definition of SYSCONFDIR is fine by me, since it doesn't decrease the readability.
I don't entirely like BASE_RC, but it can stay for now, since it's used in two places.

The relevant comments may be moved to the function description or somewhere in the function. Indented comments inside functions are not a problem to me.

fwsmit · 2021-11-03T12:22:33Z

Also, could you add some tests for the added functions, where possible. This comes with the added bonus of valgrind testing for memleaks.

fwsmit · 2021-11-08T10:23:05Z

@WhitePeter have you had some time to look at this?

fwsmit · 2021-12-06T16:22:20Z

@WhitePeter Since you don't seem to have the time to finish this, I'll go ahead an do that (keeping your commit attribution intact, of course).

EDIT messed a bit with rebasing, but feel free to overwrite. I'll go ahead an continue in another branch.

Dial back a little on the space requirement of the prefix for messages of levels "debug", "error" and "critical". The prefix now only includes the function name, padded to 16 characters so as to make for better vertical alignment, and the line number in the source file. Additionally updated feature test macro to make this work with recent enough versions of clang as well. config.mk: Disable warnings about empty __VA_ARGS__ Since compilation is done with '-std=gnu99' and token pasting of ',' and '__VA_ARGS__' is a GNU extension which also works with clang (>=6) this warning should be safe to disable, see also the changes to log.h above. The CI did not complain in my local tests. Only clang threw this warning before and only to inform that it is indeed a GNU extension, despite using the same '-std=gnu99'. This should not mask genuine errors when __VA_ARGS__ must not be empty, i.e. usage without token pasting it with ',', since those should be errors that lead to unsuccessful compilation, anyway.

@include

This is basically a proof of concept, so not much official documentation just yet. Include syntax is a little more strict. The keyword is "@include", which is borrowed from doxygen. There MUST not be any blanks before the "@", like in C. Some reasoning for these preliminary decisions: Includes should stand out and thus should get some kind of special character prefix, but since '#' is already taken, they could be mistaken for comments, especially with syntax highlighting enabled. Using all capitol letters and not allowing spaces in front also serves to make them more distinguishable. Drive-by fix: Sections surrounded by spaces are no longer possible.

Also some refinements to the FILES section.

Refactoring for necessarily added complexity and push file handling into 'load_settings()', because otherwise there would be virtually no limit on how many files could be open at the same time, while we only read them sequentially, anyway.

Also reverted stripping of section names.

A more thorough check of what constitutes as *useable* and readable file.

Moved from settings and renamed, a small wrapper around `is_readable_file()` and `fopen()`.

Do away with the 'G_*' prefix of macros because it is misleading. The original intention was to make clear that they are derived from Glib functions but instead they might be mistaken for *actual* Glib content. Also added some more documentation for clarity.

Also fixed a double free issue with the provided path parameter. Removed leftover prototype of fopen_conf().

fwsmit · 2021-12-06T17:14:19Z

This PR is now in a non-compilable state, sorry for that. It doesn't really matter, though, since I am finishing it in #997. Closing this one, as it's not needed anymore.

WhitePeter marked this pull request as ready for review October 16, 2021 01:20

WhitePeter force-pushed the dunstrc_d branch from 5223212 to 24e4380 Compare October 16, 2021 15:15

WhitePeter force-pushed the dunstrc_d branch from 24e4380 to c9565ad Compare October 16, 2021 15:42

WhitePeter force-pushed the dunstrc_d branch from c9565ad to 92dae76 Compare October 17, 2021 21:20

fwsmit reviewed Oct 18, 2021

View reviewed changes

fwsmit reviewed Oct 19, 2021

View reviewed changes

src/settings.c Show resolved Hide resolved

fwsmit mentioned this pull request Oct 24, 2021

Dunst cannot be configured through command line arguments #940

Closed

fwsmit reviewed Oct 25, 2021

View reviewed changes

config.mk Show resolved Hide resolved

fwsmit reviewed Oct 27, 2021

View reviewed changes

WhitePeter force-pushed the dunstrc_d branch from 92dae76 to c5fbdbc Compare October 28, 2021 18:49

fwsmit reviewed Nov 2, 2021

View reviewed changes

WhitePeter added 9 commits December 6, 2021 17:52

dunst.1: Document drop-in support.

55ff28a

Also some refinements to the FILES section.

settings: Initial support for drop-ins.

6dfe06d

Refactoring for necessarily added complexity and push file handling into 'load_settings()', because otherwise there would be virtually no limit on how many files could be open at the same time, while we only read them sequentially, anyway.

ini.c: Removed include code.

cf9d46c

Also reverted stripping of section names.

utils: new function is_readable_file()

3b74beb

A more thorough check of what constitutes as *useable* and readable file.

utils: new function fopen_verbose()

39e0dba

Moved from settings and renamed, a small wrapper around `is_readable_file()` and `fopen()`.

settings: Refactor load_settings()

2cb78ea

Also fixed a double free issue with the provided path parameter. Removed leftover prototype of fopen_conf().

fwsmit force-pushed the dunstrc_d branch from 9b850a9 to 2cb78ea Compare December 6, 2021 16:58

fwsmit closed this Dec 6, 2021

	return 0 == fnmatch(DROPIN_PAT, dent->d_name, FNM_PATHNAME \| FNM_PERIOD)
	return 0 == fnmatch("*.conf", dent->d_name, FNM_PATHNAME \| FNM_PERIOD)


		static int is_dropin(const struct dirent *dent);

		static bool fexists(char * const path);

Add support for config drop-ins (rudimentary includes as a bonus) #933

Add support for config drop-ins (rudimentary includes as a bonus) #933

Conversation

WhitePeter commented Oct 16, 2021 • edited Loading

codecov-commenter commented Oct 16, 2021 • edited Loading

Codecov Report

WhitePeter commented Oct 16, 2021

WhitePeter commented Oct 16, 2021

WhitePeter commented Oct 17, 2021

fwsmit left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

WhitePeter commented Oct 19, 2021

fwsmit commented Oct 21, 2021

fwsmit left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

WhitePeter Oct 25, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

WhitePeter Oct 27, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

WhitePeter Oct 25, 2021 • edited Loading

Choose a reason for hiding this comment

WhitePeter commented Oct 25, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

WhitePeter Oct 27, 2021 • edited Loading

Choose a reason for hiding this comment

fwsmit commented Oct 27, 2021 • edited Loading

WhitePeter commented Oct 27, 2021 • edited Loading

WhitePeter commented Oct 27, 2021 • edited Loading

fwsmit commented Oct 29, 2021

WhitePeter commented Oct 29, 2021 • edited Loading

fwsmit commented Oct 30, 2021 • edited Loading

fwsmit left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fwsmit commented Nov 2, 2021

fwsmit commented Nov 2, 2021

fwsmit commented Nov 3, 2021 • edited Loading

fwsmit commented Nov 8, 2021

fwsmit commented Dec 6, 2021 • edited Loading

fwsmit commented Dec 6, 2021

WhitePeter commented Oct 16, 2021 •

edited

Loading

codecov-commenter commented Oct 16, 2021 •

edited

Loading

WhitePeter Oct 25, 2021 •

edited

Loading

WhitePeter Oct 27, 2021 •

edited

Loading

WhitePeter Oct 25, 2021 •

edited

Loading

WhitePeter commented Oct 25, 2021 •

edited

Loading

WhitePeter Oct 27, 2021 •

edited

Loading

fwsmit commented Oct 27, 2021 •

edited

Loading

WhitePeter commented Oct 27, 2021 •

edited

Loading

WhitePeter commented Oct 27, 2021 •

edited

Loading

WhitePeter commented Oct 29, 2021 •

edited

Loading

fwsmit commented Oct 30, 2021 •

edited

Loading

fwsmit commented Nov 3, 2021 •

edited

Loading

fwsmit commented Dec 6, 2021 •

edited

Loading