Received-SPF: pass (google.com: domain of linux-bluetooth+bounces-1454-linux.lists.archive=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) client-ip=139.178.88.99;
Precedence: bulk
MIME-Version: 1.0
References: <20240129-rm-ext-plugins-v4-0-bfd1e08c7f99@gmail.com>
 <20240129-rm-ext-plugins-v4-7-bfd1e08c7f99@gmail.com> <a97398e2-856b-4034-b8d3-e65f54b2bb17@molgen.mpg.de>
In-Reply-To: <a97398e2-856b-4034-b8d3-e65f54b2bb17@molgen.mpg.de>
From: Luiz Augusto von Dentz <luiz.dentz@gmail.com>
Date: Mon, 29 Jan 2024 11:22:30 -0500
Message-ID: <CABBYNZJY0-hfvUEM101fE4WogdH_cosP65_ai=QUKUcs57JFUA@mail.gmail.com>
Subject: Re: [PATCH BlueZ v4 7/8] bluetoothd: change plugin loading alike obexd
To: Paul Menzel <pmenzel@molgen.mpg.de>
Cc: Emil Velikov <emil.velikov@collabora.com>, linux-bluetooth@vger.kernel.org, 
	emil.l.velikov@gmail.com
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

Hi Paul,

On Mon, Jan 29, 2024 at 10:37=E2=80=AFAM 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 th=
e
> > 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=E2=
=80=99s
> 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 =3D NULL;
> >
> >   struct bluetooth_plugin {
> >       void *handle;
> > -     gboolean active;
> >       const struct bluetooth_plugin_desc *desc;
> >   };
> >
> > @@ -44,6 +43,22 @@ static int compare_priority(gconstpointer a, gconstp=
ointer b)
> >       return plugin2->desc->priority - plugin1->desc->priority;
> >   }
> >
> > +static int init_plugin(const struct bluetooth_plugin_desc *desc)
> > +{
> > +     int err;
> > +
> > +     err =3D desc->init();
> > +     if (err < 0) {
> > +             if (err =3D=3D -ENOSYS || err =3D=3D -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 =3D g_try_new0(struct bluetooth_plugin, 1);
> >       if (plugin =3D=3D NULL)
> >               return FALSE;
> >
> >       plugin->handle =3D handle;
> > -     plugin->active =3D FALSE;
> >       plugin->desc =3D desc;
> >
> > +     if (init_plugin(desc) < 0) {
> > +             g_free(plugin);
> > +             return FALSE;
> > +     }
> > +
> >       __btd_enable_debug(desc->debug_start, desc->debug_stop);
> >
> >       plugins =3D g_slist_insert_sorted(plugins, plugin, compare_priori=
ty);
> > +     DBG("Plugin %s loaded", desc->name);
> >
> >       return TRUE;
> >   }
> > @@ -86,7 +104,13 @@ static void add_plugin(const struct bluetooth_plugi=
n_desc *desc)
> >
> >       plugin->desc =3D desc;
> >
> > +     if (init_plugin(desc) < 0) {
> > +             g_free(plugin);
> > +             return;
> > +     }
> > +
> >       plugins =3D g_slist_insert_sorted(plugins, plugin, compare_priori=
ty);
> > +     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_disable=
d, char **cli_enabled)
> >
> >   void plugin_init(const char *enable, const char *disable)
> >   {
> > -     GSList *list;
> >       char **cli_disabled =3D NULL;
> >       char **cli_enabled =3D NULL;
> >       unsigned int i;
> > @@ -205,24 +228,6 @@ void plugin_init(const char *enable, const char *d=
isable)
> >       if IS_ENABLED(EXTERNAL_PLUGINS)
> >               external_plugin_init(cli_enabled, cli_disabled);
> >
> > -     for (list =3D plugins; list; list =3D list->next) {
> > -             struct bluetooth_plugin *plugin =3D list->data;
> > -             int err;
> > -
> > -             err =3D plugin->desc->init();
> > -             if (err < 0) {
> > -                     if (err =3D=3D -ENOSYS || err =3D=3D -ENOTSUP)
> > -                             warn("System does not support %s plugin",
> > -                                                     plugin->desc->nam=
e);
> > -                     else
> > -                             error("Failed to init %s plugin",
> > -                                                     plugin->desc->nam=
e);
> > -                     continue;
> > -             }
> > -
> > -             plugin->active =3D TRUE;
> > -     }
> > -
> >       g_strfreev(cli_enabled);
> >       g_strfreev(cli_disabled);
> >   }
> > @@ -236,7 +241,7 @@ void plugin_cleanup(void)
> >       for (list =3D plugins; list; list =3D list->next) {
> >               struct bluetooth_plugin *plugin =3D list->data;
> >
> > -             if (plugin->active =3D=3D TRUE && plugin->desc->exit)
> > +             if (plugin->desc->exit)
> >                       plugin->desc->exit();
> >
> >               if (plugin->handle !=3D NULL)
> >
>


--=20
Luiz Augusto von Dentz