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.
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(-)
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)
--
2.43.0
Dear Emil,
Thank you for your patches.
Am 29.01.24 um 15:44 schrieb Emil Velikov via B4 Relay:
> 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?
> 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 <[email protected]> wrote:
>
> Dear Emil,
>
>
> Thank you for your patches.
>
> Am 29.01.24 um 15:44 schrieb Emil Velikov via B4 Relay:
> > 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?
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)
> >
>
--
Luiz Augusto von Dentz
Hello team,
On Mon, 29 Jan 2024 at 16:22, Luiz Augusto von Dentz
<[email protected]> 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