| Message ID | 20240927131441.2617450-1-quic_chejiang@quicinc.com |
|---|---|
| State | New |
| Headers | show |
| Series | [v1] Client: Fix the list_attributes command returning nothing for a dual-mode remote | expand |
This is automated email and please do not reply to this email! Dear submitter, Thank you for submitting the patches to the linux bluetooth mailing list. This is a CI test results with your patch series: PW Link:https://patchwork.kernel.org/project/bluetooth/list/?series=893475 ---Test result--- Test Summary: CheckPatch PASS 0.39 seconds GitLint FAIL 0.49 seconds BuildEll PASS 25.04 seconds BluezMake PASS 1799.90 seconds MakeCheck PASS 13.00 seconds MakeDistcheck PASS 180.98 seconds CheckValgrind PASS 252.88 seconds CheckSmatch PASS 359.66 seconds bluezmakeextell PASS 118.95 seconds IncrementalBuild PASS 1625.03 seconds ScanBuild PASS 1067.91 seconds Details ############################## Test: GitLint - FAIL Desc: Run gitlint Output: [v1] Client: Fix the list_attributes command returning nothing for a dual-mode remote WARNING: I3 - ignore-body-lines: gitlint will be switching from using Python regex 'match' (match beginning) to 'search' (match anywhere) semantics. Please review your ignore-body-lines.regex option accordingly. To remove this warning, set general.regex-style-search=True. More details: https://jorisroovers.github.io/gitlint/configuration/#regex-style-search 1: T1 Title exceeds max length (85>80): "[v1] Client: Fix the list_attributes command returning nothing for a dual-mode remote" --- Regards, Linux Bluetooth
Hi Luiz, On 9/27/2024 10:31 PM, Luiz Augusto von Dentz wrote: > Hi Cheng, > > On Fri, Sep 27, 2024 at 9:16 AM Cheng Jiang <quic_chejiang@quicinc.com> wrote: >> >> When a dual-mode device is paired first on BR/EDR and >> then on BLE through RPA, the RPA changes to a public >> address after receiving the IRK. This results in two proxies >> in default_ctrl->devices with the same public address. >> In cmd_list_attributes, if the BR/EDR proxy is found first, >> it prints no attributes. > > This seems to be a bug then, if we resolve the address and there is > already a device object for it then that shall be used instead of > keeping 2 different objects paths, fixing bluetoothctl to allow > multiple proxies with the same device won't do anything for other > clients so this is just a workaround. > > There seems to be some code for detecting and merging the objects: > > /* It is possible that we have two device objects for the same device in > * case it has first been discovered over BR/EDR and has a private > * address when discovered over LE for the first time. In such a case we > * need to inherit critical values from the duplicate so that we don't > * ovewrite them when writing to storage. The next time bluetoothd > * starts the device will show up as a single instance. > */ > void device_merge_duplicate(struct btd_device *dev, struct btd_device *dup) > > But it doesn't seem to carry over the services, etc, as it seems we > can't really just use one object at this point then both need to > interact with each other, perhaps by storing the duplicate into > btd_device so the right object can be used depending on the bearer, > etc. > Yes, this is just a workaround for the bluetoothctl client. The device_merge_duplicate can't handle it. Actually, there are two different dbus interfaces created for the two device objects. I didn't find a good way to merge these two dbus interface (I'm not familiar with dbus). Another thought is use the AddressType property to distinguish the two device objects. however, in current implementation, BR/EDR and BLE public address are both assumed as "public". Can we add a new string type (like "le_public") in `property_get_address_type` for BLE public address. Do you have any idea to get the bearer info in client? >> Modify cmd_list_attributes to search all proxies in >> default_ctrl->devices and display the related attributes. >> --- >> client/main.c | 55 +++++++++++++++++++++++++++++++++++++++++++++++---- >> 1 file changed, 51 insertions(+), 4 deletions(-) >> >> diff --git a/client/main.c b/client/main.c >> index 50aa3e7a6..17c1fb278 100644 >> --- a/client/main.c >> +++ b/client/main.c >> @@ -768,6 +768,29 @@ static GDBusProxy *find_proxy_by_address(GList *source, const char *address) >> return NULL; >> } >> >> +static GList *find_all_proxy_by_address(GList *source, const char *address) >> +{ >> + GList *list; >> + GList *all_proxy = NULL; >> + >> + for (list = g_list_first(source); list; list = g_list_next(list)) { >> + GDBusProxy *proxy = list->data; >> + DBusMessageIter iter; >> + const char *str; >> + >> + if (g_dbus_proxy_get_property(proxy, "Address", &iter) == FALSE) >> + continue; >> + >> + dbus_message_iter_get_basic(&iter, &str); >> + >> + if (!strcasecmp(str, address)) >> + all_proxy = g_list_append(all_proxy, proxy); >> + } >> + >> + return all_proxy; >> +} >> + >> + >> static gboolean check_default_ctrl(void) >> { >> if (!default_ctrl) { >> @@ -2051,7 +2074,9 @@ static void cmd_disconn(int argc, char *argv[]) >> >> static void cmd_list_attributes(int argc, char *argv[]) >> { >> - GDBusProxy *proxy; >> + GList *all_proxy = NULL; >> + GList *list; >> + GDBusProxy *proxy = NULL; >> const char *path; >> >> if (argc > 1 && !strcmp(argv[1], "local")) { >> @@ -2059,11 +2084,33 @@ static void cmd_list_attributes(int argc, char *argv[]) >> goto done; >> } >> >> - proxy = find_device(argc, argv); >> - if (!proxy) >> + if (argc < 2 || !strlen(argv[1])) { >> + if (default_dev) { >> + proxy = default_dev; >> + path = g_dbus_proxy_get_path(proxy); >> + goto done; >> + } >> + bt_shell_printf("Missing device address argument\n"); >> return bt_shell_noninteractive_quit(EXIT_FAILURE); >> + } else { >> + if (check_default_ctrl() == FALSE) >> + return bt_shell_noninteractive_quit(EXIT_FAILURE); >> >> - path = g_dbus_proxy_get_path(proxy); >> + all_proxy = find_all_proxy_by_address(default_ctrl->devices, >> + argv[1]); >> + if (!all_proxy) { >> + bt_shell_printf("Device %s not available\n", argv[1]); >> + return bt_shell_noninteractive_quit(EXIT_FAILURE); >> + } >> + for (list = g_list_first(all_proxy); list; >> + list = g_list_next(list)) { >> + proxy = list->data; >> + path = g_dbus_proxy_get_path(proxy); >> + gatt_list_attributes(path); >> + } >> + g_list_free(all_proxy); >> + return bt_shell_noninteractive_quit(EXIT_SUCCESS); >> + } >> >> done: >> gatt_list_attributes(path); >> -- >> 2.25.1 >> >> > >
Hi Cheng, On Sat, Sep 28, 2024 at 10:51 PM Cheng Jiang <quic_chejiang@quicinc.com> wrote: > > Hi Luiz, > > > On 9/27/2024 10:31 PM, Luiz Augusto von Dentz wrote: > > Hi Cheng, > > > > On Fri, Sep 27, 2024 at 9:16 AM Cheng Jiang <quic_chejiang@quicinc.com> wrote: > >> > >> When a dual-mode device is paired first on BR/EDR and > >> then on BLE through RPA, the RPA changes to a public > >> address after receiving the IRK. This results in two proxies > >> in default_ctrl->devices with the same public address. > >> In cmd_list_attributes, if the BR/EDR proxy is found first, > >> it prints no attributes. > > > > This seems to be a bug then, if we resolve the address and there is > > already a device object for it then that shall be used instead of > > keeping 2 different objects paths, fixing bluetoothctl to allow > > multiple proxies with the same device won't do anything for other > > clients so this is just a workaround. > > > > There seems to be some code for detecting and merging the objects: > > > > /* It is possible that we have two device objects for the same device in > > * case it has first been discovered over BR/EDR and has a private > > * address when discovered over LE for the first time. In such a case we > > * need to inherit critical values from the duplicate so that we don't > > * ovewrite them when writing to storage. The next time bluetoothd > > * starts the device will show up as a single instance. > > */ > > void device_merge_duplicate(struct btd_device *dev, struct btd_device *dup) > > > > But it doesn't seem to carry over the services, etc, as it seems we > > can't really just use one object at this point then both need to > > interact with each other, perhaps by storing the duplicate into > > btd_device so the right object can be used depending on the bearer, > > etc. > > > Yes, this is just a workaround for the bluetoothctl client. The device_merge_duplicate > can't handle it. Actually, there are two different dbus interfaces created for the > two device objects. I didn't find a good way to merge these two dbus interface (I'm > not familiar with dbus). > > Another thought is use the AddressType property to distinguish the two device objects. > however, in current implementation, BR/EDR and BLE public address are both assumed as > "public". Can we add a new string type (like "le_public") in `property_get_address_type` > for BLE public address. > > Do you have any idea to get the bearer info in client? Well one think that we should probably do is to list the GATT objects on the public address, probe the services, etc, so they would appear as duplicates but we can't really remove the object with the RPA address while still connected as that will probably confuse the clients, that said perhaps we should cleanup it when we disconnect since at that point it is not longer needed and shall be consider temporary. As to how the client should handle this, I think the best way to handle it to only show non-duplicate addresses, so in part I think we need to change this in bluetoothctl and then document this behavior by saying that there could be 2 different objects with the same Address where the objects with the RPA address is to be considered temporary. > > > >> Modify cmd_list_attributes to search all proxies in > >> default_ctrl->devices and display the related attributes. > >> --- > >> client/main.c | 55 +++++++++++++++++++++++++++++++++++++++++++++++---- > >> 1 file changed, 51 insertions(+), 4 deletions(-) > >> > >> diff --git a/client/main.c b/client/main.c > >> index 50aa3e7a6..17c1fb278 100644 > >> --- a/client/main.c > >> +++ b/client/main.c > >> @@ -768,6 +768,29 @@ static GDBusProxy *find_proxy_by_address(GList *source, const char *address) > >> return NULL; > >> } > >> > >> +static GList *find_all_proxy_by_address(GList *source, const char *address) > >> +{ > >> + GList *list; > >> + GList *all_proxy = NULL; > >> + > >> + for (list = g_list_first(source); list; list = g_list_next(list)) { > >> + GDBusProxy *proxy = list->data; > >> + DBusMessageIter iter; > >> + const char *str; > >> + > >> + if (g_dbus_proxy_get_property(proxy, "Address", &iter) == FALSE) > >> + continue; > >> + > >> + dbus_message_iter_get_basic(&iter, &str); > >> + > >> + if (!strcasecmp(str, address)) > >> + all_proxy = g_list_append(all_proxy, proxy); > >> + } > >> + > >> + return all_proxy; > >> +} > >> + > >> + > >> static gboolean check_default_ctrl(void) > >> { > >> if (!default_ctrl) { > >> @@ -2051,7 +2074,9 @@ static void cmd_disconn(int argc, char *argv[]) > >> > >> static void cmd_list_attributes(int argc, char *argv[]) > >> { > >> - GDBusProxy *proxy; > >> + GList *all_proxy = NULL; > >> + GList *list; > >> + GDBusProxy *proxy = NULL; > >> const char *path; > >> > >> if (argc > 1 && !strcmp(argv[1], "local")) { > >> @@ -2059,11 +2084,33 @@ static void cmd_list_attributes(int argc, char *argv[]) > >> goto done; > >> } > >> > >> - proxy = find_device(argc, argv); > >> - if (!proxy) > >> + if (argc < 2 || !strlen(argv[1])) { > >> + if (default_dev) { > >> + proxy = default_dev; > >> + path = g_dbus_proxy_get_path(proxy); > >> + goto done; > >> + } > >> + bt_shell_printf("Missing device address argument\n"); > >> return bt_shell_noninteractive_quit(EXIT_FAILURE); > >> + } else { > >> + if (check_default_ctrl() == FALSE) > >> + return bt_shell_noninteractive_quit(EXIT_FAILURE); > >> > >> - path = g_dbus_proxy_get_path(proxy); > >> + all_proxy = find_all_proxy_by_address(default_ctrl->devices, > >> + argv[1]); > >> + if (!all_proxy) { > >> + bt_shell_printf("Device %s not available\n", argv[1]); > >> + return bt_shell_noninteractive_quit(EXIT_FAILURE); > >> + } > >> + for (list = g_list_first(all_proxy); list; > >> + list = g_list_next(list)) { > >> + proxy = list->data; > >> + path = g_dbus_proxy_get_path(proxy); > >> + gatt_list_attributes(path); > >> + } > >> + g_list_free(all_proxy); > >> + return bt_shell_noninteractive_quit(EXIT_SUCCESS); > >> + } > >> > >> done: > >> gatt_list_attributes(path); > >> -- > >> 2.25.1 > >> > >> > > > > >
diff --git a/client/main.c b/client/main.c index 50aa3e7a6..17c1fb278 100644 --- a/client/main.c +++ b/client/main.c @@ -768,6 +768,29 @@ static GDBusProxy *find_proxy_by_address(GList *source, const char *address) return NULL; } +static GList *find_all_proxy_by_address(GList *source, const char *address) +{ + GList *list; + GList *all_proxy = NULL; + + for (list = g_list_first(source); list; list = g_list_next(list)) { + GDBusProxy *proxy = list->data; + DBusMessageIter iter; + const char *str; + + if (g_dbus_proxy_get_property(proxy, "Address", &iter) == FALSE) + continue; + + dbus_message_iter_get_basic(&iter, &str); + + if (!strcasecmp(str, address)) + all_proxy = g_list_append(all_proxy, proxy); + } + + return all_proxy; +} + + static gboolean check_default_ctrl(void) { if (!default_ctrl) { @@ -2051,7 +2074,9 @@ static void cmd_disconn(int argc, char *argv[]) static void cmd_list_attributes(int argc, char *argv[]) { - GDBusProxy *proxy; + GList *all_proxy = NULL; + GList *list; + GDBusProxy *proxy = NULL; const char *path; if (argc > 1 && !strcmp(argv[1], "local")) { @@ -2059,11 +2084,33 @@ static void cmd_list_attributes(int argc, char *argv[]) goto done; } - proxy = find_device(argc, argv); - if (!proxy) + if (argc < 2 || !strlen(argv[1])) { + if (default_dev) { + proxy = default_dev; + path = g_dbus_proxy_get_path(proxy); + goto done; + } + bt_shell_printf("Missing device address argument\n"); return bt_shell_noninteractive_quit(EXIT_FAILURE); + } else { + if (check_default_ctrl() == FALSE) + return bt_shell_noninteractive_quit(EXIT_FAILURE); - path = g_dbus_proxy_get_path(proxy); + all_proxy = find_all_proxy_by_address(default_ctrl->devices, + argv[1]); + if (!all_proxy) { + bt_shell_printf("Device %s not available\n", argv[1]); + return bt_shell_noninteractive_quit(EXIT_FAILURE); + } + for (list = g_list_first(all_proxy); list; + list = g_list_next(list)) { + proxy = list->data; + path = g_dbus_proxy_get_path(proxy); + gatt_list_attributes(path); + } + g_list_free(all_proxy); + return bt_shell_noninteractive_quit(EXIT_SUCCESS); + } done: gatt_list_attributes(path);