| Message ID | 20240129-rm-ext-plugins-v4-7-bfd1e08c7f99@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | Remove support for external plugins | expand |
Dear Emil, Thank you for your patches. Am 29.01.24 um 15:44 schrieb Emil Velikov via B4 Relay: > From: Emil Velikov <emil.velikov@collabora.com> > > 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? > 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 > --- > src/plugin.c | 53 +++++++++++++++++++++++++++++------------------------ > 1 file changed, 29 insertions(+), 24 deletions(-) > > diff --git a/src/plugin.c b/src/plugin.c > index b6a84299a..e6d05be4c 100644 > --- a/src/plugin.c > +++ b/src/plugin.c > @@ -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) >
Hello team, On Mon, 29 Jan 2024 at 16:22, Luiz Augusto von Dentz <luiz.dentz@gmail.com> wrote: > > 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
diff --git a/src/plugin.c b/src/plugin.c index b6a84299a..e6d05be4c 100644 --- a/src/plugin.c +++ b/src/plugin.c @@ -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)
From: Emil Velikov <emil.velikov@collabora.com> 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(-)