[RFC v2 0/2] Fix debug messages logging from non-built-in plugins

[RFC v2 0/2] Fix debug messages logging from non-built-in plugins

[Szymon Janc]

Hi All,

This is a second version, much simpler. Uses dlsym() to locate __debug
section both in external plugin and main binary. Thanks to this we have
common path for all debug messages.

Also added dummy external plugin for test purposes.

Some comments about it:
__start___debug and __stop___debug symbols must be produced explicite
with -usymbol linker flag (see [1]) if not referenced in code directly.


Comments welcome.

[1] http://lists.gnu.org/archive/html/bug-binutils/2007-11/msg00088.html

Szymon Janc (2):
  Fix debug messages logging from non-built-in plugins
  Add external dummy plugin for testing

 Makefile.am              |   12 ++++++++++-
 acinclude.m4             |    6 +++++
 plugins/external-dummy.c |   42 +++++++++++++++++++++++++++++++++++++++
 src/bluetooth.ver        |    2 +
 src/log.c                |   49 ++++++++++++++++++++++++++++++++++++---------
 src/log.h                |    2 +
 src/plugin.c             |    8 +++++-
 7 files changed, 108 insertions(+), 13 deletions(-)
 create mode 100644 plugins/external-dummy.c

-- 
on behalf of ST-Ericsson

[RFC v2 1/2] Fix debug messages logging from non-built-in plugins

[Szymon Janc]

---
 Makefile.am       |    3 ++-
 src/bluetooth.ver |    2 ++
 src/log.c         |   49 +++++++++++++++++++++++++++++++++++++++----------
 src/log.h         |    2 ++
 src/plugin.c      |    8 ++++++--
 5 files changed, 51 insertions(+), 13 deletions(-)

diff --git a/Makefile.am b/Makefile.am
index 68380d9..083ad27 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -285,7 +285,8 @@ src_bluetoothd_SOURCES = $(gdbus_sources) $(builtin_sources) \
 src_bluetoothd_LDADD = lib/libbluetooth.la @GLIB_LIBS@ @DBUS_LIBS@ \
 							@CAPNG_LIBS@ -ldl -lrt
 src_bluetoothd_LDFLAGS = -Wl,--export-dynamic \
-				-Wl,--version-script=$(srcdir)/src/bluetooth.ver
+			 -Wl,--version-script=$(srcdir)/src/bluetooth.ver \
+			 -Wl,-u__start___debug -Wl,-u__stop___debug
 
 src_bluetoothd_DEPENDENCIES = lib/libbluetooth.la
 
diff --git a/src/bluetooth.ver b/src/bluetooth.ver
index b71c70d..6ff0df2 100644
--- a/src/bluetooth.ver
+++ b/src/bluetooth.ver
@@ -5,6 +5,8 @@
 		info;
 		error;
 		debug;
+		__start___debug;
+		__stop___debug;
 	local:
 		*;
 };
diff --git a/src/log.c b/src/log.c
index 2c492e9..9d89f7d 100644
--- a/src/log.c
+++ b/src/log.c
@@ -28,6 +28,7 @@
 #include <stdio.h>
 #include <stdarg.h>
 #include <syslog.h>
+#include <dlfcn.h>
 
 #include <glib.h>
 
@@ -66,10 +67,8 @@ void btd_debug(const char *format, ...)
 	va_end(ap);
 }
 
-extern struct btd_debug_desc __start___debug[];
-extern struct btd_debug_desc __stop___debug[];
-
 static gchar **enabled = NULL;
+static GSList *handles = NULL;
 
 static gboolean is_enabled(struct btd_debug_desc *desc)
 {
@@ -88,23 +87,27 @@ static gboolean is_enabled(struct btd_debug_desc *desc)
 
 void __btd_toggle_debug(void)
 {
-	struct btd_debug_desc *desc;
+	GSList *l;
+
+	for (l = handles; l; l = l->next) {
+		struct btd_debug_desc *start, *stop;
 
-	for (desc = __start___debug; desc < __stop___debug; desc++)
-		desc->flags |= BTD_DEBUG_FLAG_PRINT;
+		start = dlsym(l->data, "__start___debug");
+		stop = dlsym(l->data, "__stop___debug");
+
+		for (; start < stop; start++)
+			start->flags |= BTD_DEBUG_FLAG_PRINT;
+	}
 }
 
 void __btd_log_init(const char *debug, int detach)
 {
 	int option = LOG_NDELAY | LOG_PID;
-	struct btd_debug_desc *desc;
 
 	if (debug != NULL)
 		enabled = g_strsplit_set(debug, ":, ", 0);
 
-	for (desc = __start___debug; desc < __stop___debug; desc++)
-		if (is_enabled(desc))
-			desc->flags |= BTD_DEBUG_FLAG_PRINT;
+	__btd_log_add(NULL);
 
 	if (!detach)
 		option |= LOG_PERROR;
@@ -116,7 +119,33 @@ void __btd_log_init(const char *debug, int detach)
 
 void __btd_log_cleanup(void)
 {
+	__btd_log_remove(NULL);
+
 	closelog();
 
 	g_strfreev(enabled);
 }
+
+void __btd_log_add(void *handle)
+{
+	struct btd_debug_desc *start, *stop;
+
+	start = dlsym(handle, "__start___debug");
+	stop = dlsym(handle, "__stop___debug");
+
+	if (!start || !stop) {
+		DBG("No __debug section found");
+		return;
+	}
+
+	for (; start < stop; start++)
+		if (is_enabled(start))
+			start->flags |= BTD_DEBUG_FLAG_PRINT;
+
+	handles = g_slist_prepend(handles, handle);
+}
+
+void __btd_log_remove(void *handle)
+{
+	handles = g_slist_remove(handles, handle);
+}
diff --git a/src/log.h b/src/log.h
index 78bbdd8..6bb1a96 100644
--- a/src/log.h
+++ b/src/log.h
@@ -29,6 +29,8 @@ void btd_debug(const char *format, ...) __attribute__((format(printf, 1, 2)));
 void __btd_log_init(const char *debug, int detach);
 void __btd_log_cleanup(void);
 void __btd_toggle_debug(void);
+void __btd_log_add(void *handle);
+void __btd_log_remove(void *handle);
 
 struct btd_debug_desc {
 	const char *file;
diff --git a/src/plugin.c b/src/plugin.c
index 3506553..05855de 100644
--- a/src/plugin.c
+++ b/src/plugin.c
@@ -202,7 +202,9 @@ gboolean plugin_init(GKeyFile *config, const char *enable, const char *disable)
 			continue;
 		}
 
-		if (add_plugin(handle, desc) == FALSE)
+		if (add_plugin(handle, desc) == TRUE)
+			__btd_log_add(handle);
+		else
 			dlclose(handle);
 	}
 
@@ -239,8 +241,10 @@ void plugin_cleanup(void)
 		if (plugin->active == TRUE && plugin->desc->exit)
 			plugin->desc->exit();
 
-		if (plugin->handle != NULL)
+		if (plugin->handle != NULL) {
+			__btd_log_remove(plugin->handle);
 			dlclose(plugin->handle);
+		}
 
 		g_free(plugin);
 	}
-- 
on behalf of ST-Ericsson

[RFC v2 2/2] Add external dummy plugin for testing

[Szymon Janc]

---
 Makefile.am              |    9 +++++++++
 acinclude.m4             |    6 ++++++
 plugins/external-dummy.c |   42 ++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 57 insertions(+), 0 deletions(-)
 create mode 100644 plugins/external-dummy.c

diff --git a/Makefile.am b/Makefile.am
index 083ad27..b51f82e 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -48,6 +48,8 @@ endif
 plugindir = $(libdir)/bluetooth/plugins
 
 plugin_LTLIBRARIES =
+plugin_LDFLAGS = -module -avoid-version -Wl,--export-dynamic \
+			-Wl,-u__start___debug -Wl,-u__stop___debug
 
 
 lib_headers = lib/bluetooth.h lib/hci.h lib/hci_lib.h lib/mgmt.h \
@@ -258,6 +260,13 @@ builtin_modules += dbusoob
 builtin_sources += plugins/dbusoob.c
 endif
 
+if EXTERNALDUMMYPLUGIN
+plugin_LTLIBRARIES += plugins/external-dummy.la
+plugins_external_dummy_la_SOURCES = plugins/external-dummy.c
+plugins_external_dummy_la_LDFLAGS = $(plugin_LDFLAGS)
+plugins_external_dummy_la_CFLAGS = -fvisibility=hidden
+endif
+
 sbin_PROGRAMS += src/bluetoothd
 
 src_bluetoothd_SOURCES = $(gdbus_sources) $(builtin_sources) \
diff --git a/acinclude.m4 b/acinclude.m4
index 3cb9459..737c000 100644
--- a/acinclude.m4
+++ b/acinclude.m4
@@ -216,6 +216,7 @@ AC_DEFUN([AC_ARG_BLUEZ], [
 	dbusoob_enable=no
 	wiimote_enable=no
 	thermometer_enable=no
+	external_dummy_enable=no
 
 	AC_ARG_ENABLE(optimization, AC_HELP_STRING([--disable-optimization], [disable code optimization]), [
 		optimization_enable=${enableval}
@@ -364,6 +365,10 @@ AC_DEFUN([AC_ARG_BLUEZ], [
 		thermometer_enable=${enableval}
 	])
 
+	AC_ARG_ENABLE(external_dummy, AC_HELP_STRING([--enable-external-dummy], [enable sample external plugin]), [
+		external_dummy_enable=${enableval}
+	])
+
 	if (test "${fortify_enable}" = "yes"); then
 		CFLAGS="$CFLAGS -D_FORTIFY_SOURCE=2"
 	fi
@@ -421,4 +426,5 @@ AC_DEFUN([AC_ARG_BLUEZ], [
 	AM_CONDITIONAL(DBUSOOBPLUGIN, test "${dbusoob_enable}" = "yes")
 	AM_CONDITIONAL(WIIMOTEPLUGIN, test "${wiimote_enable}" = "yes")
 	AM_CONDITIONAL(THERMOMETERPLUGIN, test "${thermometer_enable}" = "yes")
+	AM_CONDITIONAL(EXTERNALDUMMYPLUGIN, test "${external_dummy_enable}" = "yes")
 ])
diff --git a/plugins/external-dummy.c b/plugins/external-dummy.c
new file mode 100644
index 0000000..ff4608b
--- /dev/null
+++ b/plugins/external-dummy.c
@@ -0,0 +1,42 @@
+/*
+ *
+ *  BlueZ - Bluetooth protocol stack for Linux
+ *
+ *  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 St, Fifth Floor, Boston, MA  02110-1301  USA
+ *
+ */
+
+#ifdef HAVE_CONFIG_H
+#include <config.h>
+#endif
+
+#include "plugin.h"
+#include "log.h"
+
+static int dummy_init(void)
+{
+	DBG("");
+
+	return 0;
+}
+
+static void dummy_exit(void)
+{
+	DBG("");
+}
+
+BLUETOOTH_PLUGIN_DEFINE(external_dummy, VERSION,
+		BLUETOOTH_PLUGIN_PRIORITY_DEFAULT, dummy_init, dummy_exit)
+
-- 
on behalf of ST-Ericsson

Re: [RFC v2 2/2] Add external dummy plugin for testing

[Marcel Holtmann]

Hi Szymon,

>  Makefile.am              |    9 +++++++++
>  acinclude.m4             |    6 ++++++
>  plugins/external-dummy.c |   42 ++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 57 insertions(+), 0 deletions(-)
>  create mode 100644 plugins/external-dummy.c
> 
> diff --git a/Makefile.am b/Makefile.am
> index 083ad27..b51f82e 100644
> --- a/Makefile.am
> +++ b/Makefile.am
> @@ -48,6 +48,8 @@ endif
>  plugindir = $(libdir)/bluetooth/plugins
>  
>  plugin_LTLIBRARIES =
> +plugin_LDFLAGS = -module -avoid-version -Wl,--export-dynamic \
> +			-Wl,-u__start___debug -Wl,-u__stop___debug

this part is a bit ugly. I wish we could automate it.

And what does actually take care of bluetooth_plugin_desc since that is
suppose to be the only exported symbol in the first place.

>  lib_headers = lib/bluetooth.h lib/hci.h lib/hci_lib.h lib/mgmt.h \
> @@ -258,6 +260,13 @@ builtin_modules += dbusoob
>  builtin_sources += plugins/dbusoob.c
>  endif
>  
> +if EXTERNALDUMMYPLUGIN
> +plugin_LTLIBRARIES += plugins/external-dummy.la
> +plugins_external_dummy_la_SOURCES = plugins/external-dummy.c
> +plugins_external_dummy_la_LDFLAGS = $(plugin_LDFLAGS)
> +plugins_external_dummy_la_CFLAGS = -fvisibility=hidden
> +endif
> +
>  sbin_PROGRAMS += src/bluetoothd
>  
>  src_bluetoothd_SOURCES = $(gdbus_sources) $(builtin_sources) \
> diff --git a/acinclude.m4 b/acinclude.m4
> index 3cb9459..737c000 100644
> --- a/acinclude.m4
> +++ b/acinclude.m4
> @@ -216,6 +216,7 @@ AC_DEFUN([AC_ARG_BLUEZ], [
>  	dbusoob_enable=no
>  	wiimote_enable=no
>  	thermometer_enable=no
> +	external_dummy_enable=no
>  
>  	AC_ARG_ENABLE(optimization, AC_HELP_STRING([--disable-optimization], [disable code optimization]), [
>  		optimization_enable=${enableval}
> @@ -364,6 +365,10 @@ AC_DEFUN([AC_ARG_BLUEZ], [
>  		thermometer_enable=${enableval}
>  	])
>  
> +	AC_ARG_ENABLE(external_dummy, AC_HELP_STRING([--enable-external-dummy], [enable sample external plugin]), [
> +		external_dummy_enable=${enableval}
> +	])
> +

Please don't. This project has already too many configure options. Just
use if MAINTAINER_MODE in Makefile.am.

>  	if (test "${fortify_enable}" = "yes"); then
>  		CFLAGS="$CFLAGS -D_FORTIFY_SOURCE=2"
>  	fi
> @@ -421,4 +426,5 @@ AC_DEFUN([AC_ARG_BLUEZ], [
>  	AM_CONDITIONAL(DBUSOOBPLUGIN, test "${dbusoob_enable}" = "yes")
>  	AM_CONDITIONAL(WIIMOTEPLUGIN, test "${wiimote_enable}" = "yes")
>  	AM_CONDITIONAL(THERMOMETERPLUGIN, test "${thermometer_enable}" = "yes")
> +	AM_CONDITIONAL(EXTERNALDUMMYPLUGIN, test "${external_dummy_enable}" = "yes")
>  ])
> diff --git a/plugins/external-dummy.c b/plugins/external-dummy.c
> new file mode 100644
> index 0000000..ff4608b
> --- /dev/null
> +++ b/plugins/external-dummy.c
> @@ -0,0 +1,42 @@
> +/*
> + *
> + *  BlueZ - Bluetooth protocol stack for Linux
> + *
> + *  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 St, Fifth Floor, Boston, MA  02110-1301  USA
> + *
> + */
> +
> +#ifdef HAVE_CONFIG_H
> +#include <config.h>
> +#endif
> +
> +#include "plugin.h"
> +#include "log.h"
> +
> +static int dummy_init(void)
> +{
> +	DBG("");
> +
> +	return 0;
> +}
> +
> +static void dummy_exit(void)
> +{
> +	DBG("");
> +}
> +
> +BLUETOOTH_PLUGIN_DEFINE(external_dummy, VERSION,
> +		BLUETOOTH_PLUGIN_PRIORITY_DEFAULT, dummy_init, dummy_exit)
> +

Just to be paranoid, make it PRIORITY_LOW ;)

Regards

Marcel

Re: [RFC v2 1/2] Fix debug messages logging from non-built-in plugins

[Marcel Holtmann]

Hi Szymon,

>  Makefile.am       |    3 ++-
>  src/bluetooth.ver |    2 ++
>  src/log.c         |   49 +++++++++++++++++++++++++++++++++++++++----------
>  src/log.h         |    2 ++
>  src/plugin.c      |    8 ++++++--
>  5 files changed, 51 insertions(+), 13 deletions(-)
> 
> diff --git a/Makefile.am b/Makefile.am
> index 68380d9..083ad27 100644
> --- a/Makefile.am
> +++ b/Makefile.am
> @@ -285,7 +285,8 @@ src_bluetoothd_SOURCES = $(gdbus_sources) $(builtin_sources) \
>  src_bluetoothd_LDADD = lib/libbluetooth.la @GLIB_LIBS@ @DBUS_LIBS@ \
>  							@CAPNG_LIBS@ -ldl -lrt
>  src_bluetoothd_LDFLAGS = -Wl,--export-dynamic \
> -				-Wl,--version-script=$(srcdir)/src/bluetooth.ver
> +			 -Wl,--version-script=$(srcdir)/src/bluetooth.ver \
> +			 -Wl,-u__start___debug -Wl,-u__stop___debug

Remind me again, we we need them here and in the bluetooth.ver file?
 
>  src_bluetoothd_DEPENDENCIES = lib/libbluetooth.la
>  
> diff --git a/src/bluetooth.ver b/src/bluetooth.ver
> index b71c70d..6ff0df2 100644
> --- a/src/bluetooth.ver
> +++ b/src/bluetooth.ver
> @@ -5,6 +5,8 @@
>  		info;
>  		error;
>  		debug;
> +		__start___debug;
> +		__stop___debug;
>  	local:
>  		*;
>  };

And why are they needed to be exported here. This means that also every
single plugin can access them.

> diff --git a/src/log.c b/src/log.c
> index 2c492e9..9d89f7d 100644
> --- a/src/log.c
> +++ b/src/log.c
> @@ -28,6 +28,7 @@
>  #include <stdio.h>
>  #include <stdarg.h>
>  #include <syslog.h>
> +#include <dlfcn.h>
>  
>  #include <glib.h>
>  
> @@ -66,10 +67,8 @@ void btd_debug(const char *format, ...)
>  	va_end(ap);
>  }
>  
> -extern struct btd_debug_desc __start___debug[];
> -extern struct btd_debug_desc __stop___debug[];
> -
>  static gchar **enabled = NULL;
> +static GSList *handles = NULL;
>  
>  static gboolean is_enabled(struct btd_debug_desc *desc)
>  {
> @@ -88,23 +87,27 @@ static gboolean is_enabled(struct btd_debug_desc *desc)
>  
>  void __btd_toggle_debug(void)
>  {
> -	struct btd_debug_desc *desc;
> +	GSList *l;
> +
> +	for (l = handles; l; l = l->next) {
> +		struct btd_debug_desc *start, *stop;
>  
> -	for (desc = __start___debug; desc < __stop___debug; desc++)
> -		desc->flags |= BTD_DEBUG_FLAG_PRINT;
> +		start = dlsym(l->data, "__start___debug");
> +		stop = dlsym(l->data, "__stop___debug");
> +
> +		for (; start < stop; start++)
> +			start->flags |= BTD_DEBUG_FLAG_PRINT;
> +	}
>  }

We might wanna check if the symbols actually exist first. It think it is
perfectly valid to have plugins without any debug symbols.

And after reading the rest of this patch, you confused me even more. Why
to we have to dlsym the symbols again. Can we not just store them. I
think this is a bit of waste.
 
>  void __btd_log_init(const char *debug, int detach)
>  {
>  	int option = LOG_NDELAY | LOG_PID;
> -	struct btd_debug_desc *desc;
>  
>  	if (debug != NULL)
>  		enabled = g_strsplit_set(debug, ":, ", 0);
>  
> -	for (desc = __start___debug; desc < __stop___debug; desc++)
> -		if (is_enabled(desc))
> -			desc->flags |= BTD_DEBUG_FLAG_PRINT;
> +	__btd_log_add(NULL);
>  
>  	if (!detach)
>  		option |= LOG_PERROR;
> @@ -116,7 +119,33 @@ void __btd_log_init(const char *debug, int detach)
>  
>  void __btd_log_cleanup(void)
>  {
> +	__btd_log_remove(NULL);
> +
>  	closelog();
>  
>  	g_strfreev(enabled);
>  }
> +
> +void __btd_log_add(void *handle)
> +{
> +	struct btd_debug_desc *start, *stop;
> +
> +	start = dlsym(handle, "__start___debug");
> +	stop = dlsym(handle, "__stop___debug");
> +
> +	if (!start || !stop) {
> +		DBG("No __debug section found");
> +		return;
> +	}

So here you check the existence of start and stop. So why not store
these entry points as well.

And please to start == NULL || stop == NULL.

> +	for (; start < stop; start++)
> +		if (is_enabled(start))
> +			start->flags |= BTD_DEBUG_FLAG_PRINT;
> +
> +	handles = g_slist_prepend(handles, handle);
> +}
> +
> +void __btd_log_remove(void *handle)
> +{
> +	handles = g_slist_remove(handles, handle);
> +}
> diff --git a/src/log.h b/src/log.h
> index 78bbdd8..6bb1a96 100644
> --- a/src/log.h
> +++ b/src/log.h
> @@ -29,6 +29,8 @@ void btd_debug(const char *format, ...) __attribute__((format(printf, 1, 2)));
>  void __btd_log_init(const char *debug, int detach);
>  void __btd_log_cleanup(void);
>  void __btd_toggle_debug(void);
> +void __btd_log_add(void *handle);
> +void __btd_log_remove(void *handle);
>  
>  struct btd_debug_desc {
>  	const char *file;
> diff --git a/src/plugin.c b/src/plugin.c
> index 3506553..05855de 100644
> --- a/src/plugin.c
> +++ b/src/plugin.c
> @@ -202,7 +202,9 @@ gboolean plugin_init(GKeyFile *config, const char *enable, const char *disable)
>  			continue;
>  		}
>  
> -		if (add_plugin(handle, desc) == FALSE)
> +		if (add_plugin(handle, desc) == TRUE)
> +			__btd_log_add(handle);
> +		else
>  			dlclose(handle);
>  	}
>  
> @@ -239,8 +241,10 @@ void plugin_cleanup(void)
>  		if (plugin->active == TRUE && plugin->desc->exit)
>  			plugin->desc->exit();
>  
> -		if (plugin->handle != NULL)
> +		if (plugin->handle != NULL) {
> +			__btd_log_remove(plugin->handle);
>  			dlclose(plugin->handle);
> +		}
>  
>  		g_free(plugin);
>  	}

So besides the fact that I think we should store the plugin start and
stop symbols and the weird linker requirements, this looks good.

Regards

Marcel

Re: [RFC v2 1/2] Fix debug messages logging from non-built-in plugins

[Szymon Janc]

Hi,

> >  Makefile.am       |    3 ++-
> >  src/bluetooth.ver |    2 ++
> >  src/log.c         |   49 +++++++++++++++++++++++++++++++++++++++----------
> >  src/log.h         |    2 ++
> >  src/plugin.c      |    8 ++++++--
> >  5 files changed, 51 insertions(+), 13 deletions(-)
> > 
> > diff --git a/Makefile.am b/Makefile.am
> > index 68380d9..083ad27 100644
> > --- a/Makefile.am
> > +++ b/Makefile.am
> > @@ -285,7 +285,8 @@ src_bluetoothd_SOURCES = $(gdbus_sources) $(builtin_sources) \
> >  src_bluetoothd_LDADD = lib/libbluetooth.la @GLIB_LIBS@ @DBUS_LIBS@ \
> >  							@CAPNG_LIBS@ -ldl -lrt
> >  src_bluetoothd_LDFLAGS = -Wl,--export-dynamic \
> > -				-Wl,--version-script=$(srcdir)/src/bluetooth.ver
> > +			 -Wl,--version-script=$(srcdir)/src/bluetooth.ver \
> > +			 -Wl,-u__start___debug -Wl,-u__stop___debug
> 
> Remind me again, we we need them here and in the bluetooth.ver file?

This (-u) make (force) those symbols to be generated (Linker is not generating
them if they are not dereferenced in code) and bluetooth.ver is used to
tell linker which symbols should be visible or not. 

>  
> >  src_bluetoothd_DEPENDENCIES = lib/libbluetooth.la
> >  
> > diff --git a/src/bluetooth.ver b/src/bluetooth.ver
> > index b71c70d..6ff0df2 100644
> > --- a/src/bluetooth.ver
> > +++ b/src/bluetooth.ver
> > @@ -5,6 +5,8 @@
> >  		info;
> >  		error;
> >  		debug;
> > +		__start___debug;
> > +		__stop___debug;
> >  	local:
> >  		*;
> >  };
> 
> And why are they needed to be exported here. This means that also every
> single plugin can access them.

Otherwise those symbols will not be accessible via dlsym().

> 
> > diff --git a/src/log.c b/src/log.c
> > index 2c492e9..9d89f7d 100644
> > --- a/src/log.c
> > +++ b/src/log.c
> > @@ -28,6 +28,7 @@
> >  #include <stdio.h>
> >  #include <stdarg.h>
> >  #include <syslog.h>
> > +#include <dlfcn.h>
> >  
> >  #include <glib.h>
> >  
> > @@ -66,10 +67,8 @@ void btd_debug(const char *format, ...)
> >  	va_end(ap);
> >  }
> >  
> > -extern struct btd_debug_desc __start___debug[];
> > -extern struct btd_debug_desc __stop___debug[];
> > -
> >  static gchar **enabled = NULL;
> > +static GSList *handles = NULL;
> >  
> >  static gboolean is_enabled(struct btd_debug_desc *desc)
> >  {
> > @@ -88,23 +87,27 @@ static gboolean is_enabled(struct btd_debug_desc *desc)
> >  
> >  void __btd_toggle_debug(void)
> >  {
> > -	struct btd_debug_desc *desc;
> > +	GSList *l;
> > +
> > +	for (l = handles; l; l = l->next) {
> > +		struct btd_debug_desc *start, *stop;
> >  
> > -	for (desc = __start___debug; desc < __stop___debug; desc++)
> > -		desc->flags |= BTD_DEBUG_FLAG_PRINT;
> > +		start = dlsym(l->data, "__start___debug");
> > +		stop = dlsym(l->data, "__stop___debug");
> > +
> > +		for (; start < stop; start++)
> > +			start->flags |= BTD_DEBUG_FLAG_PRINT;
> > +	}
> >  }
> 
> We might wanna check if the symbols actually exist first. It think it is
> perfectly valid to have plugins without any debug symbols.

We add handler to this list only if those symbols were found so no need to
check this here as well.

> 
> And after reading the rest of this patch, you confused me even more. Why
> to we have to dlsym the symbols again. Can we not just store them. I
> think this is a bit of waste.

I wanted to minimize need of memory allocations. Since we need to store two
pointers, we need to allocate structure for that and now we just store pointer
to handler.

But if you prefer to store those pointers directly I will do that (it was that
way in first version anyway..)

>  
> >  void __btd_log_init(const char *debug, int detach)
> >  {
> >  	int option = LOG_NDELAY | LOG_PID;
> > -	struct btd_debug_desc *desc;
> >  
> >  	if (debug != NULL)
> >  		enabled = g_strsplit_set(debug, ":, ", 0);
> >  
> > -	for (desc = __start___debug; desc < __stop___debug; desc++)
> > -		if (is_enabled(desc))
> > -			desc->flags |= BTD_DEBUG_FLAG_PRINT;
> > +	__btd_log_add(NULL);
> >  
> >  	if (!detach)
> >  		option |= LOG_PERROR;
> > @@ -116,7 +119,33 @@ void __btd_log_init(const char *debug, int detach)
> >  
> >  void __btd_log_cleanup(void)
> >  {
> > +	__btd_log_remove(NULL);
> > +
> >  	closelog();
> >  
> >  	g_strfreev(enabled);
> >  }
> > +
> > +void __btd_log_add(void *handle)
> > +{
> > +	struct btd_debug_desc *start, *stop;
> > +
> > +	start = dlsym(handle, "__start___debug");
> > +	stop = dlsym(handle, "__stop___debug");
> > +
> > +	if (!start || !stop) {
> > +		DBG("No __debug section found");
> > +		return;
> > +	}
> 
> So here you check the existence of start and stop. So why not store
> these entry points as well.

The idea was to store only handler with those symbols available.
 
> And please to start == NULL || stop == NULL.
> 
> > +	for (; start < stop; start++)
> > +		if (is_enabled(start))
> > +			start->flags |= BTD_DEBUG_FLAG_PRINT;
> > +
> > +	handles = g_slist_prepend(handles, handle);
> > +}
> > +
> > +void __btd_log_remove(void *handle)
> > +{
> > +	handles = g_slist_remove(handles, handle);
> > +}
> > diff --git a/src/log.h b/src/log.h
> > index 78bbdd8..6bb1a96 100644
> > --- a/src/log.h
> > +++ b/src/log.h
> > @@ -29,6 +29,8 @@ void btd_debug(const char *format, ...) __attribute__((format(printf, 1, 2)));
> >  void __btd_log_init(const char *debug, int detach);
> >  void __btd_log_cleanup(void);
> >  void __btd_toggle_debug(void);
> > +void __btd_log_add(void *handle);
> > +void __btd_log_remove(void *handle);
> >  
> >  struct btd_debug_desc {
> >  	const char *file;
> > diff --git a/src/plugin.c b/src/plugin.c
> > index 3506553..05855de 100644
> > --- a/src/plugin.c
> > +++ b/src/plugin.c
> > @@ -202,7 +202,9 @@ gboolean plugin_init(GKeyFile *config, const char *enable, const char *disable)
> >  			continue;
> >  		}
> >  
> > -		if (add_plugin(handle, desc) == FALSE)
> > +		if (add_plugin(handle, desc) == TRUE)
> > +			__btd_log_add(handle);
> > +		else
> >  			dlclose(handle);
> >  	}
> >  
> > @@ -239,8 +241,10 @@ void plugin_cleanup(void)
> >  		if (plugin->active == TRUE && plugin->desc->exit)
> >  			plugin->desc->exit();
> >  
> > -		if (plugin->handle != NULL)
> > +		if (plugin->handle != NULL) {
> > +			__btd_log_remove(plugin->handle);
> >  			dlclose(plugin->handle);
> > +		}
> >  
> >  		g_free(plugin);
> >  	}
> 
> So besides the fact that I think we should store the plugin start and
> stop symbols and the weird linker requirements, this looks good.

I'll store those pointers directly. As for linker stuff the only solution
other than using -u flag is to dereference those symbols in code directly.

I'll send V3 shortly that will try to address those issues.

-- 
BR
Szymon Janc

Re: [RFC v2 2/2] Add external dummy plugin for testing

[Szymon Janc]

> Hi Szymon,
> 
> >  Makefile.am              |    9 +++++++++
> >  acinclude.m4             |    6 ++++++
> >  plugins/external-dummy.c |   42 ++++++++++++++++++++++++++++++++++++++++++
> >  3 files changed, 57 insertions(+), 0 deletions(-)
> >  create mode 100644 plugins/external-dummy.c
> > 
> > diff --git a/Makefile.am b/Makefile.am
> > index 083ad27..b51f82e 100644
> > --- a/Makefile.am
> > +++ b/Makefile.am
> > @@ -48,6 +48,8 @@ endif
> >  plugindir = $(libdir)/bluetooth/plugins
> >  
> >  plugin_LTLIBRARIES =
> > +plugin_LDFLAGS = -module -avoid-version -Wl,--export-dynamic \
> > +			-Wl,-u__start___debug -Wl,-u__stop___debug
> 
> this part is a bit ugly. I wish we could automate it.
> 
> And what does actually take care of bluetooth_plugin_desc since that is
> suppose to be the only exported symbol in the first place.

As explained in last email, this could be automated with some trigger function
that would dereference those pointer. With combination with 
__attribute__((constructor/destructor)) this could be quite automatic..
I'll investigate this and send V3 shortly.

> >  lib_headers = lib/bluetooth.h lib/hci.h lib/hci_lib.h lib/mgmt.h \
> > @@ -258,6 +260,13 @@ builtin_modules += dbusoob
> >  builtin_sources += plugins/dbusoob.c
> >  endif
> >  
> > +if EXTERNALDUMMYPLUGIN
> > +plugin_LTLIBRARIES += plugins/external-dummy.la
> > +plugins_external_dummy_la_SOURCES = plugins/external-dummy.c
> > +plugins_external_dummy_la_LDFLAGS = $(plugin_LDFLAGS)
> > +plugins_external_dummy_la_CFLAGS = -fvisibility=hidden
> > +endif
> > +
> >  sbin_PROGRAMS += src/bluetoothd
> >  
> >  src_bluetoothd_SOURCES = $(gdbus_sources) $(builtin_sources) \
> > diff --git a/acinclude.m4 b/acinclude.m4
> > index 3cb9459..737c000 100644
> > --- a/acinclude.m4
> > +++ b/acinclude.m4
> > @@ -216,6 +216,7 @@ AC_DEFUN([AC_ARG_BLUEZ], [
> >  	dbusoob_enable=no
> >  	wiimote_enable=no
> >  	thermometer_enable=no
> > +	external_dummy_enable=no
> >  
> >  	AC_ARG_ENABLE(optimization, AC_HELP_STRING([--disable-optimization], [disable code optimization]), [
> >  		optimization_enable=${enableval}
> > @@ -364,6 +365,10 @@ AC_DEFUN([AC_ARG_BLUEZ], [
> >  		thermometer_enable=${enableval}
> >  	])
> >  
> > +	AC_ARG_ENABLE(external_dummy, AC_HELP_STRING([--enable-external-dummy], [enable sample external plugin]), [
> > +		external_dummy_enable=${enableval}
> > +	])
> > +
> 
> Please don't. This project has already too many configure options. Just
> use if MAINTAINER_MODE in Makefile.am.

OK.

> 
> >  	if (test "${fortify_enable}" = "yes"); then
> >  		CFLAGS="$CFLAGS -D_FORTIFY_SOURCE=2"
> >  	fi
> > @@ -421,4 +426,5 @@ AC_DEFUN([AC_ARG_BLUEZ], [
> >  	AM_CONDITIONAL(DBUSOOBPLUGIN, test "${dbusoob_enable}" = "yes")
> >  	AM_CONDITIONAL(WIIMOTEPLUGIN, test "${wiimote_enable}" = "yes")
> >  	AM_CONDITIONAL(THERMOMETERPLUGIN, test "${thermometer_enable}" = "yes")
> > +	AM_CONDITIONAL(EXTERNALDUMMYPLUGIN, test "${external_dummy_enable}" = "yes")
> >  ])
> > diff --git a/plugins/external-dummy.c b/plugins/external-dummy.c
> > new file mode 100644
> > index 0000000..ff4608b
> > --- /dev/null
> > +++ b/plugins/external-dummy.c
> > @@ -0,0 +1,42 @@
> > +/*
> > + *
> > + *  BlueZ - Bluetooth protocol stack for Linux
> > + *
> > + *  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 St, Fifth Floor, Boston, MA  02110-1301  USA
> > + *
> > + */
> > +
> > +#ifdef HAVE_CONFIG_H
> > +#include <config.h>
> > +#endif
> > +
> > +#include "plugin.h"
> > +#include "log.h"
> > +
> > +static int dummy_init(void)
> > +{
> > +	DBG("");
> > +
> > +	return 0;
> > +}
> > +
> > +static void dummy_exit(void)
> > +{
> > +	DBG("");
> > +}
> > +
> > +BLUETOOTH_PLUGIN_DEFINE(external_dummy, VERSION,
> > +		BLUETOOTH_PLUGIN_PRIORITY_DEFAULT, dummy_init, dummy_exit)
> > +
> 
> Just to be paranoid, make it PRIORITY_LOW ;)

OK :)

Re: [RFC v2 1/2] Fix debug messages logging from non-built-in plugins

[Marcel Holtmann]

Hi Szymon,

> > >  Makefile.am       |    3 ++-
> > >  src/bluetooth.ver |    2 ++
> > >  src/log.c         |   49 +++++++++++++++++++++++++++++++++++++++----------
> > >  src/log.h         |    2 ++
> > >  src/plugin.c      |    8 ++++++--
> > >  5 files changed, 51 insertions(+), 13 deletions(-)
> > > 
> > > diff --git a/Makefile.am b/Makefile.am
> > > index 68380d9..083ad27 100644
> > > --- a/Makefile.am
> > > +++ b/Makefile.am
> > > @@ -285,7 +285,8 @@ src_bluetoothd_SOURCES = $(gdbus_sources) $(builtin_sources) \
> > >  src_bluetoothd_LDADD = lib/libbluetooth.la @GLIB_LIBS@ @DBUS_LIBS@ \
> > >  							@CAPNG_LIBS@ -ldl -lrt
> > >  src_bluetoothd_LDFLAGS = -Wl,--export-dynamic \
> > > -				-Wl,--version-script=$(srcdir)/src/bluetooth.ver
> > > +			 -Wl,--version-script=$(srcdir)/src/bluetooth.ver \
> > > +			 -Wl,-u__start___debug -Wl,-u__stop___debug
> > 
> > Remind me again, we we need them here and in the bluetooth.ver file?
> 
> This (-u) make (force) those symbols to be generated (Linker is not generating
> them if they are not dereferenced in code) and bluetooth.ver is used to
> tell linker which symbols should be visible or not. 

but if no DBG calls are present, why bother. It is fine if dlsym fails
for the main symbols. Or am I missing something?

> > >  src_bluetoothd_DEPENDENCIES = lib/libbluetooth.la
> > >  
> > > diff --git a/src/bluetooth.ver b/src/bluetooth.ver
> > > index b71c70d..6ff0df2 100644
> > > --- a/src/bluetooth.ver
> > > +++ b/src/bluetooth.ver
> > > @@ -5,6 +5,8 @@
> > >  		info;
> > >  		error;
> > >  		debug;
> > > +		__start___debug;
> > > +		__stop___debug;
> > >  	local:
> > >  		*;
> > >  };
> > 
> > And why are they needed to be exported here. This means that also every
> > single plugin can access them.
> 
> Otherwise those symbols will not be accessible via dlsym().

I see. We have to get rid of dlsym() and do something similar to
EXPORT_SYMBOL like the kernel does. Just default visibility to hidden
and change it on a per symbol basis.

> > > diff --git a/src/log.c b/src/log.c
> > > index 2c492e9..9d89f7d 100644
> > > --- a/src/log.c
> > > +++ b/src/log.c
> > > @@ -28,6 +28,7 @@
> > >  #include <stdio.h>
> > >  #include <stdarg.h>
> > >  #include <syslog.h>
> > > +#include <dlfcn.h>
> > >  
> > >  #include <glib.h>
> > >  
> > > @@ -66,10 +67,8 @@ void btd_debug(const char *format, ...)
> > >  	va_end(ap);
> > >  }
> > >  
> > > -extern struct btd_debug_desc __start___debug[];
> > > -extern struct btd_debug_desc __stop___debug[];
> > > -
> > >  static gchar **enabled = NULL;
> > > +static GSList *handles = NULL;
> > >  
> > >  static gboolean is_enabled(struct btd_debug_desc *desc)
> > >  {
> > > @@ -88,23 +87,27 @@ static gboolean is_enabled(struct btd_debug_desc *desc)
> > >  
> > >  void __btd_toggle_debug(void)
> > >  {
> > > -	struct btd_debug_desc *desc;
> > > +	GSList *l;
> > > +
> > > +	for (l = handles; l; l = l->next) {
> > > +		struct btd_debug_desc *start, *stop;
> > >  
> > > -	for (desc = __start___debug; desc < __stop___debug; desc++)
> > > -		desc->flags |= BTD_DEBUG_FLAG_PRINT;
> > > +		start = dlsym(l->data, "__start___debug");
> > > +		stop = dlsym(l->data, "__stop___debug");
> > > +
> > > +		for (; start < stop; start++)
> > > +			start->flags |= BTD_DEBUG_FLAG_PRINT;
> > > +	}
> > >  }
> > 
> > We might wanna check if the symbols actually exist first. It think it is
> > perfectly valid to have plugins without any debug symbols.
> 
> We add handler to this list only if those symbols were found so no need to
> check this here as well.
> 
> > 
> > And after reading the rest of this patch, you confused me even more. Why
> > to we have to dlsym the symbols again. Can we not just store them. I
> > think this is a bit of waste.
> 
> I wanted to minimize need of memory allocations. Since we need to store two
> pointers, we need to allocate structure for that and now we just store pointer
> to handler.
> 
> But if you prefer to store those pointers directly I will do that (it was that
> way in first version anyway..)

I am not sure what is actually cheaper in the end. I prefer not to have
the dlsym calls since eventually we wanna allow full dynamic changes for
the debug settings.

> > >  void __btd_log_init(const char *debug, int detach)
> > >  {
> > >  	int option = LOG_NDELAY | LOG_PID;
> > > -	struct btd_debug_desc *desc;
> > >  
> > >  	if (debug != NULL)
> > >  		enabled = g_strsplit_set(debug, ":, ", 0);
> > >  
> > > -	for (desc = __start___debug; desc < __stop___debug; desc++)
> > > -		if (is_enabled(desc))
> > > -			desc->flags |= BTD_DEBUG_FLAG_PRINT;
> > > +	__btd_log_add(NULL);
> > >  
> > >  	if (!detach)
> > >  		option |= LOG_PERROR;
> > > @@ -116,7 +119,33 @@ void __btd_log_init(const char *debug, int detach)
> > >  
> > >  void __btd_log_cleanup(void)
> > >  {
> > > +	__btd_log_remove(NULL);
> > > +
> > >  	closelog();
> > >  
> > >  	g_strfreev(enabled);
> > >  }
> > > +
> > > +void __btd_log_add(void *handle)
> > > +{
> > > +	struct btd_debug_desc *start, *stop;
> > > +
> > > +	start = dlsym(handle, "__start___debug");
> > > +	stop = dlsym(handle, "__stop___debug");
> > > +
> > > +	if (!start || !stop) {
> > > +		DBG("No __debug section found");
> > > +		return;
> > > +	}
> > 
> > So here you check the existence of start and stop. So why not store
> > these entry points as well.
> 
> The idea was to store only handler with those symbols available.
>  
> > And please to start == NULL || stop == NULL.
> > 
> > > +	for (; start < stop; start++)
> > > +		if (is_enabled(start))
> > > +			start->flags |= BTD_DEBUG_FLAG_PRINT;
> > > +
> > > +	handles = g_slist_prepend(handles, handle);
> > > +}
> > > +
> > > +void __btd_log_remove(void *handle)
> > > +{
> > > +	handles = g_slist_remove(handles, handle);
> > > +}
> > > diff --git a/src/log.h b/src/log.h
> > > index 78bbdd8..6bb1a96 100644
> > > --- a/src/log.h
> > > +++ b/src/log.h
> > > @@ -29,6 +29,8 @@ void btd_debug(const char *format, ...) __attribute__((format(printf, 1, 2)));
> > >  void __btd_log_init(const char *debug, int detach);
> > >  void __btd_log_cleanup(void);
> > >  void __btd_toggle_debug(void);
> > > +void __btd_log_add(void *handle);
> > > +void __btd_log_remove(void *handle);
> > >  
> > >  struct btd_debug_desc {
> > >  	const char *file;
> > > diff --git a/src/plugin.c b/src/plugin.c
> > > index 3506553..05855de 100644
> > > --- a/src/plugin.c
> > > +++ b/src/plugin.c
> > > @@ -202,7 +202,9 @@ gboolean plugin_init(GKeyFile *config, const char *enable, const char *disable)
> > >  			continue;
> > >  		}
> > >  
> > > -		if (add_plugin(handle, desc) == FALSE)
> > > +		if (add_plugin(handle, desc) == TRUE)
> > > +			__btd_log_add(handle);
> > > +		else
> > >  			dlclose(handle);
> > >  	}
> > >  
> > > @@ -239,8 +241,10 @@ void plugin_cleanup(void)
> > >  		if (plugin->active == TRUE && plugin->desc->exit)
> > >  			plugin->desc->exit();
> > >  
> > > -		if (plugin->handle != NULL)
> > > +		if (plugin->handle != NULL) {
> > > +			__btd_log_remove(plugin->handle);
> > >  			dlclose(plugin->handle);
> > > +		}
> > >  
> > >  		g_free(plugin);
> > >  	}
> > 
> > So besides the fact that I think we should store the plugin start and
> > stop symbols and the weird linker requirements, this looks good.
> 
> I'll store those pointers directly. As for linker stuff the only solution
> other than using -u flag is to dereference those symbols in code directly.
> 
> I'll send V3 shortly that will try to address those issues.

Sounds good.

Regards

Marcel