[BlueZ,v2,1/2] client/player: Rework transport.select

Commit Message

Iulia Tanasescu Jan. 27, 2025, 9:30 a.m. UTC

Since the transport.select command should also work for transports
created by audio servers, the transport should not be required to
be associated with a local bluetoothctl endpoint, to avoid errors
like below:

[bluetoothctl]> scan on
[bluetoothctl]> [NEW] Device 1C:F1:FA:E7:B0:3F 1C-F1-FA-E7-B0-3F
[1C-F1-FA-E7-B0-3F]> [NEW] Transport
                     /org/bluez/hci0/dev_1C_F1_FA_E7_B0_3F/bis1/fd0
[1C-F1-FA-E7-B0-3F]> [NEW] Transport
                     /org/bluez/hci0/dev_1C_F1_FA_E7_B0_3F/bis2/fd1
[1C-F1-FA-E7-B0-3F]> transport.select
                     /org/bluez/hci0/dev_1C_F1_FA_E7_B0_3F/bis1/fd0
                     /org/bluez/hci0/dev_1C_F1_FA_E7_B0_3F/bis2/fd
Local endpoint not found

This reworks transport.select to use a dedicated structure to hold
information about the transport and its links, instead of using the
local endpoint.
---
 client/player.c | 156 ++++++++++++++++++++++--------------------------
 1 file changed, 72 insertions(+), 84 deletions(-)

Comments

[e-mail] Jan. 27, 2025, 10:32 a.m. UTC | #1

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=928488

---Test result---

Test Summary:
CheckPatch                    PENDING   0.19 seconds
GitLint                       PENDING   0.21 seconds
BuildEll                      PASS      21.13 seconds
BluezMake                     PASS      1537.96 seconds
MakeCheck                     PASS      13.01 seconds
MakeDistcheck                 PASS      157.88 seconds
CheckValgrind                 PASS      216.10 seconds
CheckSmatch                   PASS      271.08 seconds
bluezmakeextell               PASS      98.03 seconds
IncrementalBuild              PENDING   0.33 seconds
ScanBuild                     PASS      865.86 seconds

Details
##############################
Test: CheckPatch - PENDING
Desc: Run checkpatch.pl script
Output:

##############################
Test: GitLint - PENDING
Desc: Run gitlint
Output:

##############################
Test: IncrementalBuild - PENDING
Desc: Incremental build with the patches in the series
Output:



---
Regards,
Linux Bluetooth

Patch

diff --git a/client/player.c b/client/player.c
index 464a9cc14..0aab0fc2c 100644
--- a/client/player.c
+++ b/client/player.c
@@ -4,7 +4,7 @@ 
  *  BlueZ - Bluetooth protocol stack for Linux
  *
  *  Copyright (C) 2020  Intel Corporation. All rights reserved.
- *  Copyright 2023-2024 NXP
+ *  Copyright 2023-2025 NXP
  *
  *
  */
@@ -115,8 +115,6 @@  struct endpoint {
 	uint8_t iso_group;
 	uint8_t iso_stream;
 	struct queue *acquiring;
-	struct queue *links;
-	struct queue *selecting;
 	struct queue *transports;
 	DBusMessage *msg;
 	struct preset *preset;
@@ -150,8 +148,14 @@  struct transport {
 	int num;
 };
 
-static void transport_set_links(struct endpoint *ep, GDBusProxy *proxy);
-static void transport_select(GDBusProxy *proxy);
+struct transport_select_args {
+	GDBusProxy *proxy;
+	struct queue *links;
+	struct queue *selecting;
+};
+
+static void transport_set_links(struct transport_select_args *args);
+static void transport_select(struct transport_select_args *args);
 
 static void endpoint_unregister(void *data)
 {
@@ -2923,8 +2927,6 @@  static void endpoint_free(void *data)
 		free(ep->preset);
 
 	queue_destroy(ep->acquiring, NULL);
-	queue_destroy(ep->links, NULL);
-	queue_destroy(ep->selecting, NULL);
 	queue_destroy(ep->transports, free);
 
 	g_free(ep->path);
@@ -4891,26 +4893,37 @@  static void acquire_reply(DBusMessage *message, void *user_data)
 	return bt_shell_noninteractive_quit(EXIT_FAILURE);
 }
 
+static void free_transport_select_args(struct transport_select_args *args)
+{
+	queue_destroy(args->links, NULL);
+	queue_destroy(args->selecting, NULL);
+	g_free(args);
+}
+
 static void select_reply(DBusMessage *message, void *user_data)
 {
 	DBusError error;
-	struct endpoint *ep = user_data;
+	struct transport_select_args *args = user_data;
+	GDBusProxy *link;
 
 	dbus_error_init(&error);
 
 	if (dbus_set_error_from_message(&error, message) == TRUE) {
 		bt_shell_printf("Failed to select: %s\n", error.name);
 		dbus_error_free(&error);
+		free_transport_select_args(args);
 		return bt_shell_noninteractive_quit(EXIT_FAILURE);
 	}
 
 	bt_shell_printf("Select successful\n");
 
-	if (queue_isempty(ep->selecting)) {
-		/* All links have been selected */
-		queue_destroy(ep->selecting, NULL);
-		ep->selecting = NULL;
-
+	/* Select next link */
+	link = queue_pop_head(args->selecting);
+	if (link) {
+		args->proxy = link;
+		transport_select(args);
+	} else {
+		free_transport_select_args(args);
 		return bt_shell_noninteractive_quit(EXIT_SUCCESS);
 	}
 }
@@ -5174,22 +5187,23 @@  static void cmd_acquire_transport(int argc, char *argv[])
 
 static void set_bcode_cb(const DBusError *error, void *user_data)
 {
-	GDBusProxy *proxy = user_data;
+	struct transport_select_args *args = user_data;
 
 	if (dbus_error_is_set(error)) {
 		bt_shell_printf("Failed to set broadcast code: %s\n",
 								error->name);
+		free_transport_select_args(args);
 		return bt_shell_noninteractive_quit(EXIT_FAILURE);
 	}
 
 	bt_shell_printf("Setting broadcast code succeeded\n");
 
-	transport_select(proxy);
+	transport_select(args);
 }
 
 static void set_bcode(const char *input, void *user_data)
 {
-	GDBusProxy *proxy = user_data;
+	struct transport_select_args *args = user_data;
 	char *bcode;
 
 	if (!strcasecmp(input, "n") || !strcasecmp(input, "no"))
@@ -5197,47 +5211,39 @@  static void set_bcode(const char *input, void *user_data)
 	else
 		bcode = g_strdup(input);
 
-	if (g_dbus_proxy_set_property_dict(proxy, "QoS",
+	if (g_dbus_proxy_set_property_dict(args->proxy, "QoS",
 				set_bcode_cb, user_data,
 				NULL, "BCode", DBUS_TYPE_ARRAY, DBUS_TYPE_BYTE,
 				strlen(bcode), bcode, NULL) == FALSE) {
 		bt_shell_printf("Setting broadcast code failed\n");
 		g_free(bcode);
+		free_transport_select_args(args);
 		return bt_shell_noninteractive_quit(EXIT_FAILURE);
 	}
 
 	g_free(bcode);
 }
 
-static void transport_select(GDBusProxy *proxy)
+static void transport_select(struct transport_select_args *args)
 {
-	struct endpoint *ep;
-	GDBusProxy *link;
-
-	ep = find_ep_by_transport(g_dbus_proxy_get_path(proxy));
-	if (!ep)
-		return bt_shell_noninteractive_quit(EXIT_FAILURE);
-
-	if (!g_dbus_proxy_method_call(proxy, "Select", NULL,
-					select_reply, ep, NULL)) {
+	if (!g_dbus_proxy_method_call(args->proxy, "Select", NULL,
+					select_reply, args, NULL)) {
 		bt_shell_printf("Failed select transport\n");
+		free_transport_select_args(args);
 		return bt_shell_noninteractive_quit(EXIT_FAILURE);
 	}
-
-	/* Select next link */
-	link = queue_pop_head(ep->selecting);
-	if (link)
-		transport_select(link);
 }
 
-static void transport_set_bcode(GDBusProxy *proxy)
+static void transport_set_bcode(struct transport_select_args *args)
 {
 	DBusMessageIter iter, array, entry, value;
 	unsigned char encryption;
 	const char *key;
 
-	if (g_dbus_proxy_get_property(proxy, "QoS", &iter) == FALSE)
+	if (g_dbus_proxy_get_property(args->proxy, "QoS", &iter) == FALSE) {
+		free_transport_select_args(args);
 		return bt_shell_noninteractive_quit(EXIT_FAILURE);
+	}
 
 	dbus_message_iter_recurse(&iter, &array);
 
@@ -5253,7 +5259,7 @@  static void transport_set_bcode(GDBusProxy *proxy)
 			if (encryption == 1) {
 				bt_shell_prompt_input("",
 					"Enter brocast code[value/no]:",
-					set_bcode, proxy);
+					set_bcode, args);
 				return;
 			}
 			break;
@@ -5264,7 +5270,7 @@  static void transport_set_bcode(GDBusProxy *proxy)
 	/* Go straight to selecting transport, if Broadcast Code
 	 * is not required.
 	 */
-	transport_select(proxy);
+	transport_select(args);
 }
 
 static void transport_unselect(GDBusProxy *proxy, bool prompt)
@@ -5278,58 +5284,52 @@  static void transport_unselect(GDBusProxy *proxy, bool prompt)
 
 static void set_links_cb(const DBusError *error, void *user_data)
 {
-	GDBusProxy *proxy = user_data;
-	const char *path = g_dbus_proxy_get_path(proxy);
-	struct endpoint *ep;
+	struct transport_select_args *args = user_data;
 	GDBusProxy *link;
 
-	ep = find_ep_by_transport(path);
-	if (!ep) {
-		bt_shell_printf("Local endpoint not found\n");
-		return bt_shell_noninteractive_quit(EXIT_FAILURE);
-	}
-
-	link = queue_pop_head(ep->links);
+	link = queue_pop_head(args->links);
 
-	if (queue_isempty(ep->links)) {
-		queue_destroy(ep->links, NULL);
-		ep->links = NULL;
+	if (queue_isempty(args->links)) {
+		queue_destroy(args->links, NULL);
+		args->links = NULL;
 	}
 
 	if (dbus_error_is_set(error)) {
 		bt_shell_printf("Failed to set link %s: %s\n",
 						g_dbus_proxy_get_path(link),
 						error->name);
+		free_transport_select_args(args);
 		return bt_shell_noninteractive_quit(EXIT_FAILURE);
 	}
 
 	bt_shell_printf("Successfully linked transport %s\n",
 						g_dbus_proxy_get_path(link));
 
-	if (!ep->selecting)
-		ep->selecting = queue_new();
+	if (!args->selecting)
+		args->selecting = queue_new();
 
 	/* Enqueue link to mark that it is ready to be selected */
-	queue_push_tail(ep->selecting, link);
+	queue_push_tail(args->selecting, link);
 
 	/* Continue setting the remanining links */
-	transport_set_links(ep, proxy);
+	transport_set_links(args);
 }
 
-static void transport_set_links(struct endpoint *ep, GDBusProxy *proxy)
+static void transport_set_links(struct transport_select_args *args)
 {
 	GDBusProxy *link;
 	const char *path;
 
-	link = queue_peek_head(ep->links);
+	link = queue_peek_head(args->links);
 	if (link) {
 		path = g_dbus_proxy_get_path(link);
 
-		if (g_dbus_proxy_set_property_array(proxy, "Links",
+		if (g_dbus_proxy_set_property_array(args->proxy, "Links",
 					DBUS_TYPE_OBJECT_PATH,
 					&path, 1, set_links_cb,
-					proxy, NULL) == FALSE) {
+					args, NULL) == FALSE) {
 			bt_shell_printf("Linking transport %s failed\n", path);
+			free_transport_select_args(args);
 			return bt_shell_noninteractive_quit(EXIT_FAILURE);
 		}
 
@@ -5339,28 +5339,17 @@  static void transport_set_links(struct endpoint *ep, GDBusProxy *proxy)
 	/* If all links have been set, check is transport requires the
 	 * user to provide a Broadcast Code.
 	 */
-	transport_set_bcode(proxy);
-}
-
-static void endpoint_set_links(struct endpoint *ep)
-{
-	GDBusProxy *proxy = queue_pop_head(ep->links);
-
-	if (!proxy) {
-		bt_shell_printf("No transport to set links for\n");
-		return bt_shell_noninteractive_quit(EXIT_FAILURE);
-	}
-
-	transport_set_links(ep, proxy);
+	transport_set_bcode(args);
 }
 
 static void cmd_select_transport(int argc, char *argv[])
 {
 	GDBusProxy *link = NULL;
-	struct queue *links = queue_new();
-	struct endpoint *ep;
+	struct transport_select_args *args;
 	int i;
 
+	args = g_new0(struct transport_select_args, 1);
+
 	for (i = 1; i < argc; i++) {
 		link = g_dbus_proxy_lookup(transports, NULL, argv[i],
 					BLUEZ_MEDIA_TRANSPORT_INTERFACE);
@@ -5375,26 +5364,25 @@  static void cmd_select_transport(int argc, char *argv[])
 			goto fail;
 		}
 
-		/* Enqueue all links */
-		queue_push_tail(links, link);
-	}
+		if (!args->proxy) {
+			args->proxy = link;
+			continue;
+		}
 
-	/* Get reference to local endpoint */
-	ep = find_ep_by_transport(g_dbus_proxy_get_path(link));
-	if (!ep) {
-		bt_shell_printf("Local endpoint not found\n");
-		goto fail;
-	}
+		if (!args->links)
+			args->links = queue_new();
 
-	ep->links = links;
+		/* Enqueue all links */
+		queue_push_tail(args->links, link);
+	}
 
 	/* Link streams before selecting one by one */
-	endpoint_set_links(ep);
+	transport_set_links(args);
 
 	return;
 
 fail:
-	queue_destroy(links, NULL);
+	free_transport_select_args(args);
 	return bt_shell_noninteractive_quit(EXIT_FAILURE);
 }