| Message ID | 20210906140250.Bluez.v2.1.Id597e5ae87e680e6a744a8ed08d5000aacfce867@changeid |
|---|---|
| State | New |
| Headers | show |
| Series | [Bluez,v2,1/2] plugins/admin: add adapter_remove handler | expand |
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 >
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. -- Luiz Augusto von Dentz
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.
Thanks for catching and delivering the fixes. I didn't consider the multiple adapters case, and I will send out another patch to fix it. On Fri, Sep 17, 2021 at 7:50 AM Luiz Augusto von Dentz <luiz.dentz@gmail.com> wrote: > > 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
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,