Received-SPF: pass (google.com: domain of linux-bluetooth-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <20210906140250.Bluez.v2.1.Id597e5ae87e680e6a744a8ed08d5000aacfce867@changeid>
 <CAPHZWUf91fsLi+fz8q3ocrgTfiq=+VFez75RbNQ4ckNL1jOeKw@mail.gmail.com> <CABBYNZK8iek+4Osyac=0Q4CU2gBwJvvCoJXh7xOXr2sBm4jzQw@mail.gmail.com>
In-Reply-To: <CABBYNZK8iek+4Osyac=0Q4CU2gBwJvvCoJXh7xOXr2sBm4jzQw@mail.gmail.com>
From:   Luiz Augusto von Dentz <luiz.dentz@gmail.com>
Date:   Thu, 16 Sep 2021 16:50:27 -0700
Message-ID: <CABBYNZLCR7y5SpcOGX7J+dsNBOEOd9K0w1B3ZsUUk9WZeXncEA@mail.gmail.com>
Subject: Re: [Bluez PATCH v2 1/2] plugins/admin: add adapter_remove handler
To:     Yun-hao Chung <howardchung@google.com>
Cc:     Bluez mailing list <linux-bluetooth@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

Hi Howard,

On Wed, Sep 15, 2021 at 4:40 PM Luiz Augusto von Dentz
<luiz.dentz@gmail.com> wrote:
>
> Hi Howard,
>
> On Wed, Sep 15, 2021 at 5:32 AM Yun-hao Chung <howardchung@google.com> wrote:
> >
> > Gentle ping. Thanks.
> >
> >
> > On Mon, Sep 6, 2021 at 2:03 PM Howard Chung <howardchung@google.com> wrote:
> > >
> > > From: Yun-Hao Chung <howardchung@chromium.org>
> > >
> > > Currently admin doesn't handle adapter removed callbacks, which causes
> > > interfaces AdminPolicySet1 and AdminPolicyStatus1 not being
> > > unregistered, which in turns causes these interfaces can not be
> > > re-registered once adapter is back.
> > >
> > > This adds handler for adapter_remove.
> > >
> > > Reviewed-by: Shyh-In Hwang <josephsih@chromium.org>
> > > Reviewed-by: Miao-chen Chou <mcchou@chromium.org>
> > > ---
> > > tested with following steps
> > > 1. rmmod btusb
> > > 2. modprobe btusb
> > > 3. read allowlist via bluetoothctl
> > >
> > > Changes in v2:
> > > 1. Fix make errors
> > >
> > >  plugins/admin.c | 35 ++++++++++++++++++++++++++++-------
> > >  1 file changed, 28 insertions(+), 7 deletions(-)
> > >
> > > diff --git a/plugins/admin.c b/plugins/admin.c
> > > index 02fec04568ba..82c00cabdb6b 100644
> > > --- a/plugins/admin.c
> > > +++ b/plugins/admin.c
> > > @@ -85,6 +85,17 @@ static void admin_policy_free(void *data)
> > >         g_free(admin_policy);
> > >  }
> > >
> > > +static void admin_policy_destroy(struct btd_admin_policy *admin_policy)
> > > +{
> > > +       const char *path = adapter_get_path(admin_policy->adapter);
> > > +
> > > +       g_dbus_unregister_interface(dbus_conn, path,
> > > +                                               ADMIN_POLICY_SET_INTERFACE);
> > > +       g_dbus_unregister_interface(dbus_conn, path,
> > > +                                               ADMIN_POLICY_STATUS_INTERFACE);
> > > +       admin_policy_free(admin_policy);
> > > +}
> > > +
> > >  static bool uuid_match(const void *data, const void *match_data)
> > >  {
> > >         const bt_uuid_t *uuid = data;
> > > @@ -492,7 +503,7 @@ static int admin_policy_adapter_probe(struct btd_adapter *adapter)
> > >         if (!g_dbus_register_interface(dbus_conn, adapter_path,
> > >                                         ADMIN_POLICY_SET_INTERFACE,
> > >                                         admin_policy_adapter_methods, NULL,
> > > -                                       NULL, policy_data, admin_policy_free)) {
> > > +                                       NULL, policy_data, NULL)) {
> > >                 btd_error(policy_data->adapter_id,
> > >                         "Admin Policy Set interface init failed on path %s",
> > >                                                                 adapter_path);
> > > @@ -506,7 +517,7 @@ static int admin_policy_adapter_probe(struct btd_adapter *adapter)
> > >                                         ADMIN_POLICY_STATUS_INTERFACE,
> > >                                         NULL, NULL,
> > >                                         admin_policy_adapter_properties,
> > > -                                       policy_data, admin_policy_free)) {
> > > +                                       policy_data, NULL)) {
> > >                 btd_error(policy_data->adapter_id,
> > >                         "Admin Policy Status interface init failed on path %s",
> > >                                                                 adapter_path);
> > > @@ -574,10 +585,24 @@ static void admin_policy_device_removed(struct btd_adapter *adapter,
> > >                 unregister_device_data(data, NULL);
> > >  }
> > >
> > > +static void admin_policy_remove(struct btd_adapter *adapter)
> > > +{
> > > +       DBG("");
> > > +
> > > +       queue_foreach(devices, unregister_device_data, NULL);
> > > +       queue_destroy(devices, g_free);
> > > +
> > > +       if (policy_data) {
> > > +               admin_policy_destroy(policy_data);
> > > +               policy_data = NULL;
> > > +       }
> > > +}
> > > +
> > >  static struct btd_adapter_driver admin_policy_driver = {
> > >         .name   = "admin_policy",
> > >         .probe  = admin_policy_adapter_probe,
> > >         .resume = NULL,
> > > +       .remove = admin_policy_remove,
> > >         .device_resolved = admin_policy_device_added,
> > >         .device_removed = admin_policy_device_removed
> > >  };
> > > @@ -597,11 +622,7 @@ static void admin_exit(void)
> > >         DBG("");
> > >
> > >         btd_unregister_adapter_driver(&admin_policy_driver);
> > > -       queue_foreach(devices, unregister_device_data, NULL);
> > > -       queue_destroy(devices, g_free);
> > > -
> > > -       if (policy_data)
> > > -               admin_policy_free(policy_data);
> > > +       admin_policy_remove(NULL);
> > >  }
> > >
> > >  BLUETOOTH_PLUGIN_DEFINE(admin, VERSION,
> > > --
> > > 2.33.0.153.gba50c8fa24-goog
> > >
>
> Applied, thanks.
>
>
> --

There are actually some problems with these admin plugin:

https://patchwork.kernel.org/project/bluetooth/patch/20210916223825.276530-1-luiz.dentz@gmail.com/
https://patchwork.kernel.org/project/bluetooth/patch/20210916223825.276530-2-luiz.dentz@gmail.com/

There is also some assumption that there could be only one
policy_data, when in fact there could be multiple btd_adapter in the
system so having admin plugin in a system with multiple adapters might
lead to various problems.


-- 
Luiz Augusto von Dentz