Hello everyone,
Here's v3 fixing a small bug with the previous patches, which was
tripping the CI.
Link to the previous revision can be found below.
Thanks
Emil
- Link to v2: https://lore.kernel.org/r/[email protected]
---
Emil Velikov (8):
configure, README: introduce --enable-external-plugins
obexd: factor out external plugin support
bluetoothd: remove external-dummy plugin
bluetoothd: convert external sixaxis plugin to builtin
bluetoothd: factor out external plugin support
bluetoothd: don't export internal API
bluetoothd: change plugin loading alike obexd
android: export only (android) entrypoint from the modules
Makefile.am | 17 ++----
Makefile.obexd | 2 +
Makefile.plugins | 8 +--
README | 13 +++++
android/Makefile.am | 3 +
android/hal-audio.c | 1 +
android/hal-bluetooth.c | 1 +
android/hal-sco.c | 1 +
configure.ac | 10 ++++
obexd/src/obexd.h | 2 +-
obexd/src/plugin.c | 93 +++++++++++++++++++++----------
obexd/src/plugin.h | 4 ++
plugins/external-dummy.c | 28 ----------
src/btd.h | 2 +-
src/plugin.c | 140 +++++++++++++++++++++++++++++------------------
src/plugin.h | 16 ++++++
16 files changed, 209 insertions(+), 132 deletions(-)
---
base-commit: a9d1f6f6a625607de6c3f5b7a40a3aac5f36c02b
change-id: 20240116-rm-ext-plugins-ba0b852a492b
Best regards,
--
Emil Velikov <[email protected]>
From: Emil Velikov <[email protected]>
Convert the only known external plugin to built-in. It's a tiny 20K
binary that distros ship a separate package for.
Make it a builtin, which allows distros to drop the separate package, it
also enables us to compile out support for external modules - both in
terms of extra code and hide the internal bluetoothd API.
This means that libudev.so is pulled in, which is fine since its ABI has
been stable for over a decade.
---
Makefile.plugins | 8 +++-----
1 file changed, 3 insertions(+), 5 deletions(-)
diff --git a/Makefile.plugins b/Makefile.plugins
index 5880ed0df..7cf66fd59 100644
--- a/Makefile.plugins
+++ b/Makefile.plugins
@@ -110,11 +110,9 @@ builtin_modules += battery
builtin_sources += profiles/battery/battery.c
if SIXAXIS
-plugin_LTLIBRARIES += plugins/sixaxis.la
-plugins_sixaxis_la_SOURCES = plugins/sixaxis.c
-plugins_sixaxis_la_LDFLAGS = $(AM_LDFLAGS) -module -avoid-version
-plugins_sixaxis_la_LIBADD = $(UDEV_LIBS)
-plugins_sixaxis_la_CFLAGS = $(AM_CFLAGS) -fvisibility=hidden
+builtin_modules += sixaxis
+builtin_sources += plugins/sixaxis.c
+builtin_ldadd += $(UDEV_LIBS)
endif
if BAP
--
2.43.0
From: Emil Velikov <[email protected]>
The external plugins infra is getting deprecated and disabled by
default. Remove this dummy plugin.
---
Makefile.am | 8 --------
plugins/external-dummy.c | 28 ----------------------------
2 files changed, 36 deletions(-)
diff --git a/Makefile.am b/Makefile.am
index e738eb3a5..ea51b25cc 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -293,14 +293,6 @@ builtin_ldadd =
include Makefile.plugins
-if MAINTAINER_MODE
-plugin_LTLIBRARIES += plugins/external-dummy.la
-plugins_external_dummy_la_SOURCES = plugins/external-dummy.c
-plugins_external_dummy_la_LDFLAGS = $(AM_LDFLAGS) -module -avoid-version \
- -no-undefined
-plugins_external_dummy_la_CFLAGS = $(AM_CFLAGS) -fvisibility=hidden
-endif
-
pkglibexec_PROGRAMS += src/bluetoothd
src_bluetoothd_SOURCES = $(builtin_sources) \
diff --git a/plugins/external-dummy.c b/plugins/external-dummy.c
deleted file mode 100644
index 1c209e8b7..000000000
--- a/plugins/external-dummy.c
+++ /dev/null
@@ -1,28 +0,0 @@
-// SPDX-License-Identifier: GPL-2.0-or-later
-/*
- *
- * BlueZ - Bluetooth protocol stack for Linux
- *
- */
-
-#ifdef HAVE_CONFIG_H
-#include <config.h>
-#endif
-
-#include "src/plugin.h"
-#include "src/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_LOW, dummy_init, dummy_exit)
--
2.43.0
From: Emil Velikov <[email protected]>
As a whole all plugins should be built-in, otherwise they would be using
internal, undocumented, unversioned, unstable API.
Flesh out the external plugin support into a few pre-processor blocks
and simplify the normal path.
---
Makefile.am | 4 ---
src/btd.h | 2 +-
src/plugin.c | 97 ++++++++++++++++++++++++++++++++++++++----------------------
src/plugin.h | 16 ++++++++++
4 files changed, 79 insertions(+), 40 deletions(-)
diff --git a/Makefile.am b/Makefile.am
index ea51b25cc..1b82e8551 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -51,11 +51,7 @@ EXTRA_DIST += src/bluetooth.service.in src/org.bluez.service
plugindir = $(libdir)/bluetooth/plugins
-if MAINTAINER_MODE
-build_plugindir = $(abs_top_srcdir)/plugins/.libs
-else
build_plugindir = $(plugindir)
-endif
if MANPAGES
man_MANS =
diff --git a/src/btd.h b/src/btd.h
index b7e7ebd61..7166e2168 100644
--- a/src/btd.h
+++ b/src/btd.h
@@ -155,7 +155,7 @@ struct btd_opts {
extern struct btd_opts btd_opts;
-gboolean plugin_init(const char *enable, const char *disable);
+void plugin_init(const char *enable, const char *disable);
void plugin_cleanup(void);
void rfkill_init(void);
diff --git a/src/plugin.c b/src/plugin.c
index 2a29a888e..ae9406375 100644
--- a/src/plugin.c
+++ b/src/plugin.c
@@ -29,7 +29,9 @@
static GSList *plugins = NULL;
struct bluetooth_plugin {
+#if EXTERNAL_PLUGINS
void *handle;
+#endif
gboolean active;
const struct bluetooth_plugin_desc *desc;
};
@@ -42,7 +44,8 @@ static int compare_priority(gconstpointer a, gconstpointer b)
return plugin2->desc->priority - plugin1->desc->priority;
}
-static gboolean add_plugin(void *handle,
+#if EXTERNAL_PLUGINS
+static gboolean add_external_plugin(void *handle,
const struct bluetooth_plugin_desc *desc)
{
struct bluetooth_plugin *plugin;
@@ -71,6 +74,22 @@ static gboolean add_plugin(void *handle,
return TRUE;
}
+#endif
+
+static void add_plugin(const struct bluetooth_plugin_desc *desc)
+{
+ struct bluetooth_plugin *plugin;
+
+ DBG("Loading %s plugin", desc->name);
+
+ plugin = g_try_new0(struct bluetooth_plugin, 1);
+ if (plugin == NULL)
+ return;
+
+ plugin->desc = desc;
+
+ plugins = g_slist_insert_sorted(plugins, plugin, compare_priority);
+}
static gboolean enable_plugin(const char *name, char **cli_enable,
char **cli_disable)
@@ -98,48 +117,24 @@ static gboolean enable_plugin(const char *name, char **cli_enable,
return TRUE;
}
-#include "src/builtin.h"
-gboolean plugin_init(const char *enable, const char *disable)
+static void external_plugin_init(char **cli_disabled, char **cli_enabled)
{
- GSList *list;
+#if EXTERNAL_PLUGINS
GDir *dir;
const char *file;
- char **cli_disabled, **cli_enabled;
- unsigned int i;
-
- /* Make a call to BtIO API so its symbols got resolved before the
- * plugins are loaded. */
- bt_io_error_quark();
- if (enable)
- cli_enabled = g_strsplit_set(enable, ", ", -1);
- else
- cli_enabled = NULL;
-
- if (disable)
- cli_disabled = g_strsplit_set(disable, ", ", -1);
- else
- cli_disabled = NULL;
-
- DBG("Loading builtin plugins");
-
- for (i = 0; __bluetooth_builtin[i]; i++) {
- if (!enable_plugin(__bluetooth_builtin[i]->name, cli_enabled,
- cli_disabled))
- continue;
-
- add_plugin(NULL, __bluetooth_builtin[i]);
- }
+ warn("Using external plugins is not officially supported.\n");
+ warn("Consider upstreaming your plugins into the BlueZ project.");
if (strlen(PLUGINDIR) == 0)
- goto start;
+ return;
DBG("Loading plugins %s", PLUGINDIR);
dir = g_dir_open(PLUGINDIR, 0, NULL);
if (!dir)
- goto start;
+ return;
while ((file = g_dir_read_name(dir)) != NULL) {
const struct bluetooth_plugin_desc *desc;
@@ -174,13 +169,45 @@ gboolean plugin_init(const char *enable, const char *disable)
continue;
}
- if (add_plugin(handle, desc) == FALSE)
+ if (add_external_plugin(handle, desc) == FALSE)
dlclose(handle);
}
g_dir_close(dir);
+#endif
+}
+
+#include "src/builtin.h"
+
+void plugin_init(const char *enable, const char *disable)
+{
+ GSList *list;
+ char **cli_disabled = NULL;
+ char **cli_enabled = NULL;
+ unsigned int i;
+
+ /* Make a call to BtIO API so its symbols got resolved before the
+ * plugins are loaded. */
+ bt_io_error_quark();
+
+ if (enable)
+ cli_enabled = g_strsplit_set(enable, ", ", -1);
+
+ if (disable)
+ cli_disabled = g_strsplit_set(disable, ", ", -1);
+
+ DBG("Loading builtin plugins");
+
+ for (i = 0; __bluetooth_builtin[i]; i++) {
+ if (!enable_plugin(__bluetooth_builtin[i]->name, cli_enabled,
+ cli_disabled))
+ continue;
+
+ add_plugin(__bluetooth_builtin[i]);
+ }
+
+ external_plugin_init(cli_enabled, cli_disabled);
-start:
for (list = plugins; list; list = list->next) {
struct bluetooth_plugin *plugin = list->data;
int err;
@@ -201,8 +228,6 @@ start:
g_strfreev(cli_enabled);
g_strfreev(cli_disabled);
-
- return TRUE;
}
void plugin_cleanup(void)
@@ -217,8 +242,10 @@ void plugin_cleanup(void)
if (plugin->active == TRUE && plugin->desc->exit)
plugin->desc->exit();
+#if EXTERNAL_PLUGINS
if (plugin->handle != NULL)
dlclose(plugin->handle);
+#endif
g_free(plugin);
}
diff --git a/src/plugin.h b/src/plugin.h
index 8d0903f2d..a1984d536 100644
--- a/src/plugin.h
+++ b/src/plugin.h
@@ -13,14 +13,19 @@
struct bluetooth_plugin_desc {
const char *name;
+#if EXTERNAL_PLUGINS
const char *version;
+#endif
int priority;
int (*init) (void);
void (*exit) (void);
+#if EXTERNAL_PLUGINS
void *debug_start;
void *debug_stop;
+#endif
};
+#if EXTERNAL_PLUGINS
#ifdef BLUETOOTH_PLUGIN_BUILTIN
#define BLUETOOTH_PLUGIN_DEFINE(name, version, priority, init, exit) \
const struct bluetooth_plugin_desc \
@@ -41,3 +46,14 @@ struct bluetooth_plugin_desc {
__start___debug, __stop___debug \
};
#endif
+#else
+#ifdef BLUETOOTH_PLUGIN_BUILTIN
+#define BLUETOOTH_PLUGIN_DEFINE(name, version, priority, init, exit) \
+ const struct bluetooth_plugin_desc \
+ __bluetooth_builtin_ ## name = { \
+ #name, priority, init, exit \
+ };
+#else
+#error "Requested non built-in plugin, while external plugins is disabled"
+#endif
+#endif
--
2.43.0
From: Emil Velikov <[email protected]>
The android specific modules, have a designated HMI entrypoint. Hide
everything else with -fvisibility=hidden.
---
android/Makefile.am | 3 +++
android/hal-audio.c | 1 +
android/hal-bluetooth.c | 1 +
android/hal-sco.c | 1 +
4 files changed, 6 insertions(+)
diff --git a/android/Makefile.am b/android/Makefile.am
index 309910147..e3756e89c 100644
--- a/android/Makefile.am
+++ b/android/Makefile.am
@@ -96,6 +96,7 @@ android_bluetooth_default_la_SOURCES = android/hal.h android/hal-bluetooth.c \
android/hal-log.h \
android/hal-ipc.h android/hal-ipc.c \
android/hal-utils.h android/hal-utils.c
+android_bluetooth_default_la_CFLAGS = $(AM_CFLAGS) -fvisibility=hidden
android_bluetooth_default_la_CPPFLAGS = $(AM_CPPFLAGS) -I$(srcdir)/android
android_bluetooth_default_la_LDFLAGS = $(AM_LDFLAGS) -module -avoid-version \
-no-undefined
@@ -195,6 +196,7 @@ android_audio_a2dp_default_la_SOURCES = android/audio-msg.h \
android/hardware/audio_effect.h \
android/hardware/hardware.h \
android/system/audio.h
+android_audio_a2dp_default_la_CFLAGS = $(AM_CFLAGS) -fvisibility=hidden
android_audio_a2dp_default_la_CPPFLAGS = $(AM_CPPFLAGS) -I$(srcdir)/android \
$(SBC_CFLAGS)
android_audio_a2dp_default_la_LIBADD = $(SBC_LIBS) -lrt
@@ -212,6 +214,7 @@ android_audio_sco_default_la_SOURCES = android/hal-log.h \
android/audio_utils/resampler.c \
android/audio_utils/resampler.h \
android/system/audio.h
+android_audio_sco_default_la_CFLAGS = $(AM_CFLAGS) -fvisibility=hidden
android_audio_sco_default_la_CPPFLAGS = $(AM_CPPFLAGS) -I$(srcdir)/android
android_audio_sco_default_la_LIBADD = $(SPEEXDSP_LIBS) -lrt
android_audio_sco_default_la_LDFLAGS = $(AM_LDFLAGS) -module -avoid-version \
diff --git a/android/hal-audio.c b/android/hal-audio.c
index d37d6098c..f3d9b40a6 100644
--- a/android/hal-audio.c
+++ b/android/hal-audio.c
@@ -1618,6 +1618,7 @@ static struct hw_module_methods_t hal_module_methods = {
.open = audio_open,
};
+__attribute__ ((visibility("default")))
struct audio_module HAL_MODULE_INFO_SYM = {
.common = {
.tag = HARDWARE_MODULE_TAG,
diff --git a/android/hal-bluetooth.c b/android/hal-bluetooth.c
index d4442e620..7d1e5ac63 100644
--- a/android/hal-bluetooth.c
+++ b/android/hal-bluetooth.c
@@ -1117,6 +1117,7 @@ static struct hw_module_methods_t bluetooth_module_methods = {
.open = open_bluetooth,
};
+__attribute__ ((visibility("default")))
struct hw_module_t HAL_MODULE_INFO_SYM = {
.tag = HARDWARE_MODULE_TAG,
.version_major = 1,
diff --git a/android/hal-sco.c b/android/hal-sco.c
index d7c08a68b..3d66ad357 100644
--- a/android/hal-sco.c
+++ b/android/hal-sco.c
@@ -1507,6 +1507,7 @@ static struct hw_module_methods_t hal_module_methods = {
.open = sco_open,
};
+__attribute__ ((visibility("default")))
struct audio_module HAL_MODULE_INFO_SYM = {
.common = {
.tag = HARDWARE_MODULE_TAG,
--
2.43.0
From: Emil Velikov <[email protected]>
Currently, we print "Loading foobar" for every plugin, before we try the
respective init() callback. Instead we handle the latter in a bunch, at
the end of the process.
Do the init() call early, print "Loaded" once it's actually successful
and drop the no-longer "active" tracking.
---
src/plugin.c | 53 +++++++++++++++++++++++++++++------------------------
1 file changed, 29 insertions(+), 24 deletions(-)
diff --git a/src/plugin.c b/src/plugin.c
index ae9406375..3895792bc 100644
--- a/src/plugin.c
+++ b/src/plugin.c
@@ -32,7 +32,6 @@ struct bluetooth_plugin {
#if EXTERNAL_PLUGINS
void *handle;
#endif
- gboolean active;
const struct bluetooth_plugin_desc *desc;
};
@@ -44,6 +43,22 @@ static int compare_priority(gconstpointer a, gconstpointer b)
return plugin2->desc->priority - plugin1->desc->priority;
}
+static int init_plugin(const struct bluetooth_plugin_desc *desc)
+{
+ int err;
+
+ err = desc->init();
+ if (err < 0) {
+ if (err == -ENOSYS || err == -ENOTSUP)
+ warn("System does not support %s plugin",
+ desc->name);
+ else
+ error("Failed to init %s plugin",
+ desc->name);
+ }
+ return err;
+}
+
#if EXTERNAL_PLUGINS
static gboolean add_external_plugin(void *handle,
const struct bluetooth_plugin_desc *desc)
@@ -58,19 +73,22 @@ static gboolean add_external_plugin(void *handle,
return FALSE;
}
- DBG("Loading %s plugin", desc->name);
-
plugin = g_try_new0(struct bluetooth_plugin, 1);
if (plugin == NULL)
return FALSE;
plugin->handle = handle;
- plugin->active = FALSE;
plugin->desc = desc;
+ if (init_plugin(desc) < 0) {
+ g_free(plugin);
+ return FALSE;
+ }
+
__btd_enable_debug(desc->debug_start, desc->debug_stop);
plugins = g_slist_insert_sorted(plugins, plugin, compare_priority);
+ DBG("Plugin %s loaded", desc->name);
return TRUE;
}
@@ -88,7 +106,13 @@ static void add_plugin(const struct bluetooth_plugin_desc *desc)
plugin->desc = desc;
+ if (init_plugin(desc) < 0) {
+ g_free(plugin);
+ return;
+ }
+
plugins = g_slist_insert_sorted(plugins, plugin, compare_priority);
+ DBG("Plugin %s loaded", desc->name);
}
static gboolean enable_plugin(const char *name, char **cli_enable,
@@ -181,7 +205,6 @@ static void external_plugin_init(char **cli_disabled, char **cli_enabled)
void plugin_init(const char *enable, const char *disable)
{
- GSList *list;
char **cli_disabled = NULL;
char **cli_enabled = NULL;
unsigned int i;
@@ -208,24 +231,6 @@ void plugin_init(const char *enable, const char *disable)
external_plugin_init(cli_enabled, cli_disabled);
- for (list = plugins; list; list = list->next) {
- struct bluetooth_plugin *plugin = list->data;
- int err;
-
- err = plugin->desc->init();
- if (err < 0) {
- if (err == -ENOSYS || err == -ENOTSUP)
- warn("System does not support %s plugin",
- plugin->desc->name);
- else
- error("Failed to init %s plugin",
- plugin->desc->name);
- continue;
- }
-
- plugin->active = TRUE;
- }
-
g_strfreev(cli_enabled);
g_strfreev(cli_disabled);
}
@@ -239,7 +244,7 @@ void plugin_cleanup(void)
for (list = plugins; list; list = list->next) {
struct bluetooth_plugin *plugin = list->data;
- if (plugin->active == TRUE && plugin->desc->exit)
+ if (plugin->desc->exit)
plugin->desc->exit();
#if EXTERNAL_PLUGINS
--
2.43.0
From: Emil Velikov <[email protected]>
As a whole all plugins should be built-in, otherwise they would be using
internal, undocumented, unversioned, unstable API.
Flesh out the external plugin support into a few pre-processor blocks
and simplify the normal path.
Hide the internal API (omit export-dynamic) when built without external
plugins.
---
Makefile.obexd | 2 ++
obexd/src/obexd.h | 2 +-
obexd/src/plugin.c | 93 ++++++++++++++++++++++++++++++++++++------------------
obexd/src/plugin.h | 4 +++
4 files changed, 70 insertions(+), 31 deletions(-)
diff --git a/Makefile.obexd b/Makefile.obexd
index 5d1a4ff65..9175de2a4 100644
--- a/Makefile.obexd
+++ b/Makefile.obexd
@@ -86,7 +86,9 @@ obexd_src_obexd_LDADD = lib/libbluetooth-internal.la \
$(ICAL_LIBS) $(DBUS_LIBS) $(LIBEBOOK_LIBS) \
$(LIBEDATASERVER_LIBS) $(GLIB_LIBS) -ldl
+if EXTERNAL_PLUGINS
obexd_src_obexd_LDFLAGS = $(AM_LDFLAGS) -Wl,--export-dynamic
+endif
obexd_src_obexd_CPPFLAGS = $(AM_CPPFLAGS) $(GLIB_CFLAGS) $(DBUS_CFLAGS) \
$(ICAL_CFLAGS) -DOBEX_PLUGIN_BUILTIN \
diff --git a/obexd/src/obexd.h b/obexd/src/obexd.h
index fe312a65b..af5265da5 100644
--- a/obexd/src/obexd.h
+++ b/obexd/src/obexd.h
@@ -18,7 +18,7 @@
#define OBEX_MAS (1 << 8)
#define OBEX_MNS (1 << 9)
-gboolean plugin_init(const char *pattern, const char *exclude);
+void plugin_init(const char *pattern, const char *exclude);
void plugin_cleanup(void);
gboolean manager_init(void);
diff --git a/obexd/src/plugin.c b/obexd/src/plugin.c
index a3eb24753..212299fa5 100644
--- a/obexd/src/plugin.c
+++ b/obexd/src/plugin.c
@@ -37,11 +37,14 @@
static GSList *plugins = NULL;
struct obex_plugin {
+#if EXTERNAL_PLUGINS
void *handle;
+#endif
const struct obex_plugin_desc *desc;
};
-static gboolean add_plugin(void *handle, const struct obex_plugin_desc *desc)
+#if EXTERNAL_PLUGINS
+static gboolean add_external_plugin(void *handle, const struct obex_plugin_desc *desc)
{
struct obex_plugin *plugin;
@@ -65,6 +68,26 @@ static gboolean add_plugin(void *handle, const struct obex_plugin_desc *desc)
return TRUE;
}
+#endif
+
+static void add_plugin(const struct obex_plugin_desc *desc)
+{
+ struct obex_plugin *plugin;
+
+ plugin = g_try_new0(struct obex_plugin, 1);
+ if (plugin == NULL)
+ return;
+
+ plugin->desc = desc;
+
+ if (desc->init() < 0) {
+ g_free(plugin);
+ return;
+ }
+
+ plugins = g_slist_append(plugins, plugin);
+ DBG("Plugin %s loaded", desc->name);
+}
static gboolean check_plugin(const struct obex_plugin_desc *desc,
char **patterns, char **excludes)
@@ -93,42 +116,23 @@ static gboolean check_plugin(const struct obex_plugin_desc *desc,
}
-#include "builtin.h"
-
-gboolean plugin_init(const char *pattern, const char *exclude)
+static void external_plugin_init(char **patterns, char **excludes)
{
- char **patterns = NULL;
- char **excludes = NULL;
+#if EXTERNAL_PLUGINS
GDir *dir;
const char *file;
- unsigned int i;
- if (strlen(PLUGINDIR) == 0)
- return FALSE;
-
- if (pattern)
- patterns = g_strsplit_set(pattern, ":, ", -1);
-
- if (exclude)
- excludes = g_strsplit_set(exclude, ":, ", -1);
-
- DBG("Loading builtin plugins");
-
- for (i = 0; __obex_builtin[i]; i++) {
- if (check_plugin(__obex_builtin[i],
- patterns, excludes) == FALSE)
- continue;
+ warn("Using external plugins is not officially supported.\n");
+ warn("Consider upstreaming your plugins into the BlueZ project.");
- add_plugin(NULL, __obex_builtin[i]);
- }
+ if (strlen(PLUGINDIR) == 0)
+ return;
DBG("Loading plugins %s", PLUGINDIR);
dir = g_dir_open(PLUGINDIR, 0, NULL);
if (!dir) {
- g_strfreev(patterns);
- g_strfreev(excludes);
- return FALSE;
+ return;
}
while ((file = g_dir_read_name(dir)) != NULL) {
@@ -164,15 +168,42 @@ gboolean plugin_init(const char *pattern, const char *exclude)
continue;
}
- if (add_plugin(handle, desc) == FALSE)
+ if (add_external_plugin(handle, desc) == FALSE)
dlclose(handle);
}
g_dir_close(dir);
+#endif
+}
+
+#include "builtin.h"
+
+void plugin_init(const char *pattern, const char *exclude)
+{
+ char **patterns = NULL;
+ char **excludes = NULL;
+ unsigned int i;
+
+ if (pattern)
+ patterns = g_strsplit_set(pattern, ":, ", -1);
+
+ if (exclude)
+ excludes = g_strsplit_set(exclude, ":, ", -1);
+
+ DBG("Loading builtin plugins");
+
+ for (i = 0; __obex_builtin[i]; i++) {
+ if (check_plugin(__obex_builtin[i],
+ patterns, excludes) == FALSE)
+ continue;
+
+ add_plugin(__obex_builtin[i]);
+ }
+
+ external_plugin_init(patterns, excludes);
+
g_strfreev(patterns);
g_strfreev(excludes);
-
- return TRUE;
}
void plugin_cleanup(void)
@@ -187,8 +218,10 @@ void plugin_cleanup(void)
if (plugin->desc->exit)
plugin->desc->exit();
+#if EXTERNAL_PLUGINS
if (plugin->handle != NULL)
dlclose(plugin->handle);
+#endif
g_free(plugin);
}
diff --git a/obexd/src/plugin.h b/obexd/src/plugin.h
index a91746cbc..e1756b9bf 100644
--- a/obexd/src/plugin.h
+++ b/obexd/src/plugin.h
@@ -20,10 +20,14 @@ struct obex_plugin_desc {
#name, init, exit \
};
#else
+#if EXTERNAL_PLUGINS
#define OBEX_PLUGIN_DEFINE(name,init,exit) \
extern struct obex_plugin_desc obex_plugin_desc \
__attribute__ ((visibility("default"))); \
const struct obex_plugin_desc obex_plugin_desc = { \
#name, init, exit \
};
+#else
+#error "Requested non built-in plugin, while external plugins is disabled"
+#endif
#endif
--
2.43.0
From: Emil Velikov <[email protected]>
... when building without external plugins.
---
Makefile.am | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/Makefile.am b/Makefile.am
index 1b82e8551..b98519e84 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -293,7 +293,6 @@ pkglibexec_PROGRAMS += src/bluetoothd
src_bluetoothd_SOURCES = $(builtin_sources) \
$(attrib_sources) $(btio_sources) \
- src/bluetooth.ver \
src/main.c src/log.h src/log.c \
src/backtrace.h src/backtrace.c \
src/rfkill.c src/btd.h src/sdpd.h \
@@ -325,8 +324,12 @@ src_bluetoothd_LDADD = lib/libbluetooth-internal.la \
src/libshared-glib.la \
$(BACKTRACE_LIBS) $(GLIB_LIBS) $(DBUS_LIBS) -ldl -lrt \
$(builtin_ldadd)
+
+if EXTERNAL_PLUGINS
+src_bluetoothd_SOURCES += src/bluetooth.ver
src_bluetoothd_LDFLAGS = $(AM_LDFLAGS) -Wl,--export-dynamic \
-Wl,--version-script=$(srcdir)/src/bluetooth.ver
+endif
src_bluetoothd_DEPENDENCIES = lib/libbluetooth-internal.la \
gdbus/libgdbus-internal.la \
--
2.43.0
Hi,
On Thu, 25 Jan 2024 at 01:07, Emil Velikov via B4 Relay
<[email protected]> wrote:
>
> Hello everyone,
>
> Here's v3 fixing a small bug with the previous patches, which was
> tripping the CI.
>
> Link to the previous revision can be found below.
Just a comment that external plugins support was added to avoid udev
dependency (from sixaxis) in bluetoothd.
(not that I have strong opinion on this, just a note, I don't remember
exactly why it was done, maybe Marcel recalls?)
>
> Thanks
> Emil
>
> - Link to v2: https://lore.kernel.org/r/[email protected]
>
> ---
> Emil Velikov (8):
> configure, README: introduce --enable-external-plugins
> obexd: factor out external plugin support
> bluetoothd: remove external-dummy plugin
> bluetoothd: convert external sixaxis plugin to builtin
> bluetoothd: factor out external plugin support
> bluetoothd: don't export internal API
> bluetoothd: change plugin loading alike obexd
> android: export only (android) entrypoint from the modules
>
> Makefile.am | 17 ++----
> Makefile.obexd | 2 +
> Makefile.plugins | 8 +--
> README | 13 +++++
> android/Makefile.am | 3 +
> android/hal-audio.c | 1 +
> android/hal-bluetooth.c | 1 +
> android/hal-sco.c | 1 +
> configure.ac | 10 ++++
> obexd/src/obexd.h | 2 +-
> obexd/src/plugin.c | 93 +++++++++++++++++++++----------
> obexd/src/plugin.h | 4 ++
> plugins/external-dummy.c | 28 ----------
> src/btd.h | 2 +-
> src/plugin.c | 140 +++++++++++++++++++++++++++++------------------
> src/plugin.h | 16 ++++++
> 16 files changed, 209 insertions(+), 132 deletions(-)
> ---
> base-commit: a9d1f6f6a625607de6c3f5b7a40a3aac5f36c02b
> change-id: 20240116-rm-ext-plugins-ba0b852a492b
>
> Best regards,
> --
> Emil Velikov <[email protected]>
>
>
--
pozdrawiam
Szymon K. Janc
On Thu, 25 Jan 2024 at 13:02, Szymon Janc <[email protected]> wrote:
>
> Hi,
>
> On Thu, 25 Jan 2024 at 01:07, Emil Velikov via B4 Relay
> <[email protected]> wrote:
> >
> > Hello everyone,
> >
> > Here's v3 fixing a small bug with the previous patches, which was
> > tripping the CI.
> >
> > Link to the previous revision can be found below.
>
> Just a comment that external plugins support was added to avoid udev
> dependency (from sixaxis) in bluetoothd.
> (not that I have strong opinion on this, just a note, I don't remember
> exactly why it was done, maybe Marcel recalls?)
>
Thanks, I may have some ideas why.
About 10 years ago (or so) some distributions were shipping
libudev.so.0 while others libudev.so.1. The ABI break was minimal,
although it was a thing.
I remember us doing all sorts of hacks in Mesa trying to pick the
correct one, esp when your system can have .1 while the game (or its
chroot-like environment) has .0 and vice-versa.
I would imagine a similar issue was observed in bluez - but I can only
speculate.
Over the last 5+ years, literally all supported distributions have
moved for libudev.so.1 and the Steam games (and runtime) has both with
some compat quirks to avoid explosions.
HTH o/
Emil
Hi Emil,
On Thu, Jan 25, 2024 at 8:51 AM Emil Velikov <[email protected]> wrote:
>
> On Thu, 25 Jan 2024 at 13:02, Szymon Janc <[email protected]> wrote:
> >
> > Hi,
> >
> > On Thu, 25 Jan 2024 at 01:07, Emil Velikov via B4 Relay
> > <[email protected]> wrote:
> > >
> > > Hello everyone,
> > >
> > > Here's v3 fixing a small bug with the previous patches, which was
> > > tripping the CI.
> > >
> > > Link to the previous revision can be found below.
> >
> > Just a comment that external plugins support was added to avoid udev
> > dependency (from sixaxis) in bluetoothd.
> > (not that I have strong opinion on this, just a note, I don't remember
> > exactly why it was done, maybe Marcel recalls?)
> >
>
> Thanks, I may have some ideas why.
>
> About 10 years ago (or so) some distributions were shipping
> libudev.so.0 while others libudev.so.1. The ABI break was minimal,
> although it was a thing.
> I remember us doing all sorts of hacks in Mesa trying to pick the
> correct one, esp when your system can have .1 while the game (or its
> chroot-like environment) has .0 and vice-versa.
>
> I would imagine a similar issue was observed in bluez - but I can only
> speculate.
>
> Over the last 5+ years, literally all supported distributions have
> moved for libudev.so.1 and the Steam games (and runtime) has both with
> some compat quirks to avoid explosions.
I was considering applying this week but if you want to respin this
set to sort out the dependency Im fine with, but I don't think it
would hurt to have a libudev dependency provide we have some means to
disable it in case the system don't intend to support sixaxis plugin.
--
Luiz Augusto von Dentz
Hi Luiz,
On Thu, 25 Jan 2024 at 18:30, Luiz Augusto von Dentz
<[email protected]> wrote:
>
> Hi Emil,
>
> On Thu, Jan 25, 2024 at 8:51 AM Emil Velikov <[email protected]> wrote:
> >
> > On Thu, 25 Jan 2024 at 13:02, Szymon Janc <[email protected]> wrote:
> > >
> > > Hi,
> > >
> > > On Thu, 25 Jan 2024 at 01:07, Emil Velikov via B4 Relay
> > > <[email protected]> wrote:
> > > >
> > > > Hello everyone,
> > > >
> > > > Here's v3 fixing a small bug with the previous patches, which was
> > > > tripping the CI.
> > > >
> > > > Link to the previous revision can be found below.
> > >
> > > Just a comment that external plugins support was added to avoid udev
> > > dependency (from sixaxis) in bluetoothd.
> > > (not that I have strong opinion on this, just a note, I don't remember
> > > exactly why it was done, maybe Marcel recalls?)
> > >
> >
> > Thanks, I may have some ideas why.
> >
> > About 10 years ago (or so) some distributions were shipping
> > libudev.so.0 while others libudev.so.1. The ABI break was minimal,
> > although it was a thing.
> > I remember us doing all sorts of hacks in Mesa trying to pick the
> > correct one, esp when your system can have .1 while the game (or its
> > chroot-like environment) has .0 and vice-versa.
> >
> > I would imagine a similar issue was observed in bluez - but I can only
> > speculate.
> >
> > Over the last 5+ years, literally all supported distributions have
> > moved for libudev.so.1 and the Steam games (and runtime) has both with
> > some compat quirks to avoid explosions.
>
> I was considering applying this week but if you want to respin this
> set to sort out the dependency Im fine with, but I don't think it
> would hurt to have a libudev dependency provide we have some means to
> disable it in case the system don't intend to support sixaxis plugin.
>
I don't think I follow: what do you mean with "sort out the dependency"?
Sixaxis is no different to midi where it a) pulls a third-party
library (udev vs alsa) and b) it can be disabled at build. Technically
one can dlopen/dlsym libudev.so, although that should probably be
deferred until needed IMHO.
Thanks
Emil
Hi Emil,
On Wed, Jan 24, 2024 at 7:08 PM Emil Velikov via B4 Relay
<[email protected]> wrote:
>
> From: Emil Velikov <[email protected]>
>
> As a whole all plugins should be built-in, otherwise they would be using
> internal, undocumented, unversioned, unstable API.
>
> Flesh out the external plugin support into a few pre-processor blocks
> and simplify the normal path.
>
> Hide the internal API (omit export-dynamic) when built without external
> plugins.
> ---
> Makefile.obexd | 2 ++
> obexd/src/obexd.h | 2 +-
> obexd/src/plugin.c | 93 ++++++++++++++++++++++++++++++++++++------------------
> obexd/src/plugin.h | 4 +++
> 4 files changed, 70 insertions(+), 31 deletions(-)
>
> diff --git a/Makefile.obexd b/Makefile.obexd
> index 5d1a4ff65..9175de2a4 100644
> --- a/Makefile.obexd
> +++ b/Makefile.obexd
> @@ -86,7 +86,9 @@ obexd_src_obexd_LDADD = lib/libbluetooth-internal.la \
> $(ICAL_LIBS) $(DBUS_LIBS) $(LIBEBOOK_LIBS) \
> $(LIBEDATASERVER_LIBS) $(GLIB_LIBS) -ldl
>
> +if EXTERNAL_PLUGINS
> obexd_src_obexd_LDFLAGS = $(AM_LDFLAGS) -Wl,--export-dynamic
> +endif
>
> obexd_src_obexd_CPPFLAGS = $(AM_CPPFLAGS) $(GLIB_CFLAGS) $(DBUS_CFLAGS) \
> $(ICAL_CFLAGS) -DOBEX_PLUGIN_BUILTIN \
> diff --git a/obexd/src/obexd.h b/obexd/src/obexd.h
> index fe312a65b..af5265da5 100644
> --- a/obexd/src/obexd.h
> +++ b/obexd/src/obexd.h
> @@ -18,7 +18,7 @@
> #define OBEX_MAS (1 << 8)
> #define OBEX_MNS (1 << 9)
>
> -gboolean plugin_init(const char *pattern, const char *exclude);
> +void plugin_init(const char *pattern, const char *exclude);
> void plugin_cleanup(void);
>
> gboolean manager_init(void);
> diff --git a/obexd/src/plugin.c b/obexd/src/plugin.c
> index a3eb24753..212299fa5 100644
> --- a/obexd/src/plugin.c
> +++ b/obexd/src/plugin.c
> @@ -37,11 +37,14 @@
> static GSList *plugins = NULL;
>
> struct obex_plugin {
> +#if EXTERNAL_PLUGINS
> void *handle;
> +#endif
> const struct obex_plugin_desc *desc;
> };
>
> -static gboolean add_plugin(void *handle, const struct obex_plugin_desc *desc)
> +#if EXTERNAL_PLUGINS
> +static gboolean add_external_plugin(void *handle, const struct obex_plugin_desc *desc)
> {
> struct obex_plugin *plugin;
>
> @@ -65,6 +68,26 @@ static gboolean add_plugin(void *handle, const struct obex_plugin_desc *desc)
>
> return TRUE;
> }
> +#endif
> +
> +static void add_plugin(const struct obex_plugin_desc *desc)
> +{
> + struct obex_plugin *plugin;
> +
> + plugin = g_try_new0(struct obex_plugin, 1);
> + if (plugin == NULL)
> + return;
> +
> + plugin->desc = desc;
> +
> + if (desc->init() < 0) {
> + g_free(plugin);
> + return;
> + }
> +
> + plugins = g_slist_append(plugins, plugin);
> + DBG("Plugin %s loaded", desc->name);
> +}
>
> static gboolean check_plugin(const struct obex_plugin_desc *desc,
> char **patterns, char **excludes)
> @@ -93,42 +116,23 @@ static gboolean check_plugin(const struct obex_plugin_desc *desc,
> }
>
>
> -#include "builtin.h"
> -
> -gboolean plugin_init(const char *pattern, const char *exclude)
> +static void external_plugin_init(char **patterns, char **excludes)
> {
> - char **patterns = NULL;
> - char **excludes = NULL;
> +#if EXTERNAL_PLUGINS
> GDir *dir;
> const char *file;
> - unsigned int i;
>
> - if (strlen(PLUGINDIR) == 0)
> - return FALSE;
> -
> - if (pattern)
> - patterns = g_strsplit_set(pattern, ":, ", -1);
> -
> - if (exclude)
> - excludes = g_strsplit_set(exclude, ":, ", -1);
> -
> - DBG("Loading builtin plugins");
> -
> - for (i = 0; __obex_builtin[i]; i++) {
> - if (check_plugin(__obex_builtin[i],
> - patterns, excludes) == FALSE)
> - continue;
> + warn("Using external plugins is not officially supported.\n");
> + warn("Consider upstreaming your plugins into the BlueZ project.");
>
> - add_plugin(NULL, __obex_builtin[i]);
> - }
> + if (strlen(PLUGINDIR) == 0)
> + return;
>
> DBG("Loading plugins %s", PLUGINDIR);
>
> dir = g_dir_open(PLUGINDIR, 0, NULL);
> if (!dir) {
> - g_strfreev(patterns);
> - g_strfreev(excludes);
> - return FALSE;
> + return;
> }
>
> while ((file = g_dir_read_name(dir)) != NULL) {
> @@ -164,15 +168,42 @@ gboolean plugin_init(const char *pattern, const char *exclude)
> continue;
> }
>
> - if (add_plugin(handle, desc) == FALSE)
> + if (add_external_plugin(handle, desc) == FALSE)
> dlclose(handle);
> }
>
> g_dir_close(dir);
> +#endif
> +}
> +
> +#include "builtin.h"
> +
> +void plugin_init(const char *pattern, const char *exclude)
> +{
> + char **patterns = NULL;
> + char **excludes = NULL;
> + unsigned int i;
> +
> + if (pattern)
> + patterns = g_strsplit_set(pattern, ":, ", -1);
> +
> + if (exclude)
> + excludes = g_strsplit_set(exclude, ":, ", -1);
> +
> + DBG("Loading builtin plugins");
> +
> + for (i = 0; __obex_builtin[i]; i++) {
> + if (check_plugin(__obex_builtin[i],
> + patterns, excludes) == FALSE)
> + continue;
> +
> + add_plugin(__obex_builtin[i]);
> + }
> +
> + external_plugin_init(patterns, excludes);
> +
> g_strfreev(patterns);
> g_strfreev(excludes);
> -
> - return TRUE;
> }
>
> void plugin_cleanup(void)
> @@ -187,8 +218,10 @@ void plugin_cleanup(void)
> if (plugin->desc->exit)
> plugin->desc->exit();
>
> +#if EXTERNAL_PLUGINS
> if (plugin->handle != NULL)
> dlclose(plugin->handle);
> +#endif
>
> g_free(plugin);
> }
> diff --git a/obexd/src/plugin.h b/obexd/src/plugin.h
> index a91746cbc..e1756b9bf 100644
> --- a/obexd/src/plugin.h
> +++ b/obexd/src/plugin.h
> @@ -20,10 +20,14 @@ struct obex_plugin_desc {
> #name, init, exit \
> };
> #else
> +#if EXTERNAL_PLUGINS
> #define OBEX_PLUGIN_DEFINE(name,init,exit) \
> extern struct obex_plugin_desc obex_plugin_desc \
> __attribute__ ((visibility("default"))); \
> const struct obex_plugin_desc obex_plugin_desc = { \
> #name, init, exit \
> };
> +#else
> +#error "Requested non built-in plugin, while external plugins is disabled"
> +#endif
> #endif
>
> --
> 2.43.0
How about we do something like:
https://gist.github.com/Vudentz/0b8bcb78a8898614fc4848cbf44a0a9f
That way we leave it to the compiler to remove the dead code when
linking but it still build checks which catches errors such as:
make --no-print-directory all-am
CC obexd/src/obexd-plugin.o
obexd/src/plugin.c: In function ‘external_plugin_init’:
obexd/src/plugin.c:123:9: error: implicit declaration of function
‘warn’ [-Werror=implicit-function-declaration]
123 | warn("Using external plugins is not officially supported.\n");
| ^~~~
>
--
Luiz Augusto von Dentz
Hi Luiz,
On Fri, 26 Jan 2024 at 18:50, Luiz Augusto von Dentz
<[email protected]> wrote:>> How about we do something like:
>
> https://gist.github.com/Vudentz/0b8bcb78a8898614fc4848cbf44a0a9f
>
> That way we leave it to the compiler to remove the dead code when
> linking but it still build checks which catches errors such as:
>
Not sure why I did not use that from the start. Considering I've done
similar changes in the kernel :facepalm:
Thanks an updated series is on the list,
-Emil