| Message ID | 20240125-rm-ext-plugins-v3-2-d141f7870bb6@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Remove support for external plugins | expand |
| Context | Check | Description |
|---|---|---|
| tedd_an/pre-ci_am | success | Success |
| tedd_an/CheckPatch | warning | WARNING:LONG_LINE: line length of 86 exceeds 80 columns #134: FILE: obexd/src/plugin.c:47: +static gboolean add_external_plugin(void *handle, const struct obex_plugin_desc *desc) /github/workspace/src/src/13529768.patch total: 0 errors, 1 warnings, 176 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/13529768.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. |
| tedd_an/GitLint | success | Gitlint PASS |
| tedd_an/IncrementalBuild | success | Incremental Build PASS |
Hi Emil, On Wed, Jan 24, 2024 at 7:08 PM Emil Velikov via B4 Relay <devnull+emil.l.velikov.gmail.com@kernel.org> wrote: > > From: Emil Velikov <emil.velikov@collabora.com> > > 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"); | ^~~~ >
Hi Luiz, On Fri, 26 Jan 2024 at 18:50, Luiz Augusto von Dentz <luiz.dentz@gmail.com> 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
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
From: Emil Velikov <emil.velikov@collabora.com> 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(-)