conf: turn config_get_path() into config_dup_path()

config_get_path() was somewhat flawed, because it pretended to be a
function, when it really had a side effect.  The second flaw was that
it did not return the parser error, instead it aborted the whole
process, which is bad style.  The new function returns a duplicated
(modified) string that must be freed by the caller, and returns a
GError on failure.

4 files changed, 109 insertions, 47 deletions

diff --git a/src/conf.c b/src/conf.c
index 4434a875..b3b684aa 100644
--- a/src/conf.c
+++ b/src/conf.c
@@ -503,22 +503,23 @@ config_get_string(const char *name, const char *default_value)
 	return param->value;
 }
 
-const char *
-config_get_path(const char *name)
+char *
+config_dup_path(const char *name, GError **error_r)
 {
-	struct config_param *param = config_get_param(name);
-	char *path;
+	assert(error_r != NULL);
+	assert(*error_r == NULL);
 
+	const struct config_param *param = config_get_param(name);
 	if (param == NULL)
 		return NULL;
 
-	path = parsePath(param->value);
-	if (path == NULL)
-		MPD_ERROR("error parsing \"%s\" at line %i\n",
-			  name, param->line);
+	char *path = parsePath(param->value);
+	if (G_UNLIKELY(path == NULL))
+		g_set_error(error_r, config_quark(), 0,
+			    "Invalid path in \"%s\" at line %i",
+			    name, param->line);
 
-	g_free(param->value);
-	return param->value = path;
+	return path;
 }
 
 unsigned
diff --git a/src/conf.h b/src/conf.h
index 8aad9839..9593501b 100644
--- a/src/conf.h
+++ b/src/conf.h
@@ -156,17 +156,15 @@ config_get_string(const char *name, const char *default_value);
 
 /**
  * Returns an optional configuration variable which contains an
- * absolute path.  If there is a tilde prefix, it is expanded.  Aborts
- * MPD if the path is not a valid absolute path.
+ * absolute path.  If there is a tilde prefix, it is expanded.
+ * Returns NULL if the value is not present.  If the path could not be
+ * parsed, returns NULL and sets the error.
+ *
+ * The return value must be freed with g_free().
  */
-/* We lie here really.  This function is not pure as it has side
-   effects -- it parse the value and creates new string freeing
-   previous one.  However, because this works the very same way each
-   time (ie. from the outside it appears as if function had no side
-   effects) we should be in the clear declaring it pure. */
-G_GNUC_PURE
-const char *
-config_get_path(const char *name);
+G_GNUC_MALLOC
+char *
+config_dup_path(const char *name, GError **error_r);
 
 G_GNUC_PURE
 unsigned
diff --git a/src/log.c b/src/log.c
index 140b50a2..86dd86ea 100644
--- a/src/log.c
+++ b/src/log.c
@@ -271,10 +271,18 @@ log_init(bool verbose, bool use_stdout, GError **error_r)
 			return true;
 #endif
 		} else {
-			const char *path = config_get_path(CONF_LOG_FILE);
-			assert(path != NULL);
-
-			return log_init_file(path, param->line, error_r);
+			GError *error = NULL;
+			char *path = config_dup_path(CONF_LOG_FILE, &error);
+			if (path == NULL) {
+				assert(error != NULL);
+				g_propagate_error(error_r, error);
+				return false;
+			}
+
+			bool success = log_init_file(path, param->line,
+						     error_r);
+			g_free(path);
+			return success;
 		}
 	}
 }
diff --git a/src/main.c b/src/main.c
index a80b4e17..a57a19a8 100644
--- a/src/main.c
+++ b/src/main.c
@@ -98,31 +98,54 @@ GCond *main_cond;
 
 struct player_control *global_player_control;
 
-static void
-glue_daemonize_init(const struct options *options)
+static bool
+glue_daemonize_init(const struct options *options, GError **error_r)
 {
+	GError *error = NULL;
+
+	char *pid_file = config_dup_path(CONF_PID_FILE, &error);
+	if (pid_file == NULL && error != NULL) {
+		g_propagate_error(error_r, error);
+		return false;
+	}
+
 	daemonize_init(config_get_string(CONF_USER, NULL),
 		       config_get_string(CONF_GROUP, NULL),
-		       config_get_path(CONF_PID_FILE));
+		       pid_file);
+	g_free(pid_file);
 
 	if (options->kill)
 		daemonize_kill();
+
+	return true;
 }
 
-static void
-glue_mapper_init(void)
+static bool
+glue_mapper_init(GError **error_r)
 {
-	const char *music_dir, *playlist_dir;
+	GError *error = NULL;
+	char *music_dir = config_dup_path(CONF_MUSIC_DIR, &error);
+	if (music_dir == NULL && error != NULL) {
+		g_propagate_error(error_r, error);
+		return false;
+	}
+
+	char *playlist_dir = config_dup_path(CONF_PLAYLIST_DIR, &error);
+	if (playlist_dir == NULL && error != NULL) {
+		g_propagate_error(error_r, error);
+		return false;
+	}
 
-	music_dir = config_get_path(CONF_MUSIC_DIR);
 #if GLIB_CHECK_VERSION(2,14,0)
 	if (music_dir == NULL)
-		music_dir = g_get_user_special_dir(G_USER_DIRECTORY_MUSIC);
+		music_dir = g_strdup(g_get_user_special_dir(G_USER_DIRECTORY_MUSIC));
 #endif
 
-	playlist_dir = config_get_path(CONF_PLAYLIST_DIR);
-
 	mapper_init(music_dir, playlist_dir);
+
+	g_free(music_dir);
+	g_free(playlist_dir);
+	return true;
 }
 
 /**
@@ -133,11 +156,15 @@ glue_mapper_init(void)
 static bool
 glue_db_init_and_load(void)
 {
-	const char *path = config_get_path(CONF_DB_FILE);
-	bool ret;
 	GError *error = NULL;
+	char *path = config_dup_path(CONF_DB_FILE, &error);
+	if (path == NULL && error != NULL)
+		MPD_ERROR("%s", error->message);
+
+	bool ret;
 
 	if (!mapper_has_music_directory()) {
+		g_free(path);
 		if (path != NULL)
 			g_message("Found " CONF_DB_FILE " setting without "
 				  CONF_MUSIC_DIR " - disabling database");
@@ -149,6 +176,7 @@ glue_db_init_and_load(void)
 		MPD_ERROR(CONF_DB_FILE " setting missing");
 
 	db_init(path);
+	g_free(path);
 
 	ret = db_load(&error);
 	if (!ret) {
@@ -174,21 +202,34 @@ static void
 glue_sticker_init(void)
 {
 #ifdef ENABLE_SQLITE
-	bool success;
 	GError *error = NULL;
+	char *sticker_file = config_dup_path(CONF_STICKER_FILE, &error);
+	if (sticker_file == NULL && error != NULL)
+		MPD_ERROR("%s", error->message);
 
-	success = sticker_global_init(config_get_path(CONF_STICKER_FILE),
-				      &error);
-	if (!success)
+	if (!sticker_global_init(config_get_string(CONF_STICKER_FILE, NULL),
+				 &error))
 		MPD_ERROR("%s", error->message);
+
+	g_free(sticker_file);
 #endif
 }
 
-static void
-glue_state_file_init(void)
+static bool
+glue_state_file_init(GError **error_r)
 {
-	state_file_init(config_get_path(CONF_STATE_FILE),
-			global_player_control);
+	GError *error = NULL;
+
+	char *path = config_dup_path(CONF_STATE_FILE, &error);
+	if (path == NULL && error != NULL) {
+		g_propagate_error(error_r, error);
+		return false;
+	}
+
+	state_file_init(path, global_player_control);
+	g_free(path);
+
+	return true;
 }
 
 /**
@@ -328,7 +369,11 @@ int mpd_main(int argc, char *argv[])
 		return EXIT_FAILURE;
 	}
 
-	glue_daemonize_init(&options);
+	if (!glue_daemonize_init(&options, &error)) {
+		g_warning("%s", error->message);
+		g_error_free(error);
+		return EXIT_FAILURE;
+	}
 
 	stats_global_init();
 	tag_lib_init();
@@ -357,7 +402,13 @@ int mpd_main(int argc, char *argv[])
 	event_pipe_register(PIPE_EVENT_SHUTDOWN, shutdown_event_emitted);
 
 	path_global_init();
-	glue_mapper_init();
+
+	if (!glue_mapper_init(&error)) {
+		g_warning("%s", error->message);
+		g_error_free(error);
+		return EXIT_FAILURE;
+	}
+
 	initPermissions();
 	playlist_global_init();
 	spl_global_init();
@@ -411,7 +462,11 @@ int mpd_main(int argc, char *argv[])
 			MPD_ERROR("directory update failed");
 	}
 
-	glue_state_file_init();
+	if (!glue_state_file_init(&error)) {
+		g_warning("%s", error->message);
+		g_error_free(error);
+		return EXIT_FAILURE;
+	}
 
 	success = config_get_bool(CONF_AUTO_UPDATE, false);
 #ifdef ENABLE_INOTIFY