Another batch of patches:
* Put the filter chain parsing function in its own module, filter_config
* Fixed a couple of memory leaks in the error handlers of route filter
* Got rid of loud g_error() and made filter_config report using GError
* Minor fix to error reporting of filter_configured_new, rewrote filter_config
to use it.
--
Albin Eldstål-Damlin
>From dddeb2b3124db2a72a0b230a7ac8378715999258 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Albin=20Eldst=C3=A5l-Damlin?= <laeder.k...@gmail.com>
Date: Mon, 14 Dec 2009 21:09:28 +0100
Subject: [PATCH 1/4] Split filter_config into its own module
---
Makefile.am | 2 +
src/filter/chain_filter_plugin.c | 97 -----------------------------
src/filter/chain_filter_plugin.h | 12 ----
src/filter_config.c | 125 ++++++++++++++++++++++++++++++++++++++
src/filter_config.h | 46 ++++++++++++++
src/output_init.c | 1 +
6 files changed, 174 insertions(+), 109 deletions(-)
create mode 100644 src/filter_config.c
create mode 100644 src/filter_config.h
diff --git a/Makefile.am b/Makefile.am
index a55a5e1..cfed9f2 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -47,6 +47,7 @@ mpd_headers = \
src/output_print.h \
src/output_command.h \
src/filter_internal.h \
+ src/filter_config.h \
src/filter_plugin.h \
src/filter_registry.h \
src/filter/chain_filter_plugin.h \
@@ -230,6 +231,7 @@ src_mpd_SOURCES = \
src/exclude.c \
src/fd_util.c \
src/fifo_buffer.c \
+ src/filter_config.c \
src/filter_plugin.c \
src/filter_registry.c \
src/update.c \
diff --git a/src/filter/chain_filter_plugin.c b/src/filter/chain_filter_plugin.c
index 5f9544b..48100bc 100644
--- a/src/filter/chain_filter_plugin.c
+++ b/src/filter/chain_filter_plugin.c
@@ -25,7 +25,6 @@
#include "filter_registry.h"
#include <assert.h>
-#include <string.h>
struct filter_chain {
/** the base class */
@@ -179,99 +178,3 @@ filter_chain_append(struct filter *_chain, struct filter
*filter)
chain->children = g_slist_append(chain->children, filter);
}
-/**
- * Find the "filter" configuration block for the specified name.
- *
- * @param filter_template_name the name of the filter template
- * @return the configuration block, or NULL if none was configured
- */
-static const struct config_param *
-filter_plugin_config(const char *filter_template_name)
-{
- const struct config_param *param = NULL;
-
- while ((param = config_get_next_param(CONF_AUDIO_FILTER, param)) !=
NULL) {
- const char *name =
- config_get_block_string(param, "name", NULL);
- if (name == NULL)
- g_error("filter configuration without 'name' name in
line %d",
- param->line);
-
- if (strcmp(name, filter_template_name) == 0)
- return param;
- }
-
- return NULL;
-}
-
-/**
- * Builds a filter chain from a configuration string on the form
- * "name1, name2, name3, ..." by looking up each name among the
- * configured filter sections.
- * @param chain the chain to append filters on
- * @param spec the filter chain specification
- * @return the number of filters which were successfully added
- */
-unsigned int
-filter_chain_parse(struct filter *chain, const char *spec) {
-
- // Split on comma
- gchar** tokens = g_strsplit_set(spec, ",", 255);
-
- int added_filters = 0;
-
- // Add each name to the filter chain by instantiating an actual filter
- char **template_names = tokens;
- while (*template_names != NULL) {
- const char *plugin_name;
- const struct filter_plugin *fp;
- struct filter *f;
- const struct config_param *cfg;
-
- // Squeeze whitespace
- g_strstrip(*template_names);
-
- cfg = filter_plugin_config(*template_names);
- if (cfg == NULL) {
- g_error("Unable to locate filter template %s",
- *template_names);
- ++template_names;
- continue;
- }
-
- // Figure out what kind of a plugin that is
- plugin_name = config_get_block_string(cfg, "plugin", NULL);
- if (plugin_name == NULL) {
- g_error("filter configuration without 'plugin' at line
%d",
- cfg->line);
- ++template_names;
- continue;
- }
-
- // Instantiate one of those filter plugins with the template
name as a hint
- fp = filter_plugin_by_name(plugin_name);
- if (fp == NULL) {
- g_error("filter plugin not found: %s",
- plugin_name);
- ++template_names;
- continue;
- }
-
- f = filter_new(fp, cfg, NULL);
- if (f == NULL) {
- g_error("filter plugin initialization failed: %s",
- plugin_name);
- ++template_names;
- continue;
- }
-
- filter_chain_append(chain, f);
- ++added_filters;
-
- ++template_names;
- }
-
- g_strfreev(tokens);
-
- return added_filters;
-}
diff --git a/src/filter/chain_filter_plugin.h b/src/filter/chain_filter_plugin.h
index 54e0708..f8462b2 100644
--- a/src/filter/chain_filter_plugin.h
+++ b/src/filter/chain_filter_plugin.h
@@ -45,16 +45,4 @@ filter_chain_new(void);
void
filter_chain_append(struct filter *chain, struct filter *filter);
-/**
- * Builds a filter chain from a configuration string on the form
- * "name1, name2, name3, ..." by looking up each name among the
- * configured filter sections. If no filters are specified, a
- * null filter is automatically appended.
- * @param chain the chain to append filters on
- * @param spec the filter chain specification
- * @return the number of filters which were successfully added
- */
-unsigned int
-filter_chain_parse(struct filter *chain, const char *spec);
-
#endif
diff --git a/src/filter_config.c b/src/filter_config.c
new file mode 100644
index 0000000..1e20fa0
--- /dev/null
+++ b/src/filter_config.c
@@ -0,0 +1,125 @@
+/*
+ * Copyright (C) 2003-2009 The Music Player Daemon Project
+ * http://www.musicpd.org
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License along
+ * with this program; if not, write to the Free Software Foundation, Inc.,
+ * 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA.
+ */
+
+#include "config.h"
+#include "conf.h"
+#include "filter/chain_filter_plugin.h"
+#include "filter_plugin.h"
+#include "filter_internal.h"
+#include "filter_registry.h"
+
+#include <string.h>
+
+/**
+ * Find the "filter" configuration block for the specified name.
+ *
+ * @param filter_template_name the name of the filter template
+ * @return the configuration block, or NULL if none was configured
+ */
+static const struct config_param *
+filter_plugin_config(const char *filter_template_name)
+{
+ const struct config_param *param = NULL;
+
+ while ((param = config_get_next_param(CONF_AUDIO_FILTER, param)) !=
NULL) {
+ const char *name =
+ config_get_block_string(param, "name", NULL);
+ if (name == NULL)
+ g_error("filter configuration without 'name' name in
line %d",
+ param->line);
+
+ if (strcmp(name, filter_template_name) == 0)
+ return param;
+ }
+
+ return NULL;
+}
+
+/**
+ * Builds a filter chain from a configuration string on the form
+ * "name1, name2, name3, ..." by looking up each name among the
+ * configured filter sections.
+ * @param chain the chain to append filters on
+ * @param spec the filter chain specification
+ * @return the number of filters which were successfully added
+ */
+unsigned int
+filter_chain_parse(struct filter *chain, const char *spec)
+{
+
+ // Split on comma
+ gchar** tokens = g_strsplit_set(spec, ",", 255);
+
+ int added_filters = 0;
+
+ // Add each name to the filter chain by instantiating an actual filter
+ char **template_names = tokens;
+ while (*template_names != NULL) {
+ const char *plugin_name;
+ const struct filter_plugin *fp;
+ struct filter *f;
+ const struct config_param *cfg;
+
+ // Squeeze whitespace
+ g_strstrip(*template_names);
+
+ cfg = filter_plugin_config(*template_names);
+ if (cfg == NULL) {
+ g_error("Unable to locate filter template %s",
+ *template_names);
+ ++template_names;
+ continue;
+ }
+
+ // Figure out what kind of a plugin that is
+ plugin_name = config_get_block_string(cfg, "plugin", NULL);
+ if (plugin_name == NULL) {
+ g_error("filter configuration without 'plugin' at line
%d",
+ cfg->line);
+ ++template_names;
+ continue;
+ }
+
+ // Instantiate one of those filter plugins with the template
name as a hint
+ fp = filter_plugin_by_name(plugin_name);
+ if (fp == NULL) {
+ g_error("filter plugin not found: %s",
+ plugin_name);
+ ++template_names;
+ continue;
+ }
+
+ f = filter_new(fp, cfg, NULL);
+ if (f == NULL) {
+ g_error("filter plugin initialization failed: %s",
+ plugin_name);
+ ++template_names;
+ continue;
+ }
+
+ filter_chain_append(chain, f);
+ ++added_filters;
+
+ ++template_names;
+ }
+
+ g_strfreev(tokens);
+
+ return added_filters;
+}
diff --git a/src/filter_config.h b/src/filter_config.h
new file mode 100644
index 0000000..eadaf24
--- /dev/null
+++ b/src/filter_config.h
@@ -0,0 +1,46 @@
+/*
+ * Copyright (C) 2003-2009 The Music Player Daemon Project
+ * http://www.musicpd.org
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License along
+ * with this program; if not, write to the Free Software Foundation, Inc.,
+ * 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA.
+ */
+
+/** \file
+ *
+ * Utility functions for filter configuration
+ */
+
+#ifndef MPD_FILTER_CONFIG_H
+#define MPD_FILTER_CONFIG_H
+
+#include "conf.h"
+#include "filter/chain_filter_plugin.h"
+#include "filter_plugin.h"
+#include "filter_internal.h"
+#include "filter_registry.h"
+
+
+/**
+ * Builds a filter chain from a configuration string on the form
+ * "name1, name2, name3, ..." by looking up each name among the
+ * configured filter sections.
+ * @param chain the chain to append filters on
+ * @param spec the filter chain specification
+ * @return the number of filters which were successfully added
+ */
+unsigned int
+filter_chain_parse(struct filter *chain, const char *spec);
+
+#endif
diff --git a/src/output_init.c b/src/output_init.c
index 6ca190f..f4025fb 100644
--- a/src/output_init.c
+++ b/src/output_init.c
@@ -29,6 +29,7 @@
#include "mixer/software_mixer_plugin.h"
#include "filter_plugin.h"
#include "filter_registry.h"
+#include "filter_config.h"
#include "filter/chain_filter_plugin.h"
#include <glib.h>
--
1.6.4.4
>From 0591b168dcef71099742dd781b8730d8bca137cd Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Albin=20Eldst=C3=A5l-Damlin?= <laeder.k...@gmail.com>
Date: Mon, 14 Dec 2009 21:37:13 +0100
Subject: [PATCH 2/4] Fixed memory leak on incorrect route configuration
---
src/filter/route_filter_plugin.c | 4 ++++
1 files changed, 4 insertions(+), 0 deletions(-)
diff --git a/src/filter/route_filter_plugin.c b/src/filter/route_filter_plugin.c
index 9ec30e6..5610220 100644
--- a/src/filter/route_filter_plugin.c
+++ b/src/filter/route_filter_plugin.c
@@ -159,6 +159,8 @@ route_filter_parse(const struct config_param *param,
g_set_error(error_r, config_quark(), 1,
"Invalid copy around %d in routes spec: %s",
param->line, tokens[c]);
+ g_strfreev(sd);
+ g_strfreev(tokens);
return;
}
@@ -196,6 +198,8 @@ route_filter_parse(const struct config_param *param,
g_set_error(error_r, config_quark(), 1,
"Invalid copy around %d in routes spec: %s",
param->line, tokens[c]);
+ g_strfreev(sd);
+ g_strfreev(tokens);
return;
}
--
1.6.4.4
>From 081187e91022d0e2c256d50480786c6d11c405e4 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Albin=20Eldst=C3=A5l-Damlin?= <laeder.k...@gmail.com>
Date: Mon, 14 Dec 2009 21:46:45 +0100
Subject: [PATCH 3/4] Proper error reporting from filter_config
---
src/filter_config.c | 49 +++++++++++++++++++++++++++++++------------------
src/filter_config.h | 3 ++-
src/output_init.c | 11 ++++++++++-
3 files changed, 43 insertions(+), 20 deletions(-)
diff --git a/src/filter_config.c b/src/filter_config.c
index 1e20fa0..3b3bf17 100644
--- a/src/filter_config.c
+++ b/src/filter_config.c
@@ -19,6 +19,7 @@
#include "config.h"
#include "conf.h"
+#include "filter_config.h"
#include "filter/chain_filter_plugin.h"
#include "filter_plugin.h"
#include "filter_internal.h"
@@ -26,14 +27,22 @@
#include <string.h>
+
+static GQuark
+filter_quark(void)
+{
+ return g_quark_from_static_string("filter");
+}
+
/**
* Find the "filter" configuration block for the specified name.
*
* @param filter_template_name the name of the filter template
+ * @param error_r space to return an error description
* @return the configuration block, or NULL if none was configured
*/
static const struct config_param *
-filter_plugin_config(const char *filter_template_name)
+filter_plugin_config(const char *filter_template_name, GError **error_r)
{
const struct config_param *param = NULL;
@@ -41,13 +50,18 @@ filter_plugin_config(const char *filter_template_name)
const char *name =
config_get_block_string(param, "name", NULL);
if (name == NULL)
- g_error("filter configuration without 'name' name in
line %d",
- param->line);
+ g_set_error(error_r, filter_quark(), 1,
+ "filter configuration without 'name' name
in line %d",
+ param->line);
if (strcmp(name, filter_template_name) == 0)
return param;
}
+ g_set_error(error_r, filter_quark(), 1,
+ "filter template not found: %s",
+ filter_template_name);
+
return NULL;
}
@@ -57,10 +71,11 @@ filter_plugin_config(const char *filter_template_name)
* configured filter sections.
* @param chain the chain to append filters on
* @param spec the filter chain specification
+ * @param error_r space to return an error description
* @return the number of filters which were successfully added
*/
unsigned int
-filter_chain_parse(struct filter *chain, const char *spec)
+filter_chain_parse(struct filter *chain, const char *spec, GError **error_r)
{
// Split on comma
@@ -79,38 +94,36 @@ filter_chain_parse(struct filter *chain, const char *spec)
// Squeeze whitespace
g_strstrip(*template_names);
- cfg = filter_plugin_config(*template_names);
+ cfg = filter_plugin_config(*template_names, error_r);
if (cfg == NULL) {
- g_error("Unable to locate filter template %s",
- *template_names);
- ++template_names;
- continue;
+ // The error has already been set, just stop.
+ break;
}
// Figure out what kind of a plugin that is
plugin_name = config_get_block_string(cfg, "plugin", NULL);
if (plugin_name == NULL) {
- g_error("filter configuration without 'plugin' at line
%d",
+ g_set_error(error_r, filter_quark(), 1,
+ "filter configuration without 'plugin' at
line %d",
cfg->line);
- ++template_names;
- continue;
+ break;
}
// Instantiate one of those filter plugins with the template
name as a hint
fp = filter_plugin_by_name(plugin_name);
if (fp == NULL) {
- g_error("filter plugin not found: %s",
+ g_set_error(error_r, filter_quark(), 2,
+ "filter plugin not found: %s",
plugin_name);
- ++template_names;
- continue;
+ break;
}
f = filter_new(fp, cfg, NULL);
if (f == NULL) {
- g_error("filter plugin initialization failed: %s",
+ g_set_error(error_r, filter_quark(), 3,
+ "filter plugin initialization failed: %s",
plugin_name);
- ++template_names;
- continue;
+ break;
}
filter_chain_append(chain, f);
diff --git a/src/filter_config.h b/src/filter_config.h
index eadaf24..8e20320 100644
--- a/src/filter_config.h
+++ b/src/filter_config.h
@@ -38,9 +38,10 @@
* configured filter sections.
* @param chain the chain to append filters on
* @param spec the filter chain specification
+ * @param error_r space to return an error description
* @return the number of filters which were successfully added
*/
unsigned int
-filter_chain_parse(struct filter *chain, const char *spec);
+filter_chain_parse(struct filter *chain, const char *spec, GError **error_r);
#endif
diff --git a/src/output_init.c b/src/output_init.c
index f4025fb..ab52578 100644
--- a/src/output_init.c
+++ b/src/output_init.c
@@ -193,9 +193,18 @@ audio_output_init(struct audio_output *ao, const struct
config_param *param,
ao->filter = filter_chain_new();
assert(ao->filter != NULL);
filter_chain_parse(ao->filter,
- config_get_block_string(param, AUDIO_FILTERS, "")
+ config_get_block_string(param, AUDIO_FILTERS, ""),
+ &error
);
+ // It's not really fatal - Part of the filter chain has been set up
already
+ // and even an empty one will work (if only with unexpected behaviour)
+ if (error != NULL) {
+ g_warning("Failed to initialize filter chain for '%s': %s",
+ ao->name, error->message);
+ g_error_free(error);
+ }
+
ao->thread = NULL;
ao->command = AO_COMMAND_NONE;
ao->mutex = g_mutex_new();
--
1.6.4.4
>From 3c44d91237d2326ccff8e02d57d740c06187987c Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Albin=20Eldst=C3=A5l-Damlin?= <laeder.k...@gmail.com>
Date: Mon, 14 Dec 2009 21:59:32 +0100
Subject: [PATCH 4/4] Fix and use filter_configured_new()
---
src/filter_config.c | 26 +++-----------------------
src/filter_plugin.c | 8 ++++++--
2 files changed, 9 insertions(+), 25 deletions(-)
diff --git a/src/filter_config.c b/src/filter_config.c
index 3b3bf17..2473baf 100644
--- a/src/filter_config.c
+++ b/src/filter_config.c
@@ -86,8 +86,6 @@ filter_chain_parse(struct filter *chain, const char *spec,
GError **error_r)
// Add each name to the filter chain by instantiating an actual filter
char **template_names = tokens;
while (*template_names != NULL) {
- const char *plugin_name;
- const struct filter_plugin *fp;
struct filter *f;
const struct config_param *cfg;
@@ -100,29 +98,10 @@ filter_chain_parse(struct filter *chain, const char *spec,
GError **error_r)
break;
}
- // Figure out what kind of a plugin that is
- plugin_name = config_get_block_string(cfg, "plugin", NULL);
- if (plugin_name == NULL) {
- g_set_error(error_r, filter_quark(), 1,
- "filter configuration without 'plugin' at
line %d",
- cfg->line);
- break;
- }
-
// Instantiate one of those filter plugins with the template
name as a hint
- fp = filter_plugin_by_name(plugin_name);
- if (fp == NULL) {
- g_set_error(error_r, filter_quark(), 2,
- "filter plugin not found: %s",
- plugin_name);
- break;
- }
-
- f = filter_new(fp, cfg, NULL);
+ f = filter_configured_new(cfg, error_r);
if (f == NULL) {
- g_set_error(error_r, filter_quark(), 3,
- "filter plugin initialization failed: %s",
- plugin_name);
+ // The error has already been set, just stop.
break;
}
@@ -136,3 +115,4 @@ filter_chain_parse(struct filter *chain, const char *spec,
GError **error_r)
return added_filters;
}
+
diff --git a/src/filter_plugin.c b/src/filter_plugin.c
index 4567726..ecc4b54 100644
--- a/src/filter_plugin.c
+++ b/src/filter_plugin.c
@@ -49,14 +49,18 @@ filter_configured_new(const struct config_param *param,
GError **error_r)
assert(error_r == NULL || *error_r == NULL);
plugin_name = config_get_block_string(param, "plugin", NULL);
- if (plugin_name == NULL)
+ if (plugin_name == NULL) {
g_set_error(error_r, config_quark(), 0,
"No filter plugin specified");
+ return NULL;
+ }
plugin = filter_plugin_by_name(plugin_name);
- if (plugin == NULL)
+ if (plugin == NULL) {
g_set_error(error_r, config_quark(), 0,
"No such filter plugin: %s", plugin_name);
+ return NULL;
+ }
return filter_new(plugin, param, error_r);
}
--
1.6.4.4
------------------------------------------------------------------------------
Return on Information:
Google Enterprise Search pays you back
Get the facts.
http://p.sf.net/sfu/google-dev2dev
_______________________________________________
Musicpd-dev-team mailing list
Musicpd-dev-team@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/musicpd-dev-team
