Received-SPF: pass (google.com: domain of linux-bluetooth+bounces-1451-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.48.161 as permitted sender) client-ip=147.75.48.161;
Message-ID: <a97398e2-856b-4034-b8d3-e65f54b2bb17@molgen.mpg.de>
Date: Mon, 29 Jan 2024 16:37:23 +0100
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH BlueZ v4 7/8] bluetoothd: change plugin loading alike
 obexd
To: Emil Velikov <emil.velikov@collabora.com>
Cc: linux-bluetooth@vger.kernel.org, emil.l.velikov@gmail.com
References: <20240129-rm-ext-plugins-v4-0-bfd1e08c7f99@gmail.com>
 <20240129-rm-ext-plugins-v4-7-bfd1e08c7f99@gmail.com>
Content-Language: en-US
From: Paul Menzel <pmenzel@molgen.mpg.de>
In-Reply-To: <20240129-rm-ext-plugins-v4-7-bfd1e08c7f99@gmail.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit

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