[BlueZ,v3,2/8] obexd: factor out external plugin support

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(-)

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