[PATCH BlueZ v2 0/3] pbap: use the public DBus connection

linux-bluetooth.vger.kernel.org archive mirror
 help / color / mirror / Atom feed

* [PATCH BlueZ v2 0/3] pbap: use the public DBus connection
@ 2025-06-11 13:06 Andrew Sayers
  2025-06-11 13:06 ` [PATCH BlueZ v2 1/3] obexd: Pass at_(un)register value to logind callbacks Andrew Sayers
                   ` (3 more replies)
  0 siblings, 4 replies; 6+ messages in thread
From: Andrew Sayers @ 2025-06-11 13:06 UTC (permalink / raw)
  To: linux-bluetooth; +Cc: luiz.dentz, pav, frederic.danis, Andrew Sayers

DBus requests are received by the public DBus connection, because it
registers the relevant bus name.  My previous patches told PBAP to
listen on a private connection, but DBus doesn't let connections share
bus names, so it needs to listen on the public connection instead.

This adds a little more complexity to the logind callbacks, to avoid
unregistering profiles during shutdown.  It also removes an API I
previously added, which is no longer used and would only encourage the
next person to make the same mistake.

While reviewing this change, it became obvious seat-detection should
be merged into the normal driver interface instead.  That change is
likely to add time and could create its own bugs, so this series fixes
the simple bug while we discuss the deeper issue.

Reported-by: Frédéric Danis <frederic.danis@collabora.com>
Closes: https://lore.kernel.org/linux-bluetooth/333ad76e-0aba-4f93-b141-8e69fb47535f@collabora.com/
Suggested-by: Pauli Virtanen <pav@iki.fi>
Signed-off-by: Andrew Sayers <kernel.org@pileofstuff.org>
--

Frédéric and Pauli - please let me know if you'd like to be CCed on
the architecture discussion.  Otherwise, I'll assume you're not
interested in that part of the problem.

V1 -> V2 Move instances of "(void)foo" below "int bar"
      (fixes a "mixed declarations and code" error)

Andrew Sayers (3):
      obexd: Pass at_(un)register value to logind callbacks
      pbap: use the public DBus connection
      Revert "obexd: Support creating private system/session bus connections"

 obexd/client/pbap.c       | 28 ++++++++++++++++++++++++----
 obexd/plugins/bluetooth.c |  6 ++++--
 obexd/src/logind.c        | 14 +++++++-------
 obexd/src/logind.h        |  8 ++++----
 obexd/src/main.c          |  8 --------
 obexd/src/obexd.h         |  2 --
 6 files changed, 39 insertions(+), 27 deletions(-)

^ permalink raw reply	[flat|nested] 6+ messages in thread

* [PATCH BlueZ v2 1/3] obexd: Pass at_(un)register value to logind callbacks
  2025-06-11 13:06 [PATCH BlueZ v2 0/3] pbap: use the public DBus connection Andrew Sayers
@ 2025-06-11 13:06 ` Andrew Sayers
  2025-06-11 14:35   ` pbap: use the public DBus connection bluez.test.bot
  2025-06-11 13:06 ` [PATCH BlueZ v2 2/3] " Andrew Sayers
                   ` (2 subsequent siblings)
  3 siblings, 1 reply; 6+ messages in thread
From: Andrew Sayers @ 2025-06-11 13:06 UTC (permalink / raw)
  To: linux-bluetooth; +Cc: luiz.dentz, pav, frederic.danis, Andrew Sayers

Logind (un)registers callbacks that it calls when the user's state changes.
Callbacks may also be called during (un)registration.
Clients may need to handle those initial/final calls specially.

Pass an argument indicating whether this is being called during
(un)registration, and modify existing callbacks to ignore that argument.

Signed-off-by: Andrew Sayers <kernel.org@pileofstuff.org>
---
 obexd/client/pbap.c       |  6 ++++--
 obexd/plugins/bluetooth.c |  6 ++++--
 obexd/src/logind.c        | 14 +++++++-------
 obexd/src/logind.h        |  8 ++++----
 4 files changed, 19 insertions(+), 15 deletions(-)

diff --git a/obexd/client/pbap.c b/obexd/client/pbap.c
index 51b523592..7d651722a 100644
--- a/obexd/client/pbap.c
+++ b/obexd/client/pbap.c
@@ -1455,9 +1455,10 @@ static struct obc_driver pbap = {
 	.remove = pbap_remove
 };
 
-static int pbap_init_cb(void)
+static int pbap_init_cb(gboolean at_register)
 {
 	int err;
+	(void)at_register;
 
 	DBG("");
 
@@ -1482,8 +1483,9 @@ static int pbap_init_cb(void)
 	return 0;
 }
 
-static void pbap_exit_cb(void)
+static void pbap_exit_cb(gboolean at_unregister)
 {
+	(void)at_unregister;
 	DBG("");
 
 	g_dbus_remove_watch(system_conn, listener_id);
diff --git a/obexd/plugins/bluetooth.c b/obexd/plugins/bluetooth.c
index 7ff27a8a8..355921479 100644
--- a/obexd/plugins/bluetooth.c
+++ b/obexd/plugins/bluetooth.c
@@ -427,8 +427,9 @@ static const struct obex_transport_driver driver = {
 
 static unsigned int listener_id = 0;
 
-static int bluetooth_init_cb(void)
+static int bluetooth_init_cb(gboolean at_register)
 {
+	(void)at_register;
 	connection = g_dbus_setup_private(DBUS_BUS_SYSTEM, NULL, NULL);
 	if (connection == NULL)
 		return -EPERM;
@@ -439,9 +440,10 @@ static int bluetooth_init_cb(void)
 	return obex_transport_driver_register(&driver);
 }
 
-static void bluetooth_exit_cb(void)
+static void bluetooth_exit_cb(gboolean at_unregister)
 {
 	GSList *l;
+	(void)at_unregister;
 
 	g_dbus_remove_watch(connection, listener_id);
 
diff --git a/obexd/src/logind.c b/obexd/src/logind.c
index a0eb62b1e..b4279b209 100644
--- a/obexd/src/logind.c
+++ b/obexd/src/logind.c
@@ -43,13 +43,13 @@ static void call_init_cb(gpointer data, gpointer user_data)
 {
 	int res;
 
-	res = ((struct callback_pair *)data)->init_cb();
+	res = ((struct callback_pair *)data)->init_cb(FALSE);
 	if (res)
 		*(int *)user_data = res;
 }
 static void call_exit_cb(gpointer data, gpointer user_data)
 {
-	((struct callback_pair *)data)->exit_cb();
+	((struct callback_pair *)data)->exit_cb(FALSE);
 }
 
 static int update(void)
@@ -229,7 +229,7 @@ int logind_register(logind_init_cb init_cb, logind_exit_cb exit_cb)
 	struct callback_pair *cbs;
 
 	if (!monitoring_enabled)
-		return init_cb();
+		return init_cb(TRUE);
 	if (callbacks == NULL) {
 		int res;
 
@@ -237,23 +237,23 @@ int logind_register(logind_init_cb init_cb, logind_exit_cb exit_cb)
 		if (res) {
 			error("logind_init(): %s - login detection disabled",
 				strerror(-res));
-			return init_cb();
+			return init_cb(TRUE);
 		}
 	}
 	cbs = g_new(struct callback_pair, 1);
 	cbs->init_cb = init_cb;
 	cbs->exit_cb = exit_cb;
 	callbacks = g_slist_prepend(callbacks, cbs);
-	return active ? init_cb() : 0;
+	return active ? init_cb(TRUE) : 0;
 }
 void logind_unregister(logind_init_cb init_cb, logind_exit_cb exit_cb)
 {
 	GSList *cb_node;
 
 	if (!monitoring_enabled)
-		return exit_cb();
+		return exit_cb(TRUE);
 	if (active)
-		exit_cb();
+		exit_cb(TRUE);
 	cb_node = g_slist_find_custom(callbacks, init_cb, find_cb);
 	if (cb_node != NULL)
 		callbacks = g_slist_delete_link(callbacks, cb_node);
diff --git a/obexd/src/logind.h b/obexd/src/logind.h
index eb3ff8d7b..3cdb03338 100644
--- a/obexd/src/logind.h
+++ b/obexd/src/logind.h
@@ -8,8 +8,8 @@
  *
  */
 
-typedef int (*logind_init_cb)(void);
-typedef void (*logind_exit_cb)(void);
+typedef int (*logind_init_cb)(gboolean at_register);
+typedef void (*logind_exit_cb)(gboolean at_unregister);
 
 #ifdef SYSTEMD
 
@@ -22,12 +22,12 @@ int logind_set(gboolean enabled);
 static inline int logind_register(logind_init_cb init_cb,
 					logind_exit_cb exit_cb)
 {
-	return init_cb();
+	return init_cb(TRUE);
 }
 static inline void logind_unregister(logind_init_cb init_cb,
 					logind_exit_cb exit_cb)
 {
-	return exit_cb();
+	return exit_cb(TRUE);
 }
 static inline int logind_set(gboolean enabled)
 {
-- 
2.49.0


^ permalink raw reply related	[flat|nested] 6+ messages in thread

* RE: pbap: use the public DBus connection
  2025-06-11 13:06 ` [PATCH BlueZ v2 1/3] obexd: Pass at_(un)register value to logind callbacks Andrew Sayers
@ 2025-06-11 14:35   ` bluez.test.bot
  0 siblings, 0 replies; 6+ messages in thread
From: bluez.test.bot @ 2025-06-11 14:35 UTC (permalink / raw)
  To: linux-bluetooth, kernel.org

[-- Attachment #1: Type: text/plain, Size: 1261 bytes --]

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

---Test result---

Test Summary:
CheckPatch                    PENDING   0.34 seconds
GitLint                       PENDING   0.35 seconds
BuildEll                      PASS      19.98 seconds
BluezMake                     PASS      2634.06 seconds
MakeCheck                     PASS      20.60 seconds
MakeDistcheck                 PASS      198.09 seconds
CheckValgrind                 PASS      275.31 seconds
CheckSmatch                   PASS      306.04 seconds
bluezmakeextell               PASS      129.10 seconds
IncrementalBuild              PENDING   0.37 seconds
ScanBuild                     PASS      905.63 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


^ permalink raw reply	[flat|nested] 6+ messages in thread

* [PATCH BlueZ v2 2/3] pbap: use the public DBus connection
  2025-06-11 13:06 [PATCH BlueZ v2 0/3] pbap: use the public DBus connection Andrew Sayers
  2025-06-11 13:06 ` [PATCH BlueZ v2 1/3] obexd: Pass at_(un)register value to logind callbacks Andrew Sayers
@ 2025-06-11 13:06 ` Andrew Sayers
  2025-06-11 13:06 ` [PATCH BlueZ v2 3/3] Revert "obexd: Support creating private system/session bus connections" Andrew Sayers
  2025-06-11 14:50 ` [PATCH BlueZ v2 0/3] pbap: use the public DBus connection patchwork-bot+bluetooth
  3 siblings, 0 replies; 6+ messages in thread
From: Andrew Sayers @ 2025-06-11 13:06 UTC (permalink / raw)
  To: linux-bluetooth; +Cc: luiz.dentz, pav, frederic.danis, Andrew Sayers

PBAP must not use its own private DBus connection, as it needs to
receive messages sent to the bus name of the public connection.

PBAP must not unregister itself when the program is exiting, as it might
cause a long delay.  Unregistering at exit is redundant anyway, as the
service will be implicitly unregistered when the connection is closed.

But PBAP must unregister itself when the user becomes inactive mid-session,
so systems with multiple users logged in at once can share bluetooth.

Use the public DBus connection instead of a private one,
and explicitly unregister the profile if appropriate.

Thanks Pauli Virtanen for pointing out the exit issue:
https://lore.kernel.org/linux-bluetooth/ae15a9fa4bf0bd509dd3d44f1f364e241e50956c.camel@iki.fi/

Reported-by: Frédéric Danis <frederic.danis@collabora.com>
Closes: https://lore.kernel.org/linux-bluetooth/333ad76e-0aba-4f93-b141-8e69fb47535f@collabora.com/
Suggested-by: Pauli Virtanen <pav@iki.fi>
Signed-off-by: Andrew Sayers <kernel.org@pileofstuff.org>
---
 obexd/client/pbap.c | 24 +++++++++++++++++++++---
 1 file changed, 21 insertions(+), 3 deletions(-)

diff --git a/obexd/client/pbap.c b/obexd/client/pbap.c
index 7d651722a..78c46bf86 100644
--- a/obexd/client/pbap.c
+++ b/obexd/client/pbap.c
@@ -1462,7 +1462,7 @@ static int pbap_init_cb(gboolean at_register)
 
 	DBG("");
 
-	conn = obex_setup_dbus_connection_private(NULL, NULL);
+	conn = obex_get_dbus_connection();
 	if (!conn)
 		return -EIO;
 
@@ -1485,9 +1485,28 @@ static int pbap_init_cb(gboolean at_register)
 
 static void pbap_exit_cb(gboolean at_unregister)
 {
-	(void)at_unregister;
+	DBusMessage *msg;
+	DBusMessageIter iter;
+	char *uuid = PBAP_CLIENT_UUID;
+
 	DBG("");
 
+	if (!at_unregister) {
+		client_path = g_strconcat("/org/bluez/obex/", uuid, NULL);
+		g_strdelimit(client_path, "-", '_');
+
+		msg = dbus_message_new_method_call("org.bluez", "/org/bluez",
+							"org.bluez.ProfileManager1",
+							"UnregisterProfile");
+
+		dbus_message_iter_init_append(msg, &iter);
+
+		dbus_message_iter_append_basic(&iter, DBUS_TYPE_OBJECT_PATH,
+								&client_path);
+
+		g_dbus_send_message(system_conn, msg);
+	}
+
 	g_dbus_remove_watch(system_conn, listener_id);
 
 	unregister_profile();
@@ -1499,7 +1518,6 @@ static void pbap_exit_cb(gboolean at_unregister)
 	}
 
 	if (conn) {
-		dbus_connection_close(conn);
 		dbus_connection_unref(conn);
 		conn = NULL;
 	}
-- 
2.49.0


^ permalink raw reply related	[flat|nested] 6+ messages in thread

* [PATCH BlueZ v2 3/3] Revert "obexd: Support creating private system/session bus connections"
  2025-06-11 13:06 [PATCH BlueZ v2 0/3] pbap: use the public DBus connection Andrew Sayers
  2025-06-11 13:06 ` [PATCH BlueZ v2 1/3] obexd: Pass at_(un)register value to logind callbacks Andrew Sayers
  2025-06-11 13:06 ` [PATCH BlueZ v2 2/3] " Andrew Sayers
@ 2025-06-11 13:06 ` Andrew Sayers
  2025-06-11 14:50 ` [PATCH BlueZ v2 0/3] pbap: use the public DBus connection patchwork-bot+bluetooth
  3 siblings, 0 replies; 6+ messages in thread
From: Andrew Sayers @ 2025-06-11 13:06 UTC (permalink / raw)
  To: linux-bluetooth; +Cc: luiz.dentz, pav, frederic.danis, Andrew Sayers

This reverts commit 237d818ef294e22be87fba69b3cdd79c75c201e7,
which encouraged the use of private DBus connections.
Doing so caused a bug, so it's better to remove the temptation for now
and re-add it if a legitimate use case is ever found.

Signed-off-by: Andrew Sayers <kernel.org@pileofstuff.org>
---
 obexd/src/main.c  | 8 --------
 obexd/src/obexd.h | 2 --
 2 files changed, 10 deletions(-)

diff --git a/obexd/src/main.c b/obexd/src/main.c
index df150973e..6837f0d73 100644
--- a/obexd/src/main.c
+++ b/obexd/src/main.c
@@ -254,14 +254,6 @@ DBusConnection *obex_setup_dbus_connection(const char *name,
 	return connection;
 }
 
-DBusConnection *obex_setup_dbus_connection_private(const char *name,
-					DBusError *error)
-{
-	return g_dbus_setup_private(option_system_bus ?
-				DBUS_BUS_SYSTEM : DBUS_BUS_SESSION,
-				name, error);
-}
-
 int main(int argc, char *argv[])
 {
 	GOptionContext *context;
diff --git a/obexd/src/obexd.h b/obexd/src/obexd.h
index 560db29ce..5e5edc4de 100644
--- a/obexd/src/obexd.h
+++ b/obexd/src/obexd.h
@@ -33,5 +33,3 @@ const char *obex_option_capability(void);
 DBusConnection *obex_get_dbus_connection(void);
 DBusConnection *obex_setup_dbus_connection(const char *name,
 					DBusError *error);
-DBusConnection *obex_setup_dbus_connection_private(const char *name,
-					DBusError *error);
-- 
2.49.0


^ permalink raw reply related	[flat|nested] 6+ messages in thread

* Re: [PATCH BlueZ v2 0/3] pbap: use the public DBus connection
  2025-06-11 13:06 [PATCH BlueZ v2 0/3] pbap: use the public DBus connection Andrew Sayers
                   ` (2 preceding siblings ...)
  2025-06-11 13:06 ` [PATCH BlueZ v2 3/3] Revert "obexd: Support creating private system/session bus connections" Andrew Sayers
@ 2025-06-11 14:50 ` patchwork-bot+bluetooth
  3 siblings, 0 replies; 6+ messages in thread
From: patchwork-bot+bluetooth @ 2025-06-11 14:50 UTC (permalink / raw)
  To: Andrew Sayers; +Cc: linux-bluetooth, luiz.dentz, pav, frederic.danis

Hello:

This series was applied to bluetooth/bluez.git (master)
by Luiz Augusto von Dentz <luiz.von.dentz@intel.com>:

On Wed, 11 Jun 2025 14:06:12 +0100 you wrote:
> DBus requests are received by the public DBus connection, because it
> registers the relevant bus name.  My previous patches told PBAP to
> listen on a private connection, but DBus doesn't let connections share
> bus names, so it needs to listen on the public connection instead.
> 
> This adds a little more complexity to the logind callbacks, to avoid
> unregistering profiles during shutdown.  It also removes an API I
> previously added, which is no longer used and would only encourage the
> next person to make the same mistake.
> 
> [...]

Here is the summary with links:
  - [BlueZ,v2,1/3] obexd: Pass at_(un)register value to logind callbacks
    https://git.kernel.org/pub/scm/bluetooth/bluez.git/?id=883c1c11c644
  - [BlueZ,v2,2/3] pbap: use the public DBus connection
    https://git.kernel.org/pub/scm/bluetooth/bluez.git/?id=df0036d9e41f
  - [BlueZ,v2,3/3] Revert "obexd: Support creating private system/session bus connections"
    https://git.kernel.org/pub/scm/bluetooth/bluez.git/?id=aea0f46f173f

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html



^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2025-06-11 14:50 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-06-11 13:06 [PATCH BlueZ v2 0/3] pbap: use the public DBus connection Andrew Sayers
2025-06-11 13:06 ` [PATCH BlueZ v2 1/3] obexd: Pass at_(un)register value to logind callbacks Andrew Sayers
2025-06-11 14:35   ` pbap: use the public DBus connection bluez.test.bot
2025-06-11 13:06 ` [PATCH BlueZ v2 2/3] " Andrew Sayers
2025-06-11 13:06 ` [PATCH BlueZ v2 3/3] Revert "obexd: Support creating private system/session bus connections" Andrew Sayers
2025-06-11 14:50 ` [PATCH BlueZ v2 0/3] pbap: use the public DBus connection patchwork-bot+bluetooth

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).