| Message ID | 20240129-rm-ext-plugins-v4-7-bfd1e08c7f99@gmail.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | f174724c76c6c8b91276f444142fed9c2537e5c1 |
| Headers | show |
| Series | Remove support for external plugins | expand |
| Context | Check | Description |
|---|---|---|
| tedd_an/pre-ci_am | success | Success |
| tedd_an/CheckPatch | warning | 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. |
| tedd_an/GitLint | success | Gitlint PASS |
| tedd_an/IncrementalBuild | success | Incremental Build PASS |
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) >
Hi Paul, On Mon, Jan 29, 2024 at 10:37 AM Paul Menzel <pmenzel@molgen.mpg.de> wrote: > > 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? 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. > > 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(-)