From 43cb1082675cf6ee78f6020f7150110519a2a83e Mon Sep 17 00:00:00 2001 From: WhitePeter Date: Fri, 15 Oct 2021 18:54:17 +0200 Subject: [PATCH 1/9] log.h: Refine prefix of some message levels. 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. --- config.mk | 2 +- src/log.h | 15 +++++++-------- 2 files changed, 8 insertions(+), 9 deletions(-) diff --git a/config.mk b/config.mk index c1d41bd87..753797946 100644 --- a/config.mk +++ b/config.mk @@ -35,7 +35,7 @@ ENABLE_WAYLAND= -DENABLE_WAYLAND endif # flags -DEFAULT_CPPFLAGS = -D_DEFAULT_SOURCE -DVERSION=\"${VERSION}\" -DSYSCONFDIR=\"${SYSCONFDIR}\" +DEFAULT_CPPFLAGS = -Wno-gnu-zero-variadic-macro-arguments -D_DEFAULT_SOURCE -DVERSION=\"${VERSION}\" -DSYSCONFDIR=\"${SYSCONFDIR}\" DEFAULT_CFLAGS = -g -std=gnu99 -pedantic -Wall -Wno-overlength-strings -Os ${ENABLE_WAYLAND} ${EXTRA_CFLAGS} DEFAULT_LDFLAGS = -lm -lrt diff --git a/src/log.h b/src/log.h index 7819b660b..54290d10c 100644 --- a/src/log.h +++ b/src/log.h @@ -17,24 +17,23 @@ * @... are the arguments to above format string. * * This requires -Wno-gnu-zero-variadic-macro-arguments with clang - * because __VA_OPTS__ seems not to be officially supported yet. - * However, the result is the same with both gcc and clang. + * because of token pasting ',' and %__VA_ARGS__ being a GNU extension. + * However, the result is the same with both gcc and clang and since we are + * compiling with '-std=gnu99', this should be fine. */ -#if __GNUC__ >= 8 - -#define MSG(format, ...) "[%s:%s:%04d] " format, __FILE__, __func__, __LINE__, ## __VA_ARGS__ +#if __GNUC__ >= 8 || __clang_major__ >= 6 +#define MSG(format, ...) "[%16s:%04d] " format, __func__, __LINE__, ## __VA_ARGS__ +#endif +#ifdef MSG // These should benefit from more context #define LOG_E(...) g_error(MSG(__VA_ARGS__)) #define LOG_C(...) g_critical(MSG(__VA_ARGS__)) #define LOG_D(...) g_debug(MSG(__VA_ARGS__)) - #else - #define LOG_E g_error #define LOG_C g_critical #define LOG_D g_debug - #endif #define LOG_W g_warning From f5fb8abdbd1a5e7d2a770ba14e3866f7134a3ede Mon Sep 17 00:00:00 2001 From: WhitePeter Date: Sat, 16 Oct 2021 01:19:07 +0200 Subject: [PATCH 2/9] ini.c: Rudimentary 'include' support. 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. --- src/ini.c | 27 +++++++++++++++++++++++++++ src/option_parser.c | 1 - 2 files changed, 27 insertions(+), 1 deletion(-) diff --git a/src/ini.c b/src/ini.c index ad4d9010b..a2c5c2534 100644 --- a/src/ini.c +++ b/src/ini.c @@ -2,6 +2,7 @@ #include "utils.h" #include "log.h" +#include "settings.h" struct section *get_section(struct ini *ini, const char *name) { @@ -92,6 +93,32 @@ struct ini *load_ini_file(FILE *fp) while (getline(&line, &line_len, fp) != -1) { line_num++; + /** Crude file inclusion support. + * + * TODO: Beware of include loops! Should we care more than + * warning about it in the manual? */ + if (0 == strncmp("@INCLUDE", line, 8) && strchr(line, '=')) { + gchar **split_line = g_strsplit(line, "=", 2); + gchar *inc_path; + /* Double check since previous one would allow "@INCLUDEFOO" */ + if (0 == strcmp("@INCLUDE", g_strstrip(split_line[0])) + && (inc_path = g_strstrip(split_line[1]))) { + LOG_D("Trying to include '%s'", inc_path); + FILE *f; + + if ((f = fopen_conf(inc_path))) { + load_ini_file(f); + LOG_D("Closing included file, fd: '%d'", fileno(f)); + fclose(f); + } else + g_free(inc_path); + } else + LOG_W("Invalid '@INCLUDE' line '%d'", line_num); + + g_strfreev(split_line); + continue; + } + char *start = g_strstrip(line); if (*start == ';' || *start == '#' || STR_EMPTY(start)) diff --git a/src/option_parser.c b/src/option_parser.c index 041107f98..902430678 100644 --- a/src/option_parser.c +++ b/src/option_parser.c @@ -213,7 +213,6 @@ int string_parse_bool(const void *data, const char *s, void *ret) return success; } - int get_setting_id(const char *key, const char *section) { int error_code = 0; int partial_match_id = -1; From 55ff28adf4e5912f59ad04ae559ca9251e020387 Mon Sep 17 00:00:00 2001 From: WhitePeter Date: Sat, 16 Oct 2021 01:37:30 +0200 Subject: [PATCH 3/9] dunst.1: Document drop-in support. Also some refinements to the FILES section. --- docs/dunst.1.pod | 51 +++++++++++++++++++++++++++++++++--------------- 1 file changed, 35 insertions(+), 16 deletions(-) diff --git a/docs/dunst.1.pod b/docs/dunst.1.pod index 9c85db33f..232bed423 100644 --- a/docs/dunst.1.pod +++ b/docs/dunst.1.pod @@ -100,38 +100,57 @@ missed notifications after returning to the computer. =head1 FILES These are the base directories dunst searches for configuration files in -descending order of imortance: +I: -=over 4 +=over 8 + +=item C<$XDG_CONFIG_HOME> + +This is the most important directory. (C<$HOME/.config> if unset or empty) -$XDG_CONFIG_HOME ($HOME/.config if unset or empty) +=item C<$XDG_CONFIG_DIRS> -$XDG_CONFIG_DIRS (:-separated list of base directories in descending order -of importance; ##SYSCONFDIR## if unset or empty) +This, like C<$PATH> for instance, is a :-separated list of base directories +in I. +(F<##SYSCONFDIR##> if unset or empty) =back -Dunst will search these directories for the following relative path: +Dunst will search these directories for the following relative file paths: -=over 4 +=over 8 -dunst/dunstrc +=item F -=back +This is the base config and as such the least important in a particular base +directory. -All settings from all files get applied. Settings in more important files -override those in less important ones. Settings not present in more imortant -files are taken from a less important one or from the internal defaults if they -are not found in any file. +=item F -=over 4 +These are "drop-ins" (mind the ".d" suffix of the directory). +They are more important than the base dunstrc in the parent directory, as they +are considered to be small snippets to override settings. +The last in lexical order is the most important one, so you can easily change +the order by renaming them. +A common approach to naming drop-ins is to prefix them with numbers, i.e.: -=item This is where the default (least important) config file is located: + 00-least-important.conf + 01-foo.conf + 20-bar.conf + 99-most-important.conf -##SYSCONFDIR##/dunst/dunstrc +The only requirements are that the name of the file must have the correct +suffix and the first non-blank line that is not a comment must be a section or +rule identifier. =back +All settings from all files get applied. Settings in more important files +override those in less important ones. +Settings not present in more imortant files get their value from a less +important one or from the internal defaults if respective key is not found in +any file. + =head1 AUTHORS Written by Sascha Kruse From 6dfe06d74ab0786dbc151171d31d47f375853ad0 Mon Sep 17 00:00:00 2001 From: WhitePeter Date: Sat, 16 Oct 2021 01:13:31 +0200 Subject: [PATCH 4/9] settings: Initial support for drop-ins. 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. --- src/settings.c | 281 +++++++++++++++++++++++++++++++++++-------------- src/settings.h | 11 ++ 2 files changed, 213 insertions(+), 79 deletions(-) diff --git a/src/settings.c b/src/settings.c index 17cda7365..538cafd1f 100644 --- a/src/settings.c +++ b/src/settings.c @@ -1,10 +1,20 @@ /* copyright 2013 Sascha Kruse and contributors (see LICENSE for licensing information) */ +/** @file src/settings.c + * @brief Take care of the settings. + */ + #include "settings.h" +#include +#include +#include #include #include #include +#include +#include +#include #include "dunst.h" #include "log.h" @@ -16,91 +26,193 @@ #include "x11/x.h" #include "output.h" -#ifdef SYSCONFDIR -#define XDG_CONFIG_DIRS_DEFAULT SYSCONFDIR // alternative default -#else -#define XDG_CONFIG_DIRS_DEFAULT "/etc/xdg" +#ifndef SYSCONFDIR +/** @brief Fallback for doxygen, mostly. + * + * Since this gets defined by $DEFAULT_CPPFLAGS at compile time doxygen seems to + * miss the correct value. + */ +#define SYSCONFDIR "/usr/local/etc/xdg" #endif -// path to dunstrc, relative to config directory -#define RPATH_RC "dunst/dunstrc" -#define RPATH_RC_D RPATH_RC ".d/*.conf" +/** @brief Alternative default for XDG_CONFIG_DIRS + * + * Fix peculiar behaviour if installed to a local PREFIX, i.e. + * /usr/local, when SYSCONFDIR should be /usr/local/etc/xdg and not + * /etc/xdg, hence use SYSCONFDIR (defined at compile time, see + * config.mk) as default for XDG_CONFIG_DIRS. The spec says 'should' + * and not 'must' use /etc/xdg. Users/admins can override this by + * 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 + +/** @brief Convenience macro to not have to put %NULL as the last argument. + * + * @returns a newly-allocated string that must be freed with g_free(). + */ +#define G_STRCONCAT(...) g_strconcat(__VA_ARGS__, NULL) + +/** @brief Generate path to base config 'dunstrc' in a base directory + * + * @returns a newly-allocated string that must be freed with g_free(). + */ +#define G_BASE_RC(basedir) g_build_filename(basedir, "dunst", "dunstrc", NULL) + +/** @brief Generate drop-in directory path for a base directory + * + * @returns a newly-allocated string that must be freed with g_free(). + */ +#define G_DROP_IN_DIR(basedir) G_STRCONCAT(G_BASE_RC(basedir), ".d") + +/** @brief Match pattern for drop-in file names */ +#define DROP_IN_PATTERN "*.conf" struct settings settings; -/** - * 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); + +static int is_drop_in(const struct dirent *dent); + +static bool fexists(char * const path); + +static void get_conf_files(GQueue *config_files); + +/** @brief Filter for scandir(). * - * @param cmdline_config_path - * @returns GQueue* of FILE* to config files - * @retval empty GQueue* if no file could be opened + * @returns @brief An integer indicating success * - * Use g_queue_pop_tail() to retrieve FILE* in *ascending* order of importance + * @retval @brief 1 if file name matches #DROP_IN_PATTERN + * @retval @brief 0 otherwise * - * Use g_queue_free() to free if not NULL + * @param dent [in] @brief directory entry */ -static GQueue *open_conf_files(char *cmdline_config_path) { - FILE *config_file; +static int is_drop_in(const struct dirent *dent) +{ + return 0 == fnmatch(DROP_IN_PATTERN, dent->d_name, FNM_PATHNAME | FNM_PERIOD) + ? 1 // success + : 0; +} - // Used as stack, least important pushed last but popped first. - GQueue *config_files = g_queue_new(); +/** @brief Check if file exists and is regular file or named pipe + * + * This is a convenience function to check if @p path can be resolved and makes + * sense to try and open as config, like regular files and FIFOs (named pipes). + * + * @param path [in] A string representing a path. + * @retval true in case of success. + * @retval false in case of failure and errno will be set EINVAL. + */ +static bool fexists(char * const path) { + struct stat statbuf; + bool result = false; + + if (0 == stat(path, &statbuf)) { + if (statbuf.st_mode & (S_IFIFO | S_IFREG)) + /** See what intersting stuff can be done with FIFOs + * for dunstrc/drop-ins. */ + result = true; + else + /** Sets errno if stat() was successful but @p path + * [in] does not point to a file/FIFO. This just in + * case someone queries errno which would otherwise + * indicate success. */ + errno = EINVAL; + } - if (cmdline_config_path) { - config_file = STR_EQ(cmdline_config_path, "-") - ? stdin - : fopen(cmdline_config_path, "r"); - - if (config_file) { - LOG_I(MSG_FOPEN_SUCCESS(cmdline_config_path, config_file)); - g_queue_push_tail(config_files, config_file); - } else { - // warn because we exit early - LOG_W(MSG_FOPEN_FAILURE(cmdline_config_path)); - } + return result; +} - return config_files; // ignore other config files if '-conf' given +/** + * Returns a %NULL-terminated array of all XDG Base Directories, @e most @e + * important @e first. + * + * @returns The array of paths to XDG base directories in @e descending order of + * importance + */ +static const char * const *get_xdg_basedirs() { + static const char * const *xdg_bd_arr = NULL; + if (!xdg_bd_arr) { + char * xdg_basedirs; + + char * const xcd_env = getenv("XDG_CONFIG_DIRS"); + char * const xdg_config_dirs = xcd_env && strnlen(xcd_env, 1) + ? xcd_env + : XDG_CONFIG_DIRS_DEFAULT; + + /* + * Prepend XDG_CONFIG_HOME, most important first because + * XDG_CONFIG_DIRS is already ordered that way. + */ + xdg_basedirs = G_STRCONCAT(g_get_user_config_dir(), + ":", + xdg_config_dirs); + free(xdg_config_dirs); + LOG_D("Config directories: '%s'", xdg_basedirs); + + xdg_bd_arr = (const char * const *) string_to_array(xdg_basedirs, ":"); + g_free(xdg_basedirs); } + return xdg_bd_arr; +} + +/** @brief Find all config files. + * + * Searches all XDG base directories for config files and drop-ins and push + * them onto a LIFO/stack, @e least important last. + * + * @param config_files [in|out] A pointer to a GQueue of strings representing + * config file paths + * + * Use g_queue_pop_tail() to retrieve paths in @e ascending order of importance + * + * Use g_queue_free() to free. + */ +static void get_conf_files(GQueue *config_files) { + struct dirent **drop_ins; + for (const char * const *d = get_xdg_basedirs(); *d; d++) { + gchar * const base_rc = G_BASE_RC(*d); + gchar * const drop_in_dir = G_DROP_IN_DIR(*d); + + int n = scandir(drop_in_dir, &drop_ins, is_drop_in, alphasort); + /* reverse order to get most important first */ + while (n-- > 0) { + gchar * const drop_in = G_STRCONCAT(drop_in_dir, + "/", + drop_ins[n]->d_name); + free(drop_ins[n]); + + if (fexists(drop_in)) { + LOG_D("Adding drop-in '%s'", drop_in); + g_queue_push_tail(config_files, drop_in); + } else + g_free(drop_in); + } - /* - * Fix peculiar behaviour if installed to a local PREFIX, i.e. - * /usr/local, when SYSCONFDIR should be /usr/local/etc/xdg and not - * /etc/xdg, hence use SYSCONFDIR (defined at compile time, see - * config.mk) as default for XDG_CONFIG_DIRS. The spec says 'should' and - * not 'must' use /etc/xdg. - * Users/admins can override this by explicitly setting XDG_CONFIG_DIRS - * to their liking at runtime or by setting SYSCONFDIR=/etc/xdg at - * compile time. - */ - gchar * const xdg_cdirs = g_strdup(g_getenv("XDG_CONFIG_DIRS")); - gchar * const xdg_config_dirs = xdg_cdirs && strnlen((gchar *) xdg_cdirs, 1) - ? g_strdup(xdg_cdirs) - : g_strdup(XDG_CONFIG_DIRS_DEFAULT); - g_free(xdg_cdirs); - - /* - * Prepend XDG_CONFIG_HOME, most important first because XDG_CONFIG_DIRS - * is already ordered that way. - */ - gchar * const all_conf_dirs = g_strconcat(g_get_user_config_dir(), ":", - g_strdup(xdg_config_dirs), NULL); - g_free(xdg_config_dirs); - LOG_D("Config directories: '%s'", all_conf_dirs); - - for (gchar * const *d = string_to_array(all_conf_dirs, ":"); *d; d++) { - gchar * const path = string_to_path(g_strconcat(*d, "/", RPATH_RC, NULL)); - if ((config_file = fopen(path, "r"))) { - LOG_I(MSG_FOPEN_SUCCESS(path, config_file)); - g_queue_push_tail(config_files, config_file); + /* base rc-file last, least important */ + if (fexists(base_rc)) { + LOG_D("Adding base config '%s'", base_rc); + g_queue_push_tail(config_files, base_rc); } else - // debug level because of low relevance - LOG_D(MSG_FOPEN_FAILURE(path)); - g_free(path); + g_free(base_rc); + + g_free(drop_in_dir); } - g_free(all_conf_dirs); + free(drop_ins); +} + +FILE *fopen_conf(char * const path) +{ + FILE *f = NULL; + char *real_path = string_to_path(strdup(path)); - return config_files; + if (fexists(real_path) && (f = fopen(real_path, "r"))) + LOG_I(MSG_FOPEN_SUCCESS(path, f)); + else + LOG_W(MSG_FOPEN_FAILURE(path)); + + free(real_path); + return f; } void settings_init() { @@ -178,22 +290,33 @@ void check_and_correct_settings(struct settings *s) { } -void load_settings(char *cmdline_config_path) { - FILE *config_file; - GQueue *config_files; - +void load_settings(char *path) { settings_init(); LOG_D("Setting defaults"); set_defaults(); - config_files = open_conf_files(cmdline_config_path); - if (g_queue_is_empty(config_files)) { + GQueue *config_files = g_queue_new(); + + if (path) /** If @p path [in] was supplied it will be the only one tried. */ + g_queue_push_tail(config_files, path); + else + get_conf_files(config_files); + + if (g_queue_is_empty(config_files)) LOG_W("No configuration file, using defaults"); - } else { // Add entries from all files, most important last - while ((config_file = g_queue_pop_tail(config_files))) { - LOG_I("Parsing config, fd: '%d'", fileno(config_file)); - struct ini *ini = load_ini_file(config_file); - fclose(config_file); + else { /* Load all conf files and drop-ins, least important first. */ + gchar *path; + while ((path = g_queue_pop_tail(config_files))) { + LOG_D("Trying to open '%s'", path); + FILE *f; + /* Check for "-" here, so the file handling stays in one place */ + if (!(f = STR_EQ(path, "-") ? stdin : fopen_conf(path))) + continue; + + LOG_I("Parsing config, fd: '%d'", fileno(f)); + struct ini *ini = load_ini_file(f); + LOG_D("Closing config, fd: '%d'", fileno(f)); + fclose(f); LOG_D("Loading settings"); save_settings(ini); diff --git a/src/settings.h b/src/settings.h index 80d84927c..a81d07bed 100644 --- a/src/settings.h +++ b/src/settings.h @@ -163,5 +163,16 @@ extern struct settings settings; void load_settings(char *cmdline_config_path); +/** \brief Open files verbosely. + * + * This is a wrapper around fopen() and fexists() that does some preliminary + * checks and sends log messages. + * + * @returns The result of the fopen() call. + * @retval NULL if the fopen() call failed or the file does not exist, see + * also fexists(). + */ +FILE * fopen_conf(char * const path); + #endif /* vim: set ft=c tabstop=8 shiftwidth=8 expandtab textwidth=0: */ From cf9d46c4bdbb9a645495cbfc73206caaa7ee4c07 Mon Sep 17 00:00:00 2001 From: WhitePeter Date: Thu, 28 Oct 2021 05:45:14 +0200 Subject: [PATCH 5/9] ini.c: Removed include code. Also reverted stripping of section names. --- src/ini.c | 28 +--------------------------- 1 file changed, 1 insertion(+), 27 deletions(-) diff --git a/src/ini.c b/src/ini.c index a2c5c2534..3db157ed6 100644 --- a/src/ini.c +++ b/src/ini.c @@ -93,32 +93,6 @@ struct ini *load_ini_file(FILE *fp) while (getline(&line, &line_len, fp) != -1) { line_num++; - /** Crude file inclusion support. - * - * TODO: Beware of include loops! Should we care more than - * warning about it in the manual? */ - if (0 == strncmp("@INCLUDE", line, 8) && strchr(line, '=')) { - gchar **split_line = g_strsplit(line, "=", 2); - gchar *inc_path; - /* Double check since previous one would allow "@INCLUDEFOO" */ - if (0 == strcmp("@INCLUDE", g_strstrip(split_line[0])) - && (inc_path = g_strstrip(split_line[1]))) { - LOG_D("Trying to include '%s'", inc_path); - FILE *f; - - if ((f = fopen_conf(inc_path))) { - load_ini_file(f); - LOG_D("Closing included file, fd: '%d'", fileno(f)); - fclose(f); - } else - g_free(inc_path); - } else - LOG_W("Invalid '@INCLUDE' line '%d'", line_num); - - g_strfreev(split_line); - continue; - } - char *start = g_strstrip(line); if (*start == ';' || *start == '#' || STR_EMPTY(start)) @@ -134,7 +108,7 @@ struct ini *load_ini_file(FILE *fp) *end = '\0'; g_free(current_section); - current_section = (g_strdup(start + 1)); + current_section = g_strdup(start + 1); continue; } From 3b74bebf9b933797ab96a381bf362abdb2691863 Mon Sep 17 00:00:00 2001 From: WhitePeter Date: Thu, 28 Oct 2021 18:06:40 +0200 Subject: [PATCH 6/9] utils: new function `is_readable_file()` A more thorough check of what constitutes as *useable* and readable file. --- src/icon.c | 5 ----- src/settings.c | 40 +++------------------------------------- src/utils.c | 24 ++++++++++++++++++++++++ src/utils.h | 20 ++++++++++++++++++++ 4 files changed, 47 insertions(+), 42 deletions(-) diff --git a/src/icon.c b/src/icon.c index 7e25f1cf3..e897c382f 100644 --- a/src/icon.c +++ b/src/icon.c @@ -13,11 +13,6 @@ #include "utils.h" #include "icon-lookup.h" -static bool is_readable_file(const char *filename) -{ - return (access(filename, R_OK) != -1); -} - /** * Reassemble the data parts of a GdkPixbuf into a cairo_surface_t's data field. * diff --git a/src/settings.c b/src/settings.c index 538cafd1f..fbec3b17a 100644 --- a/src/settings.c +++ b/src/settings.c @@ -12,9 +12,6 @@ #include #include #include -#include -#include -#include #include "dunst.h" #include "log.h" @@ -74,8 +71,6 @@ static const char * const *get_xdg_basedirs(void); static int is_drop_in(const struct dirent *dent); -static bool fexists(char * const path); - static void get_conf_files(GQueue *config_files); /** @brief Filter for scandir(). @@ -94,35 +89,6 @@ static int is_drop_in(const struct dirent *dent) : 0; } -/** @brief Check if file exists and is regular file or named pipe - * - * This is a convenience function to check if @p path can be resolved and makes - * sense to try and open as config, like regular files and FIFOs (named pipes). - * - * @param path [in] A string representing a path. - * @retval true in case of success. - * @retval false in case of failure and errno will be set EINVAL. - */ -static bool fexists(char * const path) { - struct stat statbuf; - bool result = false; - - if (0 == stat(path, &statbuf)) { - if (statbuf.st_mode & (S_IFIFO | S_IFREG)) - /** See what intersting stuff can be done with FIFOs - * for dunstrc/drop-ins. */ - result = true; - else - /** Sets errno if stat() was successful but @p path - * [in] does not point to a file/FIFO. This just in - * case someone queries errno which would otherwise - * indicate success. */ - errno = EINVAL; - } - - return result; -} - /** * Returns a %NULL-terminated array of all XDG Base Directories, @e most @e * important @e first. @@ -182,7 +148,7 @@ static void get_conf_files(GQueue *config_files) { drop_ins[n]->d_name); free(drop_ins[n]); - if (fexists(drop_in)) { + if (is_readable_file(drop_in)) { LOG_D("Adding drop-in '%s'", drop_in); g_queue_push_tail(config_files, drop_in); } else @@ -190,7 +156,7 @@ static void get_conf_files(GQueue *config_files) { } /* base rc-file last, least important */ - if (fexists(base_rc)) { + if (is_readable_file(base_rc)) { LOG_D("Adding base config '%s'", base_rc); g_queue_push_tail(config_files, base_rc); } else @@ -206,7 +172,7 @@ FILE *fopen_conf(char * const path) FILE *f = NULL; char *real_path = string_to_path(strdup(path)); - if (fexists(real_path) && (f = fopen(real_path, "r"))) + if (is_readable_file(real_path) && (f = fopen(real_path, "r"))) LOG_I(MSG_FOPEN_SUCCESS(path, f)); else LOG_W(MSG_FOPEN_FAILURE(path)); diff --git a/src/utils.c b/src/utils.c index 07ca9189b..e35eacbef 100644 --- a/src/utils.c +++ b/src/utils.c @@ -6,8 +6,11 @@ #include #include #include +#include #include #include +#include +#include #include #include @@ -406,4 +409,25 @@ char *string_strip_brackets(const char* s) { } +/* see utils.h */ +bool is_readable_file(const char * const path) +{ + struct stat statbuf; + bool result = false; + + if (0 == stat(path, &statbuf)) { + /** See what intersting stuff can be done with FIFOs */ + if (!(statbuf.st_mode & (S_IFIFO | S_IFREG))) { + /** Sets errno if stat() was successful but @p path [in] + * does not point to a regular file or FIFO. This + * just in case someone queries errno which would + * otherwise indicate success. */ + errno = EINVAL; + } else if (0 == access(path, R_OK)) { /* must also be readable */ + result = true; + } + } + + return result; +} /* vim: set ft=c tabstop=8 shiftwidth=8 expandtab textwidth=0: */ diff --git a/src/utils.h b/src/utils.h index 84bc660aa..326cba02d 100644 --- a/src/utils.h +++ b/src/utils.h @@ -198,5 +198,25 @@ char *string_strip_brackets(const char* s); * Returns the length of a string array, -1 if the input is NULL. */ int string_array_length(char **s); + +/** @brief Check if file is readable + * + * This is a convenience function to check if @p path can be resolved and makes + * sense to try and open, like regular files and FIFOs (named pipes). Finally, + * a preliminary check is done to see if read permission is granted. + * + * Do not rely too hard on the result, though, since this is racy. A case can + * be made that these conditions might not be true anymore by the time the file + * is acutally opened for reading. + * + * Also, no tilde expansion is done. Use the result of `string_to_path()` for + * @p path. + * + * @param path [in] A string representing a path. + * @retval true in case of success. + * @retval false in case of failure, errno will be set appropriately. + */ +bool is_readable_file(const char * const path); + #endif /* vim: set ft=c tabstop=8 shiftwidth=8 expandtab textwidth=0: */ From 39e0dba59598b4dde570d5f5d33c8636b615690d Mon Sep 17 00:00:00 2001 From: WhitePeter Date: Thu, 28 Oct 2021 18:32:28 +0200 Subject: [PATCH 7/9] utils: new function `fopen_verbose()` Moved from settings and renamed, a small wrapper around `is_readable_file()` and `fopen()`. --- src/settings.c | 2 +- src/utils.c | 15 +++++++++++++++ src/utils.h | 15 ++++++++++++++- 3 files changed, 30 insertions(+), 2 deletions(-) diff --git a/src/settings.c b/src/settings.c index fbec3b17a..de159cc08 100644 --- a/src/settings.c +++ b/src/settings.c @@ -276,7 +276,7 @@ void load_settings(char *path) { LOG_D("Trying to open '%s'", path); FILE *f; /* Check for "-" here, so the file handling stays in one place */ - if (!(f = STR_EQ(path, "-") ? stdin : fopen_conf(path))) + if (!(f = STR_EQ(path, "-") ? stdin : fopen_verbose(path))) continue; LOG_I("Parsing config, fd: '%d'", fileno(f)); diff --git a/src/utils.c b/src/utils.c index e35eacbef..f337cb6fd 100644 --- a/src/utils.c +++ b/src/utils.c @@ -430,4 +430,19 @@ bool is_readable_file(const char * const path) return result; } + +FILE *fopen_verbose(const char * const path) +{ + FILE *f = NULL; + char *real_path = string_to_path(strdup(path)); + + if (is_readable_file(real_path) && (f = fopen(real_path, "r"))) + LOG_I(MSG_FOPEN_SUCCESS(path, f)); + else + LOG_W(MSG_FOPEN_FAILURE(path)); + + free(real_path); + return f; +} + /* vim: set ft=c tabstop=8 shiftwidth=8 expandtab textwidth=0: */ diff --git a/src/utils.h b/src/utils.h index 326cba02d..70d1db132 100644 --- a/src/utils.h +++ b/src/utils.h @@ -3,8 +3,9 @@ #define DUNST_UTILS_H #include -#include #include +#include +#include //! Test if a string is NULL or empty #define STR_EMPTY(s) (!s || (*s == '\0')) @@ -218,5 +219,17 @@ int string_array_length(char **s); */ bool is_readable_file(const char * const path); +/** @brief Open files verbosely. + * + * This is a wrapper around fopen() and is_readable_file() that does some + * preliminary checks and sends log messages. + * + * @p path [in] A char* string representing a filesystem path + * @returns The result of the fopen() call. + * @retval NULL if the fopen() call failed or @p path does not satisfy the + * conditions of is_readable_file(). + */ +FILE * fopen_verbose(const char * const path); + #endif /* vim: set ft=c tabstop=8 shiftwidth=8 expandtab textwidth=0: */ From 9ed126ac09dea9ac958ff7ea30f6285e1df34749 Mon Sep 17 00:00:00 2001 From: WhitePeter Date: Thu, 28 Oct 2021 18:54:56 +0200 Subject: [PATCH 8/9] settings.c: Update functions/macros. 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. --- src/settings.c | 82 ++++++++++++++++++++++++++------------------------ 1 file changed, 43 insertions(+), 39 deletions(-) diff --git a/src/settings.c b/src/settings.c index de159cc08..3330bb2b0 100644 --- a/src/settings.c +++ b/src/settings.c @@ -44,30 +44,24 @@ */ #define XDG_CONFIG_DIRS_DEFAULT SYSCONFDIR -/** @brief Convenience macro to not have to put %NULL as the last argument. - * - * @returns a newly-allocated string that must be freed with g_free(). - */ -#define G_STRCONCAT(...) g_strconcat(__VA_ARGS__, NULL) - /** @brief Generate path to base config 'dunstrc' in a base directory * - * @returns a newly-allocated string that must be freed with g_free(). + * @returns a newly-allocated gchar* string that must be freed with g_free(). */ -#define G_BASE_RC(basedir) g_build_filename(basedir, "dunst", "dunstrc", NULL) +#define BASE_RC(basedir) g_build_filename(basedir, "dunst", "dunstrc", NULL) /** @brief Generate drop-in directory path for a base directory * - * @returns a newly-allocated string that must be freed with g_free(). + * @returns a newly-allocated gchar* string that must be freed with g_free(). */ -#define G_DROP_IN_DIR(basedir) G_STRCONCAT(G_BASE_RC(basedir), ".d") +#define DROP_IN_DIR(basedir) g_strconcat(BASE_RC(basedir), ".d", NULL) /** @brief Match pattern for drop-in file names */ #define DROP_IN_PATTERN "*.conf" struct settings settings; -static const char * const *get_xdg_basedirs(void); +static const char * const *get_xdg_conf_basedirs(void); static int is_drop_in(const struct dirent *dent); @@ -82,38 +76,41 @@ static void get_conf_files(GQueue *config_files); * * @param dent [in] @brief directory entry */ -static int is_drop_in(const struct dirent *dent) -{ +static int is_drop_in(const struct dirent *dent) { return 0 == fnmatch(DROP_IN_PATTERN, dent->d_name, FNM_PATHNAME | FNM_PERIOD) ? 1 // success : 0; } -/** - * Returns a %NULL-terminated array of all XDG Base Directories, @e most @e - * important @e first. +/** @brief Get all relevant config base directories + * + * Returns an array of all XDG config base directories, @e most @e important @e + * first. * - * @returns The array of paths to XDG base directories in @e descending order of - * importance + * @returns A %NULL-terminated array of gchar* strings representing the paths + * of all XDG base directories in @e descending order of importance. + * + * The result @e must @e not be freed! The array is cached in a static variable, + * so it is OK to call this again instead of caching its return value. */ -static const char * const *get_xdg_basedirs() { +static const char * const *get_xdg_conf_basedirs() { static const char * const *xdg_bd_arr = NULL; if (!xdg_bd_arr) { char * xdg_basedirs; - char * const xcd_env = getenv("XDG_CONFIG_DIRS"); - char * const xdg_config_dirs = xcd_env && strnlen(xcd_env, 1) - ? xcd_env - : XDG_CONFIG_DIRS_DEFAULT; + const char * const xcd_env = getenv("XDG_CONFIG_DIRS"); + const char * const xdg_config_dirs = xcd_env && strnlen(xcd_env, 1) + ? xcd_env + : XDG_CONFIG_DIRS_DEFAULT; /* * Prepend XDG_CONFIG_HOME, most important first because * XDG_CONFIG_DIRS is already ordered that way. */ - xdg_basedirs = G_STRCONCAT(g_get_user_config_dir(), + xdg_basedirs = g_strconcat(g_get_user_config_dir(), ":", - xdg_config_dirs); - free(xdg_config_dirs); + xdg_config_dirs, + NULL); LOG_D("Config directories: '%s'", xdg_basedirs); xdg_bd_arr = (const char * const *) string_to_array(xdg_basedirs, ":"); @@ -124,28 +121,36 @@ static const char * const *get_xdg_basedirs() { /** @brief Find all config files. * - * Searches all XDG base directories for config files and drop-ins and push - * them onto a LIFO/stack, @e least important last. + * Searches all XDG config base directories for config files and drop-ins and + * puts them in a GQueue, @e least important last. + * + * @param config_files [in|out] A pointer to a GQueue of gchar* strings + * representing config file paths * - * @param config_files [in|out] A pointer to a GQueue of strings representing - * config file paths + * Use g_queue_pop_tail() to retrieve paths in @e ascending order of + * importance, or use g_queue_reverse() before iterating over it with + * g_queue_for_each(). * - * Use g_queue_pop_tail() to retrieve paths in @e ascending order of importance + * Use g_free() to free the retrieved elements of the queue. * - * Use g_queue_free() to free. + * Use g_queue_free() to free after emptying it or g_queue_free_full() for a + * non-empty queue. */ static void get_conf_files(GQueue *config_files) { struct dirent **drop_ins; - for (const char * const *d = get_xdg_basedirs(); *d; d++) { - gchar * const base_rc = G_BASE_RC(*d); - gchar * const drop_in_dir = G_DROP_IN_DIR(*d); + for (const char * const *d = get_xdg_conf_basedirs(); *d; d++) { + /* absolute path to the base rc-file */ + gchar * const base_rc = BASE_RC(*d); + /* absolute path to the corresponding drop-in directory */ + gchar * const drop_in_dir = DROP_IN_DIR(*d); int n = scandir(drop_in_dir, &drop_ins, is_drop_in, alphasort); /* reverse order to get most important first */ while (n-- > 0) { - gchar * const drop_in = G_STRCONCAT(drop_in_dir, + gchar * const drop_in = g_strconcat(drop_in_dir, "/", - drop_ins[n]->d_name); + drop_ins[n]->d_name, + NULL); free(drop_ins[n]); if (is_readable_file(drop_in)) { @@ -154,6 +159,7 @@ static void get_conf_files(GQueue *config_files) { } else g_free(drop_in); } + g_free(drop_in_dir); /* base rc-file last, least important */ if (is_readable_file(base_rc)) { @@ -161,8 +167,6 @@ static void get_conf_files(GQueue *config_files) { g_queue_push_tail(config_files, base_rc); } else g_free(base_rc); - - g_free(drop_in_dir); } free(drop_ins); } From 2cb78ea7840084282d7ab5630259c6f1dab00fb5 Mon Sep 17 00:00:00 2001 From: WhitePeter Date: Thu, 28 Oct 2021 19:01:35 +0200 Subject: [PATCH 9/9] settings: Refactor `load_settings()` Also fixed a double free issue with the provided path parameter. Removed leftover prototype of fopen_conf(). --- src/settings.c | 67 ++++++++++++++++++++++++++++---------------------- src/settings.h | 13 +--------- 2 files changed, 38 insertions(+), 42 deletions(-) diff --git a/src/settings.c b/src/settings.c index 3330bb2b0..80642b2a9 100644 --- a/src/settings.c +++ b/src/settings.c @@ -260,45 +260,52 @@ void check_and_correct_settings(struct settings *s) { } -void load_settings(char *path) { +static void process_conf_file(const gpointer conf_fname, gpointer n_success) { + const gchar * const p = conf_fname; + + LOG_D("Trying to open '%s'", p); + /* Check for "-" here, so the file handling stays in one place */ + FILE *f = STR_EQ(p, "-") ? stdin : fopen_verbose(p); + if (!f) + return; + + LOG_I("Parsing config, fd: '%d'", fileno(f)); + struct ini *ini = load_ini_file(f); + LOG_D("Closing config, fd: '%d'", fileno(f)); + fclose(f); + + LOG_D("Loading settings"); + save_settings(ini); + + LOG_D("Checking/correcting settings"); + check_and_correct_settings(&settings); + + finish_ini(ini); + free(ini); + + ++(*(int *) n_success); +} + +void load_settings(const char * const path) { settings_init(); LOG_D("Setting defaults"); set_defaults(); - GQueue *config_files = g_queue_new(); + GQueue *conf_files = g_queue_new(); if (path) /** If @p path [in] was supplied it will be the only one tried. */ - g_queue_push_tail(config_files, path); + g_queue_push_tail(conf_files, g_strdup(path)); else - get_conf_files(config_files); + get_conf_files(conf_files); + + /* Load all conf files and drop-ins, least important first. */ + g_queue_reverse(conf_files); /* so we can iterate from least to most important */ + int n_loaded_confs = 0; + g_queue_foreach(conf_files, process_conf_file, &n_loaded_confs); + g_queue_free_full(conf_files, g_free); - if (g_queue_is_empty(config_files)) + if (0 == n_loaded_confs) LOG_W("No configuration file, using defaults"); - else { /* Load all conf files and drop-ins, least important first. */ - gchar *path; - while ((path = g_queue_pop_tail(config_files))) { - LOG_D("Trying to open '%s'", path); - FILE *f; - /* Check for "-" here, so the file handling stays in one place */ - if (!(f = STR_EQ(path, "-") ? stdin : fopen_verbose(path))) - continue; - - LOG_I("Parsing config, fd: '%d'", fileno(f)); - struct ini *ini = load_ini_file(f); - LOG_D("Closing config, fd: '%d'", fileno(f)); - fclose(f); - - LOG_D("Loading settings"); - save_settings(ini); - - LOG_D("Checking/correcting settings"); - check_and_correct_settings(&settings); - - finish_ini(ini); - free(ini); - } - } - g_queue_free(config_files); for (GSList *iter = rules; iter; iter = iter->next) { struct rule *r = iter->data; diff --git a/src/settings.h b/src/settings.h index a81d07bed..d23585483 100644 --- a/src/settings.h +++ b/src/settings.h @@ -161,18 +161,7 @@ struct settings { extern struct settings settings; -void load_settings(char *cmdline_config_path); - -/** \brief Open files verbosely. - * - * This is a wrapper around fopen() and fexists() that does some preliminary - * checks and sends log messages. - * - * @returns The result of the fopen() call. - * @retval NULL if the fopen() call failed or the file does not exist, see - * also fexists(). - */ -FILE * fopen_conf(char * const path); +void load_settings(const char * const path); #endif /* vim: set ft=c tabstop=8 shiftwidth=8 expandtab textwidth=0: */