Remove support for external plugins
Subsystems (2)
v4 cover Remove support for external plugins
Hello everyone,
With v4 we have moved from pre-processor to compilation checking for the external plugins support.
Namely, as we build without one the dead-code elimination will discard all the relevant code. Ultimately this means we compile check both paths in order to catch mistakes. Thanks to Luiz for the suggestion.
Link to the previous revision can be found below.
Thanks
Emil
- Link to v3: 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 | 89 ++++++++++++++++++++----------- obexd/src/plugin.h | 4 ++ plugins/external-dummy.c | 28 ---------- src/btd.h | 2 +- src/plugin.c | 135 ++++++++++++++++++++++++++++------------------- src/plugin.h | 4 ++ 16 files changed, 188 insertions(+), 132 deletions(-)
---
base-commit: 0de32f67f685b95c35a5c2f1206081af89bd88b6
change-id: 20240116-rm-ext-plugins-ba0b852a492b
Best regards,
Signature · 1 line
Emil Velikov <[email protected]>
PATCH v4 1/8 configure, README: introduce --enable-external-plugins
From: Emil Velikov <[email protected]>
As the README chunk says, disabled by default, since they rely on internal API/ABI and can break at any point.
Instead everyone affected should work and upstream their plugin into the bluez project.
--- README | 13 +++++++++++++ configure.ac | 10 ++++++++++ 2 files changed, 23 insertions(+)
README +13 -0
@@ -249,6 +249,19 @@ For a working system, certain configuration options need to be enabled: systems. The behavior of the deprecated tools may be unstable or simply don't work anymore. + --enable-external-plugins + + Enable support for external plugins + + By default external plugins for bluetoothd and obexd are not + supported and thus disabled. + + External plugins require access to internal, undocumented and + unversioned API in said daemons. As such they can break at any + time. If you have such plugins, enable this option and work + actively with the community to make said plugin part of the + upstream bluez project. + --enable-nfc This option enable NFC pairing support.
configure.ac +10 -0
@@ -364,6 +364,16 @@ AC_ARG_ENABLE(deprecated, AS_HELP_STRING([--enable-deprecated], [enable_deprecated=${enableval}]) AM_CONDITIONAL(DEPRECATED, test "${enable_deprecated}" = "yes") +AC_ARG_ENABLE(external-plugsin, AS_HELP_STRING([--enable-external-plugins], + [enable support for external plugins]), + [enable_external_plugins=${enableval}]) +AM_CONDITIONAL(EXTERNAL_PLUGINS, test "${enable_external_plugins}" = "yes") +if (test "${enable_external_plugins}" = "yes"); then + AC_DEFINE(EXTERNAL_PLUGINS, 1, [Define if external plugin support is required]) +else + AC_DEFINE(EXTERNAL_PLUGINS, 0, [Define if external plugin support is required]) +fi + AC_ARG_ENABLE(sixaxis, AS_HELP_STRING([--enable-sixaxis], [enable sixaxis plugin]), [enable_sixaxis=${enableval}]) AM_CONDITIONAL(SIXAXIS, test "${enable_sixaxis}" = "yes" &&
Signature · 1 line
2.43.0
This is automated email and please do not reply to this email!
Dear submitter,
Thank you for submitting the patches to the linux bluetooth mailing list. This is a CI test results with your patch series:
PW Link:https://patchwork.kernel.org/project/bluetooth/list/?series=820935
---Test result---
Test Summary:
CheckPatch FAIL 3.64 seconds GitLint PASS 2.33 seconds BuildEll PASS 24.08 seconds BluezMake PASS 732.29 seconds MakeCheck PASS 13.76 seconds MakeDistcheck PASS 164.26 seconds CheckValgrind PASS 226.32 seconds CheckSmatch PASS 329.54 seconds bluezmakeextell PASS 107.29 seconds IncrementalBuild PASS 5502.72 seconds ScanBuild PASS 928.77 seconds
Details
##############################
Test: CheckPatch - FAIL
Desc: Run checkpatch.pl script
Output:
[BlueZ,v4,2/8] obexd: factor out external plugin support WARNING:IS_ENABLED_CONFIG: IS_ENABLED(x) is normally used as IS_ENABLED(CONFIG_x) #127: FILE: obexd/src/plugin.c:37:
+#define IS_ENABLED(x) (x)
WARNING:IS_ENABLED_CONFIG: IS_ENABLED(EXTERNAL_PLUGINS) is normally used as IS_ENABLED(CONFIG_EXTERNAL_PLUGINS) #253: FILE: obexd/src/plugin.c:200:
+ if IS_ENABLED(EXTERNAL_PLUGINS)
/github/workspace/src/src/13535810.patch total: 0 errors, 2 warnings, 166 lines checked
NOTE: For some of the reported defects, checkpatch may be able to
mechanically convert to the typical style using --fix or --fix-inplace.
/github/workspace/src/src/13535810.patch has style problems, please review.
NOTE: Ignored message types: COMMIT_MESSAGE COMPLEX_MACRO CONST_STRUCT FILE_PATH_CHANGES MISSING_SIGN_OFF PREFER_PACKED SPDX_LICENSE_TAG SPLIT_STRING SSCANF_TO_KSTRTO
NOTE: If any of the errors are false positives, please report
them to the maintainer, see CHECKPATCH in MAINTAINERS.
[BlueZ,v4,5/8] bluetoothd: factor out external plugin support WARNING:IS_ENABLED_CONFIG: IS_ENABLED(x) is normally used as IS_ENABLED(CONFIG_x) #127: FILE: src/plugin.c:29:
+#define IS_ENABLED(x) (x)
WARNING:BLOCK_COMMENT_STYLE: Block comments use a trailing */ on a separate line #239: FILE: src/plugin.c:186:
+ * plugins are loaded. */
WARNING:IS_ENABLED_CONFIG: IS_ENABLED(EXTERNAL_PLUGINS) is normally used as IS_ENABLED(CONFIG_EXTERNAL_PLUGINS) #258: FILE: src/plugin.c:205:
+ if IS_ENABLED(EXTERNAL_PLUGINS)
/github/workspace/src/src/13535813.patch total: 0 errors, 3 warnings, 178 lines checked
NOTE: For some of the reported defects, checkpatch may be able to
mechanically convert to the typical style using --fix or --fix-inplace.
/github/workspace/src/src/13535813.patch has style problems, please review.
NOTE: Ignored message types: COMMIT_MESSAGE COMPLEX_MACRO CONST_STRUCT FILE_PATH_CHANGES MISSING_SIGN_OFF PREFER_PACKED SPDX_LICENSE_TAG SPLIT_STRING SSCANF_TO_KSTRTO
NOTE: If any of the errors are false positives, please report
them to the maintainer, see CHECKPATCH in MAINTAINERS.
[BlueZ,v4,7/8] bluetoothd: change plugin loading alike obexd WARNING:ENOSYS: ENOSYS means 'invalid syscall nr' and nothing else #108: FILE: src/plugin.c:52:
+ if (err == -ENOSYS || err == -ENOTSUP)
/github/workspace/src/src/13535815.patch total: 0 errors, 1 warnings, 106 lines checked
NOTE: For some of the reported defects, checkpatch may be able to
mechanically convert to the typical style using --fix or --fix-inplace.
/github/workspace/src/src/13535815.patch has style problems, please review.
NOTE: Ignored message types: COMMIT_MESSAGE COMPLEX_MACRO CONST_STRUCT FILE_PATH_CHANGES MISSING_SIGN_OFF PREFER_PACKED SPDX_LICENSE_TAG SPLIT_STRING SSCANF_TO_KSTRTO
NOTE: If any of the errors are false positives, please report
them to the maintainer, see CHECKPATCH in MAINTAINERS.
---
Regards,
Linux Bluetooth
PATCH v4 2/8 obexd: factor out external plugin support
In reply to Emil VelikovFrom: 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 blocks and simplify the normal path. Guard the external plugin support behind a runtime check, which will be dead-code eliminated in the default case.
Hide the internal API (omit export-dynamic) when built without external plugins.
--- Makefile.obexd | 2 ++ obexd/src/obexd.h | 2 +- obexd/src/plugin.c | 89 ++++++++++++++++++++++++++++++++++++------------------ obexd/src/plugin.h | 4 +++ 4 files changed, 66 insertions(+), 31 deletions(-)
Makefile.obexd +2 -0
@@ -92,7 +92,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 \
obexd/src/obexd.h +1 -1
@@ -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 );
obexd/src/plugin.c +59 -30
135 lines not on this page · raw message
obexd/src/plugin.h +4 -0
@@ -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
Signature · 1 line
2.43.0
PATCH v4 3/8 bluetoothd: remove external-dummy plugin
In reply to Emil VelikovFrom: 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(-)
Makefile.am +0 -8
@@ -289,14 +289,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) \
plugins/external-dummy.c deleted +0 -28
@@ -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)
Signature · 1 line
2.43.0
PATCH v4 4/8 bluetoothd: convert external sixaxis plugin to builtin
In reply to Emil VelikovFrom: 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(-)
Makefile.plugins +3 -5
@@ -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
Signature · 1 line
2.43.0
PATCH v4 5/8 bluetoothd: factor out external plugin support
In reply to Emil VelikovFrom: 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 and simplify the normal path. Guard the external plugin support behind a runtime check, which will be dead-code eliminated in the default case.
--- Makefile.am | 4 --- src/btd.h | 2 +- src/plugin.c | 92 +++++++++++++++++++++++++++++++++++++----------------------- src/plugin.h | 4 +++ 4 files changed, 62 insertions(+), 40 deletions(-)
Makefile.am +0 -4
@@ -51,11 +51,7 @@ EXTRA_DIST += 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 =
src/btd.h +1 -1
@@ -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 );
src/plugin.c +57 -35
145 lines not on this page · raw message
src/plugin.h +4 -0
@@ -28,6 +28,7 @@ struct bluetooth_plugin_desc {#name , version, priority, init, exit \ };#else +#if EXTERNAL_PLUGINS #define BLUETOOTH_PLUGIN_DEFINE (name, version, priority, init, exit) \ extern struct btd_debug_desc __start___debug[] \ __attribute__ ((weak, visibility("hidden"))); \
@@ -40,4 +41,7 @@ struct bluetooth_plugin_desc {#name , version, priority, init, exit, \ __start___debug, __stop___debug \ }; +#else +#error "Requested non built-in plugin, while external plugins is disabled" +#endif #endif
Signature · 1 line
2.43.0
PATCH v4 6/8 bluetoothd: don't export internal API
In reply to Emil VelikovFrom: Emil Velikov <[email protected]>
... when building without external plugins.
--- Makefile.am | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)
Makefile.am +4 -1
@@ -289,7 +289,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 \
@@ -321,8 +320,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_CPPFLAGS = $(AM_CPPFLAGS) -DBLUETOOTH_PLUGIN_BUILTIN \ -DPLUGINDIR=\""$(build_plugindir)"\" \
Signature · 1 line
2.43.0
PATCH v4 7/8 bluetoothd: change plugin loading alike obexd
In reply to Emil VelikovFrom: 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(-)
src/plugin.c +29 -24
@@ -32,7 +32,6 @@ static GSList *plugins = NULL;struct bluetooth_plugin {void *handle; -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; +} +static gboolean add_external_plugin (void *handle,const struct bluetooth_plugin_desc *desc) {
@@ -57,19 +72,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; }
@@ -86,7 +104,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,
@@ -177,7 +201,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;
@@ -205,24 +228,6 @@ void plugin_init(const char *enable, const char *disable) if IS_ENABLED(EXTERNAL_PLUGINS)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); }
@@ -236,7 +241,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 (plugin->handle !=NULL )
Signature · 1 line
2.43.0
Re: [PATCH BlueZ v4 7/8] bluetoothd: change plugin loading alike obexd
Dear Emil,
Thank you for your patches.
Am 29.01.24 um 15:44 schrieb Emil Velikov via B4 Relay:
Quoted 5 lines
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.
Excuse my ignorance, but would you be so kind to state the problem. It’s causing confusion to have Loading foobar, in case it fails? It clutters the output or uses unnecessory resources?
Quoted 2 lines
Do the
init()call early, print "Loaded" once it's actually successful and drop the no-longer "active" tracking.
It would help me, if you pasted the logs without and with your patch.
Kind regards,
Paul
Quoted 122 lines
--- src/plugin.c | 53 +++++++++++++++++++++++++++++------------------------ 1 file changed, 29 insertions(+), 24 deletions(-)src/plugin.c +29
-24@@ -32,7 +32,6 @@ static GSList *plugins = NULL;struct bluetooth_plugin {void *handle; -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; +} +static gboolean add_external_plugin (void *handle,const struct bluetooth_plugin_desc *desc) {@@ -57,19 +72,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; }@@ -86,7 +104,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,@@ -177,7 +201,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;@@ -205,24 +228,6 @@ void plugin_init(const char *enable, const char *disable) if IS_ENABLED(EXTERNAL_PLUGINS)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); }@@ -236,7 +241,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 (plugin->handle !=NULL )
Luiz Augusto von Dentz
maintainerRe: [PATCH BlueZ v4 7/8] bluetoothd: change plugin loading alike obexd
Hi Paul,
Quoted 9 lines · Paul Menzel
Dear Emil,
Thank you for your patches.
Am 29.01.24 um 15:44 schrieb Emil Velikov via B4 Relay:
Quoted 2 lines
From: Emil Velikov <[email protected]>
Quoted 7 lines
Quoted 3 lines
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.Excuse my ignorance, but would you be so kind to state the problem. It’s causing confusion to have
Loading foobar, in case it fails? It clutters the output or uses unnecessory resources?
To me it sounds quite clear that he is refering to the fact that obexd prints it a little differently so he is just trying to align the behavior of the daemons.
Quoted 134 lines
Quoted 2 lines
Do the
init()call early, print "Loaded" once it's actually successful and drop the no-longer "active" tracking.It would help me, if you pasted the logs without and with your patch.
Kind regards,
Paul
Quoted 122 lines
--- src/plugin.c | 53 +++++++++++++++++++++++++++++------------------------ 1 file changed, 29 insertions(+), 24 deletions(-)src/plugin.c +29
-24@@ -32,7 +32,6 @@ static GSList *plugins = NULL;struct bluetooth_plugin {void *handle; -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; +} +static gboolean add_external_plugin (void *handle,const struct bluetooth_plugin_desc *desc) {@@ -57,19 +72,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; }@@ -86,7 +104,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,@@ -177,7 +201,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;@@ -205,24 +228,6 @@ void plugin_init(const char *enable, const char *disable) if IS_ENABLED(EXTERNAL_PLUGINS)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); }@@ -236,7 +241,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 (plugin->handle !=NULL )
Signature · 1 line
Luiz Augusto von Dentz
Re: [PATCH BlueZ v4 7/8] bluetoothd: change plugin loading alike obexd
Hello team,
Quoted 5 lines · Luiz Augusto von Dentz
To me it sounds quite clear that he is refering to the fact that obexd prints it a little differently so he is just trying to align the behavior of the daemons.
Precisely - we do basic alignment across the two daemons. Both in terms of code as well as logging pattern.
Although if that's not wanted, please drop this patch. There's a reason why I kept it near the end of the series ;-)
Re-sending the series for the comma in 8/8 seems excessive IMHO. If there are other concerns, I'd be happy to respin the lot. Otherwise I think it's simpler if Luiz fixes it on applying.
Thanks
Emil
PATCH v4 8/8 android: export only (android) entrypoint from the modules
In reply to Emil VelikovFrom: 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(+)
android/Makefile.am +3 -0
@@ -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 \
android/hal-audio.c +1 -0
@@ -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 ,
android/hal-bluetooth.c +1 -0
@@ -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 ,
android/hal-sco.c +1 -0
@@ -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 ,
Signature · 1 line
2.43.0
Re: [PATCH BlueZ v4 8/8] android: export only (android) entrypoint from the modules
Dear Emil,
Thank you for your patches. One small nit.
Am 29.01.24 um 15:44 schrieb Emil Velikov via B4 Relay:
Quoted 3 lines
From: Emil Velikov <[email protected]>
The android specific modules, have a designated HMI entrypoint. Hide
The comma seems superfluous.
Quoted 7 lines
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(+)
[…]
Kind regards,
Paul
This series was applied to bluetooth/bluez.git (master) by Luiz Augusto von Dentz <[email protected]>:
Hello:
This series was applied to bluetooth/bluez.git (master) by Luiz Augusto von Dentz <[email protected]>:
Quoted 10 lines · On Mon, 29 Jan 2024 14:44:14 +0000 you wrote:
Hello everyone,
With v4 we have moved from pre-processor to compilation checking for the external plugins support.
Namely, as we build without one the dead-code elimination will discard all the relevant code. Ultimately this means we compile check both paths in order to catch mistakes. Thanks to Luiz for the suggestion.
[...]
Here is the summary with links:
- [BlueZ,v4,1/8] configure, README: introduce --enable-external-plugins
https://git.kernel.org/pub/scm/bluetooth/bluez.git/?id=2a5c9cf632e6 - [BlueZ,v4,2/8] obexd: factor out external plugin support https://git.kernel.org/pub/scm/bluetooth/bluez.git/?id=004b5b28a04c - [BlueZ,v4,3/8] bluetoothd: remove external-dummy plugin https://git.kernel.org/pub/scm/bluetooth/bluez.git/?id=1db7a00e35c4 - [BlueZ,v4,4/8] bluetoothd: convert external sixaxis plugin to builtin https://git.kernel.org/pub/scm/bluetooth/bluez.git/?id=9f71892b63f6 - [BlueZ,v4,5/8] bluetoothd: factor out external plugin support https://git.kernel.org/pub/scm/bluetooth/bluez.git/?id=109cc8a0f6aa - [BlueZ,v4,6/8] bluetoothd: don't export internal API https://git.kernel.org/pub/scm/bluetooth/bluez.git/?id=7a1d3c7c4bc7 - [BlueZ,v4,7/8] bluetoothd: change plugin loading alike obexd https://git.kernel.org/pub/scm/bluetooth/bluez.git/?id=f174724c76c6 - [BlueZ,v4,8/8] android: export only (android) entrypoint from the modules https://git.kernel.org/pub/scm/bluetooth/bluez.git/?id=f59f4902bc4e
You are awesome, thank you!
Signature · 2 lines
Deet-doot-dot, I am a bot. https://korg.docs.kernel.org/patchwork/pwbot.html