Github user mike-jumper commented on a diff in the pull request:
https://github.com/apache/guacamole-server/pull/205#discussion_r236555556
--- Diff: src/guacd/conf-file.c ---
@@ -188,15 +188,21 @@ guacd_config* guacd_conf_load() {
conf->key_file = NULL;
#endif
+ /* Determine path of configuration file */
+ if(conf_file_path == NULL) {
--- End diff --
> Why do this particular check, here? Why not always make sure this
function is fed a path, and then upstream feed either the provided location or
the constant `GUACD_CONF_FILE`?
I agree with this. This doesn't seem the place for a `NULL` check and
additional semantics. It would be far better if the default value for the
config file path were handled like other defaults.
I'd argue that it's the current handling of defaults that should be
restructured - bringing that out of `guacd_conf_load()` such that the process
becomes:
1. Populate `guacd_config` with defaults.
2. Apply any settings from config file to that `guacd_config`, overriding
default values.
3. Apply any settings from command line to that `guacd_config`, overriding
default values and those from the config file.
See:
https://github.com/apache/guacamole-server/blob/bbb6afaf462f6930a589f962302806c52f350c0b/src/guacd/conf-file.c#L178-L184
---
