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
--
To unsubscribe from this list: send the line "unsubscribe linux-bluetooth" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
