[PATCH BlueZ v5 0/6] obexd: unregister profiles when the user is inactive

[PATCH BlueZ v5 0/6] obexd: unregister profiles when the user is inactive

[Andrew Sayers]

This is a follow-up to "obexd: only run one instance at once".
Instead of refusing to run parallel services, it unregisters
profiles when the user is inactive.  This avoids the need
for tmpfiles, and avoids issues where the user with the
obex service logs out, leaving obex disabled altogether.

Luiz previously suggested moving this to systemd, but I haven't had much
luck getting the systemd devs to accept changes, and Pauli's mention of
elogind (i.e. logind without systemd) suggests it's probably better
to avoid the dependency anyway.

I considered writing a separate D-Bus service that would notify you
when the session became (in)active, but D-Bus doesn't have particularly
strong guarantees about how long messages take to deliver, which could
lead to race conditions between instances on overloaded systems.

I also considered writing some kind of library, but there's not much
code to deduplicate, and most of it would need to be reworked for every
service that uses it.  So I wrote a gist for people to copy/paste:

https://gist.github.com/andrew-sayers/1c4a24f86a9a4c1b1e38d109f1bd1d1e

Signed-off-by: Andrew Sayers <kernel.org@pileofstuff.org>
---

Luiz, I've looked through the CI errors and added `#include <stdlib.h>` -
the other errors don't seem related to anything I've done,
but happy to fix anything I've missed.

Pauli mentioned that, since the API assumes we're calling poll() directly,
the timeout could theoretically be updated by events that don't trigger
an event_handler() call.  I agree it would be bikeshedding to fix that
now, but it would be a nightmare to debug if it ever happened,
so I've mentioned it in the relevant commit message.

v1 -> v2 open/close private DBus connections instead of
           calling UnregisterProfile (thanks Pauli Virtanen)
         Add obex_setup_dbus_connection_private() (to enable the above)
         Fix CI errors
         Minor issues I missed last time...
           s/regster/register/ in e-mail subject lines
           s/login_.*_cb/logind_$1_cb/g in logind.[ch]
           remove watches on exit in pbap.c and bluetooth.c
V2 -> V3 Fix checkpatch.pl errors - thanks Luiz
           Note: false positive warning about commit #4 is still present -
           this URL can't be split or shortened without causing issues.
V3 -> V4 Fix various bugs - thanks Pauli
         Split timeout_handler() into a separate patch - see above
V4 -> V5 Add #include <stdlib.h> based on CI tests - thanks Luiz
         Update the timeout on event_handler calls (and handle a couple of
           edge cases that could have misbehaved before) - thanks Pauli
         Discuss how the API assumes poll() in the penultimate
           commit message - thanks Pauli

Andrew Sayers (6):
      pbap: Support calling pbap_init() after pbap_exit()
      obexd/bluetooth: Support calling bluetooth_init() after bluetooth_exit()
      obexd: Support creating private system/session bus connections
      obexd: Unregister profiles when the user is inactive
      obexd: Support sd_login_monitor_get_timeout()
      Revert "obexd: only run one instance at once"

 Makefile.obexd            |  10 ++
 obexd/client/pbap.c       |  33 +++++-
 obexd/plugins/bluetooth.c |  30 ++++-
 obexd/src/logind.c        | 282 ++++++++++++++++++++++++++++++++++++++++++++++
 obexd/src/logind.h        |  26 +++++
 obexd/src/main.c          |  12 ++
 obexd/src/obex.service.in |   9 --
 obexd/src/obexd.h         |   2 +
 8 files changed, 386 insertions(+), 18 deletions(-)

[PATCH BlueZ v5 1/6] pbap: Support calling pbap_init() after pbap_exit()

[Andrew Sayers]

pbap_exit() didn't previously unregister itself thoroughly.  That
was fine if it was only called when the service was about to exit,
because everything was implicitly unregistered when the process ended.
But we need to be more scrupulous if this can be called throughout
the program's lifecycle.

Send the UnregisterProfile message directly from pbap_exit(),
then call unregister_profile().

The UnregisterProfile message can't be sent directly from
unregister_profile(), because that also needs to be called when
register_profile() fails halfway through.

Signed-off-by: Andrew Sayers <kernel.org@pileofstuff.org>
---
 obexd/client/pbap.c | 16 ++++++++++++++--
 1 file changed, 14 insertions(+), 2 deletions(-)

diff --git a/obexd/client/pbap.c b/obexd/client/pbap.c
index 2f234fadf..90f8bdc02 100644
--- a/obexd/client/pbap.c
+++ b/obexd/client/pbap.c
@@ -1485,8 +1485,20 @@ void pbap_exit(void)
 {
 	DBG("");
 
-	dbus_connection_unref(conn);
-	conn = NULL;
+	g_dbus_remove_watch(system_conn, listener_id);
+
+	unregister_profile();
+
+	if (system_conn) {
+		dbus_connection_close(system_conn);
+		dbus_connection_unref(system_conn);
+		system_conn = NULL;
+	}
+
+	if (conn) {
+		dbus_connection_unref(conn);
+		conn = NULL;
+	}
 
 	obc_driver_unregister(&pbap);
 }
-- 
2.49.0

RE: obexd: unregister profiles when the user is inactive

[bluez.test.bot]

[-- 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=958158

---Test result---

Test Summary:
CheckPatch                    PENDING   0.30 seconds
GitLint                       PENDING   0.30 seconds
BuildEll                      PASS      20.64 seconds
BluezMake                     PASS      2678.12 seconds
MakeCheck                     PASS      20.86 seconds
MakeDistcheck                 PASS      199.06 seconds
CheckValgrind                 PASS      276.37 seconds
CheckSmatch                   PASS      306.71 seconds
bluezmakeextell               PASS      127.62 seconds
IncrementalBuild              PENDING   0.33 seconds
ScanBuild                     PASS      925.74 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 BlueZ v5 2/6] obexd/bluetooth: Support calling bluetooth_init() after bluetooth_exit()

[Andrew Sayers]

bluetooth_exit() didn't previously unregister itself thoroughly.  That
was fine if it was only called when the service was about to exit,
because everything was implicitly unregistered when the process ended.
But we need to be more scrupulous if this can be called throughout
the program's lifecycle.

Send UnregisterProfile messages directly from bluetooth_exit(),
then call unregister_profile(profile).

The UnregisterProfile message can't be sent directly from
unregister_profile(), because that also needs to be called when
register_profile() fails halfway through.

Do not free profiles in bluetooth_exit() - profiles are needed
by a future call to bluetooth_init(), or will be freed by
bluetooth_stop() if necessary.

Signed-off-by: Andrew Sayers <kernel.org@pileofstuff.org>
---
 obexd/plugins/bluetooth.c | 16 ++++++++++++++--
 1 file changed, 14 insertions(+), 2 deletions(-)

diff --git a/obexd/plugins/bluetooth.c b/obexd/plugins/bluetooth.c
index afb2215bd..8cf718922 100644
--- a/obexd/plugins/bluetooth.c
+++ b/obexd/plugins/bluetooth.c
@@ -440,12 +440,24 @@ static int bluetooth_init(void)
 
 static void bluetooth_exit(void)
 {
+	GSList *l;
+
 	g_dbus_remove_watch(connection, listener_id);
 
-	g_slist_free_full(profiles, profile_free);
+	for (l = profiles; l; l = l->next) {
+		struct bluetooth_profile *profile = l->data;
+
+		if (profile->path == NULL)
+			continue;
+
+		unregister_profile(profile);
+	}
 
-	if (connection)
+	if (connection) {
+		dbus_connection_close(connection);
 		dbus_connection_unref(connection);
+		connection = NULL;
+	}
 
 	obex_transport_driver_unregister(&driver);
 }
-- 
2.49.0

[PATCH BlueZ v5 3/6] obexd: Support creating private system/session bus connections

[Andrew Sayers]

Obexd can either connect to the system or session bus.
We mostly share a common connection to that bus, but it can be useful
to have a private connection.  For example, this allows us to quickly
unregister profiles when switching user.

Add obex_setup_dbus_connection_private(), which creates a new
connection to whichever bus was specified by the user.

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

diff --git a/obexd/src/main.c b/obexd/src/main.c
index 703173662..ca95a70de 100644
--- a/obexd/src/main.c
+++ b/obexd/src/main.c
@@ -253,6 +253,14 @@ 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 5e5edc4de..560db29ce 100644
--- a/obexd/src/obexd.h
+++ b/obexd/src/obexd.h
@@ -33,3 +33,5 @@ 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

[PATCH BlueZ v5 4/6] obexd: Unregister profiles when the user is inactive

[Andrew Sayers]

Obexd is usually run as a user service, and can exhibit surprising
behaviour if two users are logged in at the same time.

Unregister profiles when the user is detected to be off-seat.

It may be impossible to detect whether a user is on-seat in some cases.
For example, a version of obexd compiled with systemd support might be
run outside of a systemd environment.  Warn and leave services
registered if that happens.

Obexd can be run as a system service, in which case this check makes no
sense.  Disable this check when called with `--system-bus`.

Obexd can also be run by a user that does not have an active session.
For example, someone could use `ssh` to access the system.  There might
be a use case where someone needs Bluetooth access but can't log in with
a keyboard, or there might be a security issue with doing so.  This isn't
handled explicitly by this patch, but a future patch could add support
by calling `logind_set(FALSE)` in the same way as is currently done
with `--system-bus`.

Unregister profiles by closing private connections instead of sending
UnregisterProfile on the shared connection.  Pipewire has apparently
found the latter to cause long shutdown delays, because bluetoothd
may be shutting down and unable to handle this message.

Based in large part on the wireplumber code mentioned by Pauli Virtanen:
https://gitlab.freedesktop.org/pipewire/wireplumber/-/blob/master/modules/module-logind.c#L52

Other services are likely to need similar functionality,
so I have created a gist to demonstrate the basic technique:
https://gist.github.com/andrew-sayers/1c4a24f86a9a4c1b1e38d109f1bd1d1e

Suggested-by: Pauli Virtanen <pav@iki.fi>
Signed-off-by: Andrew Sayers <kernel.org@pileofstuff.org>
---
 Makefile.obexd            |  10 ++
 obexd/client/pbap.c       |  17 ++-
 obexd/plugins/bluetooth.c |  14 ++-
 obexd/src/logind.c        | 239 ++++++++++++++++++++++++++++++++++++++
 obexd/src/logind.h        |  26 +++++
 obexd/src/main.c          |   4 +
 6 files changed, 305 insertions(+), 5 deletions(-)
 create mode 100644 obexd/src/logind.c
 create mode 100644 obexd/src/logind.h

diff --git a/Makefile.obexd b/Makefile.obexd
index 74dd977a0..b59cfaf8f 100644
--- a/Makefile.obexd
+++ b/Makefile.obexd
@@ -67,6 +67,7 @@ obexd_src_obexd_SOURCES = $(btio_sources) $(gobex_sources) \
 			obexd/src/main.c obexd/src/obexd.h \
 			obexd/src/plugin.h obexd/src/plugin.c \
 			obexd/src/log.h obexd/src/log.c \
+			obexd/src/logind.h obexd/src/logind.c \
 			obexd/src/manager.h obexd/src/manager.c \
 			obexd/src/obex.h obexd/src/obex.c obexd/src/obex-priv.h \
 			obexd/src/mimetype.h obexd/src/mimetype.c \
@@ -96,6 +97,8 @@ obexd_src_obexd_LDADD = lib/libbluetooth-internal.la \
 
 if EXTERNAL_PLUGINS
 obexd_src_obexd_LDFLAGS = $(AM_LDFLAGS) -Wl,--export-dynamic
+else
+obexd_src_obexd_LDFLAGS =
 endif
 
 obexd_src_obexd_CPPFLAGS = $(AM_CPPFLAGS) $(GLIB_CFLAGS) $(DBUS_CFLAGS) \
@@ -109,6 +112,13 @@ obexd-add-service-symlink:
 obexd-remove-service-symlink:
 endif
 
+if OBEX
+if SYSTEMD
+obexd_src_obexd_CPPFLAGS += -DSYSTEMD
+obexd_src_obexd_LDFLAGS += -lsystemd
+endif
+endif
+
 obexd_src_obexd_SHORTNAME = obexd
 
 obexd_builtin_files = obexd/src/builtin.h $(obexd_builtin_nodist)
diff --git a/obexd/client/pbap.c b/obexd/client/pbap.c
index 90f8bdc02..51b523592 100644
--- a/obexd/client/pbap.c
+++ b/obexd/client/pbap.c
@@ -27,6 +27,7 @@
 #include "gdbus/gdbus.h"
 
 #include "obexd/src/log.h"
+#include "obexd/src/logind.h"
 #include "obexd/src/obexd.h"
 
 #include "transfer.h"
@@ -1454,13 +1455,13 @@ static struct obc_driver pbap = {
 	.remove = pbap_remove
 };
 
-int pbap_init(void)
+static int pbap_init_cb(void)
 {
 	int err;
 
 	DBG("");
 
-	conn = obex_get_dbus_connection();
+	conn = obex_setup_dbus_connection_private(NULL, NULL);
 	if (!conn)
 		return -EIO;
 
@@ -1481,7 +1482,7 @@ int pbap_init(void)
 	return 0;
 }
 
-void pbap_exit(void)
+static void pbap_exit_cb(void)
 {
 	DBG("");
 
@@ -1496,9 +1497,19 @@ void pbap_exit(void)
 	}
 
 	if (conn) {
+		dbus_connection_close(conn);
 		dbus_connection_unref(conn);
 		conn = NULL;
 	}
 
 	obc_driver_unregister(&pbap);
 }
+
+int pbap_init(void)
+{
+	return logind_register(pbap_init_cb, pbap_exit_cb);
+}
+void pbap_exit(void)
+{
+	return logind_unregister(pbap_init_cb, pbap_exit_cb);
+}
diff --git a/obexd/plugins/bluetooth.c b/obexd/plugins/bluetooth.c
index 8cf718922..7ff27a8a8 100644
--- a/obexd/plugins/bluetooth.c
+++ b/obexd/plugins/bluetooth.c
@@ -35,6 +35,7 @@
 #include "obexd/src/transport.h"
 #include "obexd/src/service.h"
 #include "obexd/src/log.h"
+#include "obexd/src/logind.h"
 
 #define BT_RX_MTU 32767
 #define BT_TX_MTU 32767
@@ -426,7 +427,7 @@ static const struct obex_transport_driver driver = {
 
 static unsigned int listener_id = 0;
 
-static int bluetooth_init(void)
+static int bluetooth_init_cb(void)
 {
 	connection = g_dbus_setup_private(DBUS_BUS_SYSTEM, NULL, NULL);
 	if (connection == NULL)
@@ -438,7 +439,7 @@ static int bluetooth_init(void)
 	return obex_transport_driver_register(&driver);
 }
 
-static void bluetooth_exit(void)
+static void bluetooth_exit_cb(void)
 {
 	GSList *l;
 
@@ -462,4 +463,13 @@ static void bluetooth_exit(void)
 	obex_transport_driver_unregister(&driver);
 }
 
+static int bluetooth_init(void)
+{
+	return logind_register(bluetooth_init_cb, bluetooth_exit_cb);
+}
+static void bluetooth_exit(void)
+{
+	return logind_unregister(bluetooth_init_cb, bluetooth_exit_cb);
+}
+
 OBEX_PLUGIN_DEFINE(bluetooth, bluetooth_init, bluetooth_exit)
diff --git a/obexd/src/logind.c b/obexd/src/logind.c
new file mode 100644
index 000000000..ff2bf3219
--- /dev/null
+++ b/obexd/src/logind.c
@@ -0,0 +1,239 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ *
+ *  Enable functionality only when the user is active
+ *
+ *  Copyright (C) 2007-2010  Marcel Holtmann <marcel@holtmann.org>
+ *
+ *
+ */
+
+#ifdef SYSTEMD
+
+#include <assert.h>
+#include <errno.h>
+#include <poll.h>
+#include <stddef.h>
+#include <stdlib.h>
+#include <string.h>
+#include <time.h>
+#include <unistd.h>
+#include <glib.h>
+
+#include <systemd/sd-login.h>
+
+#include "obexd/src/log.h"
+#include "obexd/src/logind.h"
+
+static sd_login_monitor * monitor;
+static int uid;
+static gboolean active = FALSE;
+static gboolean monitoring_enabled = TRUE;
+static guint event_source;
+
+struct callback_pair {
+	logind_init_cb init_cb;
+	logind_exit_cb exit_cb;
+};
+
+GSList *callbacks;
+
+static void call_init_cb(gpointer data, gpointer user_data)
+{
+	int res;
+
+	res = ((struct callback_pair *)data)->init_cb();
+	if (res)
+		*(int *)user_data = res;
+}
+static void call_exit_cb(gpointer data, gpointer user_data)
+{
+	((struct callback_pair *)data)->exit_cb();
+}
+
+static int update(void)
+{
+	char *state = NULL;
+	gboolean state_is_active;
+	int res;
+
+	res = sd_login_monitor_flush(monitor);
+	if (res < 0)
+		return res;
+	res = sd_uid_get_state(uid, &state);
+	state_is_active = g_strcmp0(state, "active");
+	free(state);
+	if (res < 0)
+		return res;
+
+	if (state_is_active) {
+		if (!active)
+			return 0;
+	} else {
+		res = sd_uid_get_seats(uid, 1, NULL);
+		if (res < 0)
+			return res;
+		if (active == !!res)
+			return 0;
+	}
+	active ^= TRUE;
+	res = 0;
+	g_slist_foreach(callbacks, active ? call_init_cb : call_exit_cb, &res);
+	return res;
+}
+
+static int check_event(void)
+{
+	int res;
+
+	res = sd_login_monitor_flush(monitor);
+	if (res < 0)
+		return res;
+	if (!monitoring_enabled)
+		return 0;
+	res = update();
+	if (res < 0)
+		return res;
+
+	return 0;
+}
+
+
+static gboolean event_handler(GIOChannel *source, GIOCondition condition,
+				gpointer data)
+{
+	int res;
+
+	res = check_event();
+	if (res) {
+		error("%s: %s", __func__, strerror(-res));
+		return FALSE;
+	}
+
+	return TRUE;
+}
+
+static int logind_init(void)
+{
+	GIOChannel *channel;
+	int events;
+	int fd;
+	int res;
+
+	monitor = NULL;
+
+	DBG("");
+
+	if (!monitoring_enabled)
+		return 0;
+
+	uid = getuid();
+
+	res = sd_login_monitor_new("uid", &monitor);
+	if (res < 0) {
+		monitor = NULL;
+		goto FAIL;
+	}
+
+	// Check this after creating the monitor, in case of race conditions:
+	res = update();
+	if (res < 0)
+		goto FAIL;
+
+	events = res = sd_login_monitor_get_events(monitor);
+	if (res < 0)
+		goto FAIL;
+
+	fd = res = sd_login_monitor_get_fd(monitor);
+	if (res < 0)
+		goto FAIL;
+
+	channel = g_io_channel_unix_new(fd);
+
+	g_io_channel_set_close_on_unref(channel, TRUE);
+	g_io_channel_set_encoding(channel, NULL, NULL);
+	g_io_channel_set_buffered(channel, FALSE);
+
+	event_source = g_io_add_watch(channel, events, event_handler, NULL);
+
+	g_io_channel_unref(channel);
+
+	return check_event();
+
+FAIL:
+	sd_login_monitor_unref(monitor);
+	monitoring_enabled = FALSE;
+	active = TRUE;
+	return res;
+}
+
+static void logind_exit(void)
+{
+	if (event_source) {
+		g_source_remove(event_source);
+		event_source = 0;
+	}
+	sd_login_monitor_unref(monitor);
+}
+
+static gint find_cb(gconstpointer a, gconstpointer b)
+{
+	return ((struct callback_pair *)a)->init_cb - (logind_init_cb)b;
+}
+
+int logind_register(logind_init_cb init_cb, logind_exit_cb exit_cb)
+{
+	struct callback_pair *cbs;
+
+	if (!monitoring_enabled)
+		return init_cb();
+	if (callbacks == NULL) {
+		int res;
+
+		res = logind_init();
+		if (res) {
+			error("logind_init(): %s - login detection disabled",
+				strerror(-res));
+			return init_cb();
+		}
+	}
+	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;
+}
+void logind_unregister(logind_init_cb init_cb, logind_exit_cb exit_cb)
+{
+	GSList *cb_node;
+
+	if (!monitoring_enabled)
+		return exit_cb();
+	if (active)
+		exit_cb();
+	cb_node = g_slist_find_custom(callbacks, init_cb, find_cb);
+	if (cb_node != NULL)
+		callbacks = g_slist_delete_link(callbacks, cb_node);
+	if (callbacks == NULL)
+		logind_exit();
+}
+
+int logind_set(gboolean enabled)
+{
+	int res = 0;
+
+	if (monitoring_enabled == enabled)
+		return 0;
+
+	monitoring_enabled = enabled;
+	if (enabled) {
+		active = FALSE;
+		return update();
+	}
+
+	active = TRUE;
+	g_slist_foreach(callbacks, call_exit_cb, &res);
+	return res;
+}
+
+#endif
diff --git a/obexd/src/logind.h b/obexd/src/logind.h
new file mode 100644
index 000000000..1a92a8b87
--- /dev/null
+++ b/obexd/src/logind.h
@@ -0,0 +1,26 @@
+/* SPDX-License-Identifier: GPL-2.0-or-later */
+/*
+ *
+ *  Enable functionality only when the user is active
+ *
+ *  Copyright (C) 2007-2010  Marcel Holtmann <marcel@holtmann.org>
+ *
+ *
+ */
+
+#ifdef SYSTEMD
+
+typedef int (*logind_init_cb)(void);
+typedef void (*logind_exit_cb)(void);
+
+int logind_register(logind_init_cb init_cb, logind_exit_cb exit_cb);
+void logind_unregister(logind_init_cb init_cb, logind_exit_cb exit_cb);
+int logind_set(gboolean enabled);
+
+#else
+
+#define logind_register(init_cb, exit_cb) init_cb()
+#define logind_unregister(init_cb, exit_cb) exit_cb()
+#define logind_set(enabled) 0
+
+#endif
diff --git a/obexd/src/main.c b/obexd/src/main.c
index ca95a70de..df150973e 100644
--- a/obexd/src/main.c
+++ b/obexd/src/main.c
@@ -35,6 +35,7 @@
 #include "../client/manager.h"
 
 #include "log.h"
+#include "logind.h"
 #include "obexd.h"
 #include "server.h"
 
@@ -283,6 +284,9 @@ int main(int argc, char *argv[])
 
 	__obex_log_init(option_debug, option_detach);
 
+	if (option_system_bus)
+		logind_set(FALSE);
+
 	DBG("Entering main loop");
 
 	main_loop = g_main_loop_new(NULL, FALSE);
-- 
2.49.0

Re: [PATCH BlueZ v5 4/6] obexd: Unregister profiles when the user is inactive

[Luiz Augusto von Dentz]

Hi Andrew,

On Tue, Apr 29, 2025 at 10:15 AM Andrew Sayers
<kernel.org@pileofstuff.org> wrote:
>
> Obexd is usually run as a user service, and can exhibit surprising
> behaviour if two users are logged in at the same time.
>
> Unregister profiles when the user is detected to be off-seat.
>
> It may be impossible to detect whether a user is on-seat in some cases.
> For example, a version of obexd compiled with systemd support might be
> run outside of a systemd environment.  Warn and leave services
> registered if that happens.
>
> Obexd can be run as a system service, in which case this check makes no
> sense.  Disable this check when called with `--system-bus`.
>
> Obexd can also be run by a user that does not have an active session.
> For example, someone could use `ssh` to access the system.  There might
> be a use case where someone needs Bluetooth access but can't log in with
> a keyboard, or there might be a security issue with doing so.  This isn't
> handled explicitly by this patch, but a future patch could add support
> by calling `logind_set(FALSE)` in the same way as is currently done
> with `--system-bus`.
>
> Unregister profiles by closing private connections instead of sending
> UnregisterProfile on the shared connection.  Pipewire has apparently
> found the latter to cause long shutdown delays, because bluetoothd
> may be shutting down and unable to handle this message.
>
> Based in large part on the wireplumber code mentioned by Pauli Virtanen:
> https://gitlab.freedesktop.org/pipewire/wireplumber/-/blob/master/modules/module-logind.c#L52
>
> Other services are likely to need similar functionality,
> so I have created a gist to demonstrate the basic technique:
> https://gist.github.com/andrew-sayers/1c4a24f86a9a4c1b1e38d109f1bd1d1e
>
> Suggested-by: Pauli Virtanen <pav@iki.fi>
> Signed-off-by: Andrew Sayers <kernel.org@pileofstuff.org>
> ---
>  Makefile.obexd            |  10 ++
>  obexd/client/pbap.c       |  17 ++-
>  obexd/plugins/bluetooth.c |  14 ++-
>  obexd/src/logind.c        | 239 ++++++++++++++++++++++++++++++++++++++
>  obexd/src/logind.h        |  26 +++++
>  obexd/src/main.c          |   4 +
>  6 files changed, 305 insertions(+), 5 deletions(-)
>  create mode 100644 obexd/src/logind.c
>  create mode 100644 obexd/src/logind.h
>
> diff --git a/Makefile.obexd b/Makefile.obexd
> index 74dd977a0..b59cfaf8f 100644
> --- a/Makefile.obexd
> +++ b/Makefile.obexd
> @@ -67,6 +67,7 @@ obexd_src_obexd_SOURCES = $(btio_sources) $(gobex_sources) \
>                         obexd/src/main.c obexd/src/obexd.h \
>                         obexd/src/plugin.h obexd/src/plugin.c \
>                         obexd/src/log.h obexd/src/log.c \
> +                       obexd/src/logind.h obexd/src/logind.c \
>                         obexd/src/manager.h obexd/src/manager.c \
>                         obexd/src/obex.h obexd/src/obex.c obexd/src/obex-priv.h \
>                         obexd/src/mimetype.h obexd/src/mimetype.c \
> @@ -96,6 +97,8 @@ obexd_src_obexd_LDADD = lib/libbluetooth-internal.la \
>
>  if EXTERNAL_PLUGINS
>  obexd_src_obexd_LDFLAGS = $(AM_LDFLAGS) -Wl,--export-dynamic
> +else
> +obexd_src_obexd_LDFLAGS =
>  endif
>
>  obexd_src_obexd_CPPFLAGS = $(AM_CPPFLAGS) $(GLIB_CFLAGS) $(DBUS_CFLAGS) \
> @@ -109,6 +112,13 @@ obexd-add-service-symlink:
>  obexd-remove-service-symlink:
>  endif
>
> +if OBEX
> +if SYSTEMD
> +obexd_src_obexd_CPPFLAGS += -DSYSTEMD
> +obexd_src_obexd_LDFLAGS += -lsystemd
> +endif
> +endif
> +
>  obexd_src_obexd_SHORTNAME = obexd
>
>  obexd_builtin_files = obexd/src/builtin.h $(obexd_builtin_nodist)
> diff --git a/obexd/client/pbap.c b/obexd/client/pbap.c
> index 90f8bdc02..51b523592 100644
> --- a/obexd/client/pbap.c
> +++ b/obexd/client/pbap.c
> @@ -27,6 +27,7 @@
>  #include "gdbus/gdbus.h"
>
>  #include "obexd/src/log.h"
> +#include "obexd/src/logind.h"
>  #include "obexd/src/obexd.h"
>
>  #include "transfer.h"
> @@ -1454,13 +1455,13 @@ static struct obc_driver pbap = {
>         .remove = pbap_remove
>  };
>
> -int pbap_init(void)
> +static int pbap_init_cb(void)
>  {
>         int err;
>
>         DBG("");
>
> -       conn = obex_get_dbus_connection();
> +       conn = obex_setup_dbus_connection_private(NULL, NULL);
>         if (!conn)
>                 return -EIO;
>
> @@ -1481,7 +1482,7 @@ int pbap_init(void)
>         return 0;
>  }
>
> -void pbap_exit(void)
> +static void pbap_exit_cb(void)
>  {
>         DBG("");
>
> @@ -1496,9 +1497,19 @@ void pbap_exit(void)
>         }
>
>         if (conn) {
> +               dbus_connection_close(conn);
>                 dbus_connection_unref(conn);
>                 conn = NULL;
>         }
>
>         obc_driver_unregister(&pbap);
>  }
> +
> +int pbap_init(void)
> +{
> +       return logind_register(pbap_init_cb, pbap_exit_cb);
> +}
> +void pbap_exit(void)
> +{
> +       return logind_unregister(pbap_init_cb, pbap_exit_cb);
> +}
> diff --git a/obexd/plugins/bluetooth.c b/obexd/plugins/bluetooth.c
> index 8cf718922..7ff27a8a8 100644
> --- a/obexd/plugins/bluetooth.c
> +++ b/obexd/plugins/bluetooth.c
> @@ -35,6 +35,7 @@
>  #include "obexd/src/transport.h"
>  #include "obexd/src/service.h"
>  #include "obexd/src/log.h"
> +#include "obexd/src/logind.h"
>
>  #define BT_RX_MTU 32767
>  #define BT_TX_MTU 32767
> @@ -426,7 +427,7 @@ static const struct obex_transport_driver driver = {
>
>  static unsigned int listener_id = 0;
>
> -static int bluetooth_init(void)
> +static int bluetooth_init_cb(void)
>  {
>         connection = g_dbus_setup_private(DBUS_BUS_SYSTEM, NULL, NULL);
>         if (connection == NULL)
> @@ -438,7 +439,7 @@ static int bluetooth_init(void)
>         return obex_transport_driver_register(&driver);
>  }
>
> -static void bluetooth_exit(void)
> +static void bluetooth_exit_cb(void)
>  {
>         GSList *l;
>
> @@ -462,4 +463,13 @@ static void bluetooth_exit(void)
>         obex_transport_driver_unregister(&driver);
>  }
>
> +static int bluetooth_init(void)
> +{
> +       return logind_register(bluetooth_init_cb, bluetooth_exit_cb);
> +}
> +static void bluetooth_exit(void)
> +{
> +       return logind_unregister(bluetooth_init_cb, bluetooth_exit_cb);
> +}
> +
>  OBEX_PLUGIN_DEFINE(bluetooth, bluetooth_init, bluetooth_exit)
> diff --git a/obexd/src/logind.c b/obexd/src/logind.c
> new file mode 100644
> index 000000000..ff2bf3219
> --- /dev/null
> +++ b/obexd/src/logind.c
> @@ -0,0 +1,239 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + *
> + *  Enable functionality only when the user is active
> + *
> + *  Copyright (C) 2007-2010  Marcel Holtmann <marcel@holtmann.org>
> + *
> + *
> + */
> +
> +#ifdef SYSTEMD
> +
> +#include <assert.h>
> +#include <errno.h>
> +#include <poll.h>
> +#include <stddef.h>
> +#include <stdlib.h>
> +#include <string.h>
> +#include <time.h>
> +#include <unistd.h>
> +#include <glib.h>
> +
> +#include <systemd/sd-login.h>
> +
> +#include "obexd/src/log.h"
> +#include "obexd/src/logind.h"
> +
> +static sd_login_monitor * monitor;
> +static int uid;
> +static gboolean active = FALSE;
> +static gboolean monitoring_enabled = TRUE;
> +static guint event_source;
> +
> +struct callback_pair {
> +       logind_init_cb init_cb;
> +       logind_exit_cb exit_cb;
> +};
> +
> +GSList *callbacks;
> +
> +static void call_init_cb(gpointer data, gpointer user_data)
> +{
> +       int res;
> +
> +       res = ((struct callback_pair *)data)->init_cb();
> +       if (res)
> +               *(int *)user_data = res;
> +}
> +static void call_exit_cb(gpointer data, gpointer user_data)
> +{
> +       ((struct callback_pair *)data)->exit_cb();
> +}
> +
> +static int update(void)
> +{
> +       char *state = NULL;
> +       gboolean state_is_active;
> +       int res;
> +
> +       res = sd_login_monitor_flush(monitor);
> +       if (res < 0)
> +               return res;
> +       res = sd_uid_get_state(uid, &state);
> +       state_is_active = g_strcmp0(state, "active");
> +       free(state);
> +       if (res < 0)
> +               return res;
> +
> +       if (state_is_active) {
> +               if (!active)
> +                       return 0;
> +       } else {
> +               res = sd_uid_get_seats(uid, 1, NULL);
> +               if (res < 0)
> +                       return res;
> +               if (active == !!res)
> +                       return 0;
> +       }
> +       active ^= TRUE;
> +       res = 0;
> +       g_slist_foreach(callbacks, active ? call_init_cb : call_exit_cb, &res);
> +       return res;
> +}
> +
> +static int check_event(void)
> +{
> +       int res;
> +
> +       res = sd_login_monitor_flush(monitor);
> +       if (res < 0)
> +               return res;
> +       if (!monitoring_enabled)
> +               return 0;
> +       res = update();
> +       if (res < 0)
> +               return res;
> +
> +       return 0;
> +}
> +
> +
> +static gboolean event_handler(GIOChannel *source, GIOCondition condition,
> +                               gpointer data)
> +{
> +       int res;
> +
> +       res = check_event();
> +       if (res) {
> +               error("%s: %s", __func__, strerror(-res));
> +               return FALSE;
> +       }
> +
> +       return TRUE;
> +}
> +
> +static int logind_init(void)
> +{
> +       GIOChannel *channel;
> +       int events;
> +       int fd;
> +       int res;
> +
> +       monitor = NULL;
> +
> +       DBG("");
> +
> +       if (!monitoring_enabled)
> +               return 0;
> +
> +       uid = getuid();
> +
> +       res = sd_login_monitor_new("uid", &monitor);
> +       if (res < 0) {
> +               monitor = NULL;
> +               goto FAIL;
> +       }
> +
> +       // Check this after creating the monitor, in case of race conditions:
> +       res = update();
> +       if (res < 0)
> +               goto FAIL;
> +
> +       events = res = sd_login_monitor_get_events(monitor);
> +       if (res < 0)
> +               goto FAIL;
> +
> +       fd = res = sd_login_monitor_get_fd(monitor);
> +       if (res < 0)
> +               goto FAIL;
> +
> +       channel = g_io_channel_unix_new(fd);
> +
> +       g_io_channel_set_close_on_unref(channel, TRUE);
> +       g_io_channel_set_encoding(channel, NULL, NULL);
> +       g_io_channel_set_buffered(channel, FALSE);
> +
> +       event_source = g_io_add_watch(channel, events, event_handler, NULL);
> +
> +       g_io_channel_unref(channel);
> +
> +       return check_event();
> +
> +FAIL:
> +       sd_login_monitor_unref(monitor);
> +       monitoring_enabled = FALSE;
> +       active = TRUE;
> +       return res;
> +}
> +
> +static void logind_exit(void)
> +{
> +       if (event_source) {
> +               g_source_remove(event_source);
> +               event_source = 0;
> +       }
> +       sd_login_monitor_unref(monitor);
> +}
> +
> +static gint find_cb(gconstpointer a, gconstpointer b)
> +{
> +       return ((struct callback_pair *)a)->init_cb - (logind_init_cb)b;
> +}
> +
> +int logind_register(logind_init_cb init_cb, logind_exit_cb exit_cb)
> +{
> +       struct callback_pair *cbs;
> +
> +       if (!monitoring_enabled)
> +               return init_cb();
> +       if (callbacks == NULL) {
> +               int res;
> +
> +               res = logind_init();
> +               if (res) {
> +                       error("logind_init(): %s - login detection disabled",
> +                               strerror(-res));
> +                       return init_cb();
> +               }
> +       }
> +       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;
> +}
> +void logind_unregister(logind_init_cb init_cb, logind_exit_cb exit_cb)
> +{
> +       GSList *cb_node;
> +
> +       if (!monitoring_enabled)
> +               return exit_cb();
> +       if (active)
> +               exit_cb();
> +       cb_node = g_slist_find_custom(callbacks, init_cb, find_cb);
> +       if (cb_node != NULL)
> +               callbacks = g_slist_delete_link(callbacks, cb_node);
> +       if (callbacks == NULL)
> +               logind_exit();
> +}
> +
> +int logind_set(gboolean enabled)
> +{
> +       int res = 0;
> +
> +       if (monitoring_enabled == enabled)
> +               return 0;
> +
> +       monitoring_enabled = enabled;
> +       if (enabled) {
> +               active = FALSE;
> +               return update();
> +       }
> +
> +       active = TRUE;
> +       g_slist_foreach(callbacks, call_exit_cb, &res);
> +       return res;
> +}
> +
> +#endif
> diff --git a/obexd/src/logind.h b/obexd/src/logind.h
> new file mode 100644
> index 000000000..1a92a8b87
> --- /dev/null
> +++ b/obexd/src/logind.h
> @@ -0,0 +1,26 @@
> +/* SPDX-License-Identifier: GPL-2.0-or-later */
> +/*
> + *
> + *  Enable functionality only when the user is active
> + *
> + *  Copyright (C) 2007-2010  Marcel Holtmann <marcel@holtmann.org>
> + *
> + *
> + */
> +
> +#ifdef SYSTEMD
> +
> +typedef int (*logind_init_cb)(void);
> +typedef void (*logind_exit_cb)(void);
> +
> +int logind_register(logind_init_cb init_cb, logind_exit_cb exit_cb);
> +void logind_unregister(logind_init_cb init_cb, logind_exit_cb exit_cb);
> +int logind_set(gboolean enabled);
> +
> +#else
> +
> +#define logind_register(init_cb, exit_cb) init_cb()
> +#define logind_unregister(init_cb, exit_cb) exit_cb()
> +#define logind_set(enabled) 0
> +
> +#endif
> diff --git a/obexd/src/main.c b/obexd/src/main.c
> index ca95a70de..df150973e 100644
> --- a/obexd/src/main.c
> +++ b/obexd/src/main.c
> @@ -35,6 +35,7 @@
>  #include "../client/manager.h"
>
>  #include "log.h"
> +#include "logind.h"
>  #include "obexd.h"
>  #include "server.h"
>
> @@ -283,6 +284,9 @@ int main(int argc, char *argv[])
>
>         __obex_log_init(option_debug, option_detach);
>
> +       if (option_system_bus)
> +               logind_set(FALSE);
> +
>         DBG("Entering main loop");
>
>         main_loop = g_main_loop_new(NULL, FALSE);
> --
> 2.49.0

Applying: obexd: Unregister profiles when the user is inactive
WARNING:MACRO_ARG_UNUSED: Argument 'exit_cb' is not used in function-like macro
#409: FILE: obexd/src/logind.h:22:
+#define logind_register(init_cb, exit_cb) init_cb()

WARNING:MACRO_ARG_UNUSED: Argument 'init_cb' is not used in function-like macro
#410: FILE: obexd/src/logind.h:23:
+#define logind_unregister(init_cb, exit_cb) exit_cb()

WARNING:MACRO_ARG_UNUSED: Argument 'enabled' is not used in function-like macro
#411: FILE: obexd/src/logind.h:24:
+#define logind_set(enabled) 0

-- 
Luiz Augusto von Dentz

Re: [PATCH BlueZ v5 4/6] obexd: Unregister profiles when the user is inactive

[Andrew Sayers]

On Tue, Apr 29, 2025 at 01:25:26PM -0400, Luiz Augusto von Dentz wrote:
> Hi Andrew,
> 
> On Tue, Apr 29, 2025 at 10:15 AM Andrew Sayers
> <kernel.org@pileofstuff.org> wrote:
> >
> > Obexd is usually run as a user service, and can exhibit surprising
> > behaviour if two users are logged in at the same time.
> >
> > Unregister profiles when the user is detected to be off-seat.
> >
> > It may be impossible to detect whether a user is on-seat in some cases.
> > For example, a version of obexd compiled with systemd support might be
> > run outside of a systemd environment.  Warn and leave services
> > registered if that happens.
> >
> > Obexd can be run as a system service, in which case this check makes no
> > sense.  Disable this check when called with `--system-bus`.
> >
> > Obexd can also be run by a user that does not have an active session.
> > For example, someone could use `ssh` to access the system.  There might
> > be a use case where someone needs Bluetooth access but can't log in with
> > a keyboard, or there might be a security issue with doing so.  This isn't
> > handled explicitly by this patch, but a future patch could add support
> > by calling `logind_set(FALSE)` in the same way as is currently done
> > with `--system-bus`.
> >
> > Unregister profiles by closing private connections instead of sending
> > UnregisterProfile on the shared connection.  Pipewire has apparently
> > found the latter to cause long shutdown delays, because bluetoothd
> > may be shutting down and unable to handle this message.
> >
> > Based in large part on the wireplumber code mentioned by Pauli Virtanen:
> > https://gitlab.freedesktop.org/pipewire/wireplumber/-/blob/master/modules/module-logind.c#L52
> >
> > Other services are likely to need similar functionality,
> > so I have created a gist to demonstrate the basic technique:
> > https://gist.github.com/andrew-sayers/1c4a24f86a9a4c1b1e38d109f1bd1d1e
> >
> > Suggested-by: Pauli Virtanen <pav@iki.fi>
> > Signed-off-by: Andrew Sayers <kernel.org@pileofstuff.org>
> > ---
> >  Makefile.obexd            |  10 ++
> >  obexd/client/pbap.c       |  17 ++-
> >  obexd/plugins/bluetooth.c |  14 ++-
> >  obexd/src/logind.c        | 239 ++++++++++++++++++++++++++++++++++++++
> >  obexd/src/logind.h        |  26 +++++
> >  obexd/src/main.c          |   4 +
> >  6 files changed, 305 insertions(+), 5 deletions(-)
> >  create mode 100644 obexd/src/logind.c
> >  create mode 100644 obexd/src/logind.h
> >
> > diff --git a/Makefile.obexd b/Makefile.obexd
> > index 74dd977a0..b59cfaf8f 100644
> > --- a/Makefile.obexd
> > +++ b/Makefile.obexd
> > @@ -67,6 +67,7 @@ obexd_src_obexd_SOURCES = $(btio_sources) $(gobex_sources) \
> >                         obexd/src/main.c obexd/src/obexd.h \
> >                         obexd/src/plugin.h obexd/src/plugin.c \
> >                         obexd/src/log.h obexd/src/log.c \
> > +                       obexd/src/logind.h obexd/src/logind.c \
> >                         obexd/src/manager.h obexd/src/manager.c \
> >                         obexd/src/obex.h obexd/src/obex.c obexd/src/obex-priv.h \
> >                         obexd/src/mimetype.h obexd/src/mimetype.c \
> > @@ -96,6 +97,8 @@ obexd_src_obexd_LDADD = lib/libbluetooth-internal.la \
> >
> >  if EXTERNAL_PLUGINS
> >  obexd_src_obexd_LDFLAGS = $(AM_LDFLAGS) -Wl,--export-dynamic
> > +else
> > +obexd_src_obexd_LDFLAGS =
> >  endif
> >
> >  obexd_src_obexd_CPPFLAGS = $(AM_CPPFLAGS) $(GLIB_CFLAGS) $(DBUS_CFLAGS) \
> > @@ -109,6 +112,13 @@ obexd-add-service-symlink:
> >  obexd-remove-service-symlink:
> >  endif
> >
> > +if OBEX
> > +if SYSTEMD
> > +obexd_src_obexd_CPPFLAGS += -DSYSTEMD
> > +obexd_src_obexd_LDFLAGS += -lsystemd
> > +endif
> > +endif
> > +
> >  obexd_src_obexd_SHORTNAME = obexd
> >
> >  obexd_builtin_files = obexd/src/builtin.h $(obexd_builtin_nodist)
> > diff --git a/obexd/client/pbap.c b/obexd/client/pbap.c
> > index 90f8bdc02..51b523592 100644
> > --- a/obexd/client/pbap.c
> > +++ b/obexd/client/pbap.c
> > @@ -27,6 +27,7 @@
> >  #include "gdbus/gdbus.h"
> >
> >  #include "obexd/src/log.h"
> > +#include "obexd/src/logind.h"
> >  #include "obexd/src/obexd.h"
> >
> >  #include "transfer.h"
> > @@ -1454,13 +1455,13 @@ static struct obc_driver pbap = {
> >         .remove = pbap_remove
> >  };
> >
> > -int pbap_init(void)
> > +static int pbap_init_cb(void)
> >  {
> >         int err;
> >
> >         DBG("");
> >
> > -       conn = obex_get_dbus_connection();
> > +       conn = obex_setup_dbus_connection_private(NULL, NULL);
> >         if (!conn)
> >                 return -EIO;
> >
> > @@ -1481,7 +1482,7 @@ int pbap_init(void)
> >         return 0;
> >  }
> >
> > -void pbap_exit(void)
> > +static void pbap_exit_cb(void)
> >  {
> >         DBG("");
> >
> > @@ -1496,9 +1497,19 @@ void pbap_exit(void)
> >         }
> >
> >         if (conn) {
> > +               dbus_connection_close(conn);
> >                 dbus_connection_unref(conn);
> >                 conn = NULL;
> >         }
> >
> >         obc_driver_unregister(&pbap);
> >  }
> > +
> > +int pbap_init(void)
> > +{
> > +       return logind_register(pbap_init_cb, pbap_exit_cb);
> > +}
> > +void pbap_exit(void)
> > +{
> > +       return logind_unregister(pbap_init_cb, pbap_exit_cb);
> > +}
> > diff --git a/obexd/plugins/bluetooth.c b/obexd/plugins/bluetooth.c
> > index 8cf718922..7ff27a8a8 100644
> > --- a/obexd/plugins/bluetooth.c
> > +++ b/obexd/plugins/bluetooth.c
> > @@ -35,6 +35,7 @@
> >  #include "obexd/src/transport.h"
> >  #include "obexd/src/service.h"
> >  #include "obexd/src/log.h"
> > +#include "obexd/src/logind.h"
> >
> >  #define BT_RX_MTU 32767
> >  #define BT_TX_MTU 32767
> > @@ -426,7 +427,7 @@ static const struct obex_transport_driver driver = {
> >
> >  static unsigned int listener_id = 0;
> >
> > -static int bluetooth_init(void)
> > +static int bluetooth_init_cb(void)
> >  {
> >         connection = g_dbus_setup_private(DBUS_BUS_SYSTEM, NULL, NULL);
> >         if (connection == NULL)
> > @@ -438,7 +439,7 @@ static int bluetooth_init(void)
> >         return obex_transport_driver_register(&driver);
> >  }
> >
> > -static void bluetooth_exit(void)
> > +static void bluetooth_exit_cb(void)
> >  {
> >         GSList *l;
> >
> > @@ -462,4 +463,13 @@ static void bluetooth_exit(void)
> >         obex_transport_driver_unregister(&driver);
> >  }
> >
> > +static int bluetooth_init(void)
> > +{
> > +       return logind_register(bluetooth_init_cb, bluetooth_exit_cb);
> > +}
> > +static void bluetooth_exit(void)
> > +{
> > +       return logind_unregister(bluetooth_init_cb, bluetooth_exit_cb);
> > +}
> > +
> >  OBEX_PLUGIN_DEFINE(bluetooth, bluetooth_init, bluetooth_exit)
> > diff --git a/obexd/src/logind.c b/obexd/src/logind.c
> > new file mode 100644
> > index 000000000..ff2bf3219
> > --- /dev/null
> > +++ b/obexd/src/logind.c
> > @@ -0,0 +1,239 @@
> > +// SPDX-License-Identifier: GPL-2.0-or-later
> > +/*
> > + *
> > + *  Enable functionality only when the user is active
> > + *
> > + *  Copyright (C) 2007-2010  Marcel Holtmann <marcel@holtmann.org>
> > + *
> > + *
> > + */
> > +
> > +#ifdef SYSTEMD
> > +
> > +#include <assert.h>
> > +#include <errno.h>
> > +#include <poll.h>
> > +#include <stddef.h>
> > +#include <stdlib.h>
> > +#include <string.h>
> > +#include <time.h>
> > +#include <unistd.h>
> > +#include <glib.h>
> > +
> > +#include <systemd/sd-login.h>
> > +
> > +#include "obexd/src/log.h"
> > +#include "obexd/src/logind.h"
> > +
> > +static sd_login_monitor * monitor;
> > +static int uid;
> > +static gboolean active = FALSE;
> > +static gboolean monitoring_enabled = TRUE;
> > +static guint event_source;
> > +
> > +struct callback_pair {
> > +       logind_init_cb init_cb;
> > +       logind_exit_cb exit_cb;
> > +};
> > +
> > +GSList *callbacks;
> > +
> > +static void call_init_cb(gpointer data, gpointer user_data)
> > +{
> > +       int res;
> > +
> > +       res = ((struct callback_pair *)data)->init_cb();
> > +       if (res)
> > +               *(int *)user_data = res;
> > +}
> > +static void call_exit_cb(gpointer data, gpointer user_data)
> > +{
> > +       ((struct callback_pair *)data)->exit_cb();
> > +}
> > +
> > +static int update(void)
> > +{
> > +       char *state = NULL;
> > +       gboolean state_is_active;
> > +       int res;
> > +
> > +       res = sd_login_monitor_flush(monitor);
> > +       if (res < 0)
> > +               return res;
> > +       res = sd_uid_get_state(uid, &state);
> > +       state_is_active = g_strcmp0(state, "active");
> > +       free(state);
> > +       if (res < 0)
> > +               return res;
> > +
> > +       if (state_is_active) {
> > +               if (!active)
> > +                       return 0;
> > +       } else {
> > +               res = sd_uid_get_seats(uid, 1, NULL);
> > +               if (res < 0)
> > +                       return res;
> > +               if (active == !!res)
> > +                       return 0;
> > +       }
> > +       active ^= TRUE;
> > +       res = 0;
> > +       g_slist_foreach(callbacks, active ? call_init_cb : call_exit_cb, &res);
> > +       return res;
> > +}
> > +
> > +static int check_event(void)
> > +{
> > +       int res;
> > +
> > +       res = sd_login_monitor_flush(monitor);
> > +       if (res < 0)
> > +               return res;
> > +       if (!monitoring_enabled)
> > +               return 0;
> > +       res = update();
> > +       if (res < 0)
> > +               return res;
> > +
> > +       return 0;
> > +}
> > +
> > +
> > +static gboolean event_handler(GIOChannel *source, GIOCondition condition,
> > +                               gpointer data)
> > +{
> > +       int res;
> > +
> > +       res = check_event();
> > +       if (res) {
> > +               error("%s: %s", __func__, strerror(-res));
> > +               return FALSE;
> > +       }
> > +
> > +       return TRUE;
> > +}
> > +
> > +static int logind_init(void)
> > +{
> > +       GIOChannel *channel;
> > +       int events;
> > +       int fd;
> > +       int res;
> > +
> > +       monitor = NULL;
> > +
> > +       DBG("");
> > +
> > +       if (!monitoring_enabled)
> > +               return 0;
> > +
> > +       uid = getuid();
> > +
> > +       res = sd_login_monitor_new("uid", &monitor);
> > +       if (res < 0) {
> > +               monitor = NULL;
> > +               goto FAIL;
> > +       }
> > +
> > +       // Check this after creating the monitor, in case of race conditions:
> > +       res = update();
> > +       if (res < 0)
> > +               goto FAIL;
> > +
> > +       events = res = sd_login_monitor_get_events(monitor);
> > +       if (res < 0)
> > +               goto FAIL;
> > +
> > +       fd = res = sd_login_monitor_get_fd(monitor);
> > +       if (res < 0)
> > +               goto FAIL;
> > +
> > +       channel = g_io_channel_unix_new(fd);
> > +
> > +       g_io_channel_set_close_on_unref(channel, TRUE);
> > +       g_io_channel_set_encoding(channel, NULL, NULL);
> > +       g_io_channel_set_buffered(channel, FALSE);
> > +
> > +       event_source = g_io_add_watch(channel, events, event_handler, NULL);
> > +
> > +       g_io_channel_unref(channel);
> > +
> > +       return check_event();
> > +
> > +FAIL:
> > +       sd_login_monitor_unref(monitor);
> > +       monitoring_enabled = FALSE;
> > +       active = TRUE;
> > +       return res;
> > +}
> > +
> > +static void logind_exit(void)
> > +{
> > +       if (event_source) {
> > +               g_source_remove(event_source);
> > +               event_source = 0;
> > +       }
> > +       sd_login_monitor_unref(monitor);
> > +}
> > +
> > +static gint find_cb(gconstpointer a, gconstpointer b)
> > +{
> > +       return ((struct callback_pair *)a)->init_cb - (logind_init_cb)b;
> > +}
> > +
> > +int logind_register(logind_init_cb init_cb, logind_exit_cb exit_cb)
> > +{
> > +       struct callback_pair *cbs;
> > +
> > +       if (!monitoring_enabled)
> > +               return init_cb();
> > +       if (callbacks == NULL) {
> > +               int res;
> > +
> > +               res = logind_init();
> > +               if (res) {
> > +                       error("logind_init(): %s - login detection disabled",
> > +                               strerror(-res));
> > +                       return init_cb();
> > +               }
> > +       }
> > +       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;
> > +}
> > +void logind_unregister(logind_init_cb init_cb, logind_exit_cb exit_cb)
> > +{
> > +       GSList *cb_node;
> > +
> > +       if (!monitoring_enabled)
> > +               return exit_cb();
> > +       if (active)
> > +               exit_cb();
> > +       cb_node = g_slist_find_custom(callbacks, init_cb, find_cb);
> > +       if (cb_node != NULL)
> > +               callbacks = g_slist_delete_link(callbacks, cb_node);
> > +       if (callbacks == NULL)
> > +               logind_exit();
> > +}
> > +
> > +int logind_set(gboolean enabled)
> > +{
> > +       int res = 0;
> > +
> > +       if (monitoring_enabled == enabled)
> > +               return 0;
> > +
> > +       monitoring_enabled = enabled;
> > +       if (enabled) {
> > +               active = FALSE;
> > +               return update();
> > +       }
> > +
> > +       active = TRUE;
> > +       g_slist_foreach(callbacks, call_exit_cb, &res);
> > +       return res;
> > +}
> > +
> > +#endif
> > diff --git a/obexd/src/logind.h b/obexd/src/logind.h
> > new file mode 100644
> > index 000000000..1a92a8b87
> > --- /dev/null
> > +++ b/obexd/src/logind.h
> > @@ -0,0 +1,26 @@
> > +/* SPDX-License-Identifier: GPL-2.0-or-later */
> > +/*
> > + *
> > + *  Enable functionality only when the user is active
> > + *
> > + *  Copyright (C) 2007-2010  Marcel Holtmann <marcel@holtmann.org>
> > + *
> > + *
> > + */
> > +
> > +#ifdef SYSTEMD
> > +
> > +typedef int (*logind_init_cb)(void);
> > +typedef void (*logind_exit_cb)(void);
> > +
> > +int logind_register(logind_init_cb init_cb, logind_exit_cb exit_cb);
> > +void logind_unregister(logind_init_cb init_cb, logind_exit_cb exit_cb);
> > +int logind_set(gboolean enabled);
> > +
> > +#else
> > +
> > +#define logind_register(init_cb, exit_cb) init_cb()
> > +#define logind_unregister(init_cb, exit_cb) exit_cb()
> > +#define logind_set(enabled) 0
> > +
> > +#endif
> > diff --git a/obexd/src/main.c b/obexd/src/main.c
> > index ca95a70de..df150973e 100644
> > --- a/obexd/src/main.c
> > +++ b/obexd/src/main.c
> > @@ -35,6 +35,7 @@
> >  #include "../client/manager.h"
> >
> >  #include "log.h"
> > +#include "logind.h"
> >  #include "obexd.h"
> >  #include "server.h"
> >
> > @@ -283,6 +284,9 @@ int main(int argc, char *argv[])
> >
> >         __obex_log_init(option_debug, option_detach);
> >
> > +       if (option_system_bus)
> > +               logind_set(FALSE);
> > +
> >         DBG("Entering main loop");
> >
> >         main_loop = g_main_loop_new(NULL, FALSE);
> > --
> > 2.49.0
> 
> Applying: obexd: Unregister profiles when the user is inactive
> WARNING:MACRO_ARG_UNUSED: Argument 'exit_cb' is not used in function-like macro
> #409: FILE: obexd/src/logind.h:22:
> +#define logind_register(init_cb, exit_cb) init_cb()
> 
> WARNING:MACRO_ARG_UNUSED: Argument 'init_cb' is not used in function-like macro
> #410: FILE: obexd/src/logind.h:23:
> +#define logind_unregister(init_cb, exit_cb) exit_cb()
> 
> WARNING:MACRO_ARG_UNUSED: Argument 'enabled' is not used in function-like macro
> #411: FILE: obexd/src/logind.h:24:
> +#define logind_set(enabled) 0

Those #define's are dummy implementations for use when systemd is disabled.
Which of these would you rather silence the warnings with?

    // inline functions:
    static inline int logind_set(gboolean enabled) { return 0; }

    // comma operator:
    #define logind_set(enabled) (enabled,0)

    // varargs:
    #define login_set(...) 0

> 
> -- 
> Luiz Augusto von Dentz
> 

Re: [PATCH BlueZ v5 4/6] obexd: Unregister profiles when the user is inactive

[Luiz Augusto von Dentz]

Hi Andrew,

On Tue, Apr 29, 2025 at 3:20 PM Andrew Sayers
<kernel.org@pileofstuff.org> wrote:
>
> On Tue, Apr 29, 2025 at 01:25:26PM -0400, Luiz Augusto von Dentz wrote:
> > Hi Andrew,
> >
> > On Tue, Apr 29, 2025 at 10:15 AM Andrew Sayers
> > <kernel.org@pileofstuff.org> wrote:
> > >
> > > Obexd is usually run as a user service, and can exhibit surprising
> > > behaviour if two users are logged in at the same time.
> > >
> > > Unregister profiles when the user is detected to be off-seat.
> > >
> > > It may be impossible to detect whether a user is on-seat in some cases.
> > > For example, a version of obexd compiled with systemd support might be
> > > run outside of a systemd environment.  Warn and leave services
> > > registered if that happens.
> > >
> > > Obexd can be run as a system service, in which case this check makes no
> > > sense.  Disable this check when called with `--system-bus`.
> > >
> > > Obexd can also be run by a user that does not have an active session.
> > > For example, someone could use `ssh` to access the system.  There might
> > > be a use case where someone needs Bluetooth access but can't log in with
> > > a keyboard, or there might be a security issue with doing so.  This isn't
> > > handled explicitly by this patch, but a future patch could add support
> > > by calling `logind_set(FALSE)` in the same way as is currently done
> > > with `--system-bus`.
> > >
> > > Unregister profiles by closing private connections instead of sending
> > > UnregisterProfile on the shared connection.  Pipewire has apparently
> > > found the latter to cause long shutdown delays, because bluetoothd
> > > may be shutting down and unable to handle this message.
> > >
> > > Based in large part on the wireplumber code mentioned by Pauli Virtanen:
> > > https://gitlab.freedesktop.org/pipewire/wireplumber/-/blob/master/modules/module-logind.c#L52
> > >
> > > Other services are likely to need similar functionality,
> > > so I have created a gist to demonstrate the basic technique:
> > > https://gist.github.com/andrew-sayers/1c4a24f86a9a4c1b1e38d109f1bd1d1e
> > >
> > > Suggested-by: Pauli Virtanen <pav@iki.fi>
> > > Signed-off-by: Andrew Sayers <kernel.org@pileofstuff.org>
> > > ---
> > >  Makefile.obexd            |  10 ++
> > >  obexd/client/pbap.c       |  17 ++-
> > >  obexd/plugins/bluetooth.c |  14 ++-
> > >  obexd/src/logind.c        | 239 ++++++++++++++++++++++++++++++++++++++
> > >  obexd/src/logind.h        |  26 +++++
> > >  obexd/src/main.c          |   4 +
> > >  6 files changed, 305 insertions(+), 5 deletions(-)
> > >  create mode 100644 obexd/src/logind.c
> > >  create mode 100644 obexd/src/logind.h
> > >
> > > diff --git a/Makefile.obexd b/Makefile.obexd
> > > index 74dd977a0..b59cfaf8f 100644
> > > --- a/Makefile.obexd
> > > +++ b/Makefile.obexd
> > > @@ -67,6 +67,7 @@ obexd_src_obexd_SOURCES = $(btio_sources) $(gobex_sources) \
> > >                         obexd/src/main.c obexd/src/obexd.h \
> > >                         obexd/src/plugin.h obexd/src/plugin.c \
> > >                         obexd/src/log.h obexd/src/log.c \
> > > +                       obexd/src/logind.h obexd/src/logind.c \
> > >                         obexd/src/manager.h obexd/src/manager.c \
> > >                         obexd/src/obex.h obexd/src/obex.c obexd/src/obex-priv.h \
> > >                         obexd/src/mimetype.h obexd/src/mimetype.c \
> > > @@ -96,6 +97,8 @@ obexd_src_obexd_LDADD = lib/libbluetooth-internal.la \
> > >
> > >  if EXTERNAL_PLUGINS
> > >  obexd_src_obexd_LDFLAGS = $(AM_LDFLAGS) -Wl,--export-dynamic
> > > +else
> > > +obexd_src_obexd_LDFLAGS =
> > >  endif
> > >
> > >  obexd_src_obexd_CPPFLAGS = $(AM_CPPFLAGS) $(GLIB_CFLAGS) $(DBUS_CFLAGS) \
> > > @@ -109,6 +112,13 @@ obexd-add-service-symlink:
> > >  obexd-remove-service-symlink:
> > >  endif
> > >
> > > +if OBEX
> > > +if SYSTEMD
> > > +obexd_src_obexd_CPPFLAGS += -DSYSTEMD
> > > +obexd_src_obexd_LDFLAGS += -lsystemd
> > > +endif
> > > +endif
> > > +
> > >  obexd_src_obexd_SHORTNAME = obexd
> > >
> > >  obexd_builtin_files = obexd/src/builtin.h $(obexd_builtin_nodist)
> > > diff --git a/obexd/client/pbap.c b/obexd/client/pbap.c
> > > index 90f8bdc02..51b523592 100644
> > > --- a/obexd/client/pbap.c
> > > +++ b/obexd/client/pbap.c
> > > @@ -27,6 +27,7 @@
> > >  #include "gdbus/gdbus.h"
> > >
> > >  #include "obexd/src/log.h"
> > > +#include "obexd/src/logind.h"
> > >  #include "obexd/src/obexd.h"
> > >
> > >  #include "transfer.h"
> > > @@ -1454,13 +1455,13 @@ static struct obc_driver pbap = {
> > >         .remove = pbap_remove
> > >  };
> > >
> > > -int pbap_init(void)
> > > +static int pbap_init_cb(void)
> > >  {
> > >         int err;
> > >
> > >         DBG("");
> > >
> > > -       conn = obex_get_dbus_connection();
> > > +       conn = obex_setup_dbus_connection_private(NULL, NULL);
> > >         if (!conn)
> > >                 return -EIO;
> > >
> > > @@ -1481,7 +1482,7 @@ int pbap_init(void)
> > >         return 0;
> > >  }
> > >
> > > -void pbap_exit(void)
> > > +static void pbap_exit_cb(void)
> > >  {
> > >         DBG("");
> > >
> > > @@ -1496,9 +1497,19 @@ void pbap_exit(void)
> > >         }
> > >
> > >         if (conn) {
> > > +               dbus_connection_close(conn);
> > >                 dbus_connection_unref(conn);
> > >                 conn = NULL;
> > >         }
> > >
> > >         obc_driver_unregister(&pbap);
> > >  }
> > > +
> > > +int pbap_init(void)
> > > +{
> > > +       return logind_register(pbap_init_cb, pbap_exit_cb);
> > > +}
> > > +void pbap_exit(void)
> > > +{
> > > +       return logind_unregister(pbap_init_cb, pbap_exit_cb);
> > > +}
> > > diff --git a/obexd/plugins/bluetooth.c b/obexd/plugins/bluetooth.c
> > > index 8cf718922..7ff27a8a8 100644
> > > --- a/obexd/plugins/bluetooth.c
> > > +++ b/obexd/plugins/bluetooth.c
> > > @@ -35,6 +35,7 @@
> > >  #include "obexd/src/transport.h"
> > >  #include "obexd/src/service.h"
> > >  #include "obexd/src/log.h"
> > > +#include "obexd/src/logind.h"
> > >
> > >  #define BT_RX_MTU 32767
> > >  #define BT_TX_MTU 32767
> > > @@ -426,7 +427,7 @@ static const struct obex_transport_driver driver = {
> > >
> > >  static unsigned int listener_id = 0;
> > >
> > > -static int bluetooth_init(void)
> > > +static int bluetooth_init_cb(void)
> > >  {
> > >         connection = g_dbus_setup_private(DBUS_BUS_SYSTEM, NULL, NULL);
> > >         if (connection == NULL)
> > > @@ -438,7 +439,7 @@ static int bluetooth_init(void)
> > >         return obex_transport_driver_register(&driver);
> > >  }
> > >
> > > -static void bluetooth_exit(void)
> > > +static void bluetooth_exit_cb(void)
> > >  {
> > >         GSList *l;
> > >
> > > @@ -462,4 +463,13 @@ static void bluetooth_exit(void)
> > >         obex_transport_driver_unregister(&driver);
> > >  }
> > >
> > > +static int bluetooth_init(void)
> > > +{
> > > +       return logind_register(bluetooth_init_cb, bluetooth_exit_cb);
> > > +}
> > > +static void bluetooth_exit(void)
> > > +{
> > > +       return logind_unregister(bluetooth_init_cb, bluetooth_exit_cb);
> > > +}
> > > +
> > >  OBEX_PLUGIN_DEFINE(bluetooth, bluetooth_init, bluetooth_exit)
> > > diff --git a/obexd/src/logind.c b/obexd/src/logind.c
> > > new file mode 100644
> > > index 000000000..ff2bf3219
> > > --- /dev/null
> > > +++ b/obexd/src/logind.c
> > > @@ -0,0 +1,239 @@
> > > +// SPDX-License-Identifier: GPL-2.0-or-later
> > > +/*
> > > + *
> > > + *  Enable functionality only when the user is active
> > > + *
> > > + *  Copyright (C) 2007-2010  Marcel Holtmann <marcel@holtmann.org>
> > > + *
> > > + *
> > > + */
> > > +
> > > +#ifdef SYSTEMD
> > > +
> > > +#include <assert.h>
> > > +#include <errno.h>
> > > +#include <poll.h>
> > > +#include <stddef.h>
> > > +#include <stdlib.h>
> > > +#include <string.h>
> > > +#include <time.h>
> > > +#include <unistd.h>
> > > +#include <glib.h>
> > > +
> > > +#include <systemd/sd-login.h>
> > > +
> > > +#include "obexd/src/log.h"
> > > +#include "obexd/src/logind.h"
> > > +
> > > +static sd_login_monitor * monitor;
> > > +static int uid;
> > > +static gboolean active = FALSE;
> > > +static gboolean monitoring_enabled = TRUE;
> > > +static guint event_source;
> > > +
> > > +struct callback_pair {
> > > +       logind_init_cb init_cb;
> > > +       logind_exit_cb exit_cb;
> > > +};
> > > +
> > > +GSList *callbacks;
> > > +
> > > +static void call_init_cb(gpointer data, gpointer user_data)
> > > +{
> > > +       int res;
> > > +
> > > +       res = ((struct callback_pair *)data)->init_cb();
> > > +       if (res)
> > > +               *(int *)user_data = res;
> > > +}
> > > +static void call_exit_cb(gpointer data, gpointer user_data)
> > > +{
> > > +       ((struct callback_pair *)data)->exit_cb();
> > > +}
> > > +
> > > +static int update(void)
> > > +{
> > > +       char *state = NULL;
> > > +       gboolean state_is_active;
> > > +       int res;
> > > +
> > > +       res = sd_login_monitor_flush(monitor);
> > > +       if (res < 0)
> > > +               return res;
> > > +       res = sd_uid_get_state(uid, &state);
> > > +       state_is_active = g_strcmp0(state, "active");
> > > +       free(state);
> > > +       if (res < 0)
> > > +               return res;
> > > +
> > > +       if (state_is_active) {
> > > +               if (!active)
> > > +                       return 0;
> > > +       } else {
> > > +               res = sd_uid_get_seats(uid, 1, NULL);
> > > +               if (res < 0)
> > > +                       return res;
> > > +               if (active == !!res)
> > > +                       return 0;
> > > +       }
> > > +       active ^= TRUE;
> > > +       res = 0;
> > > +       g_slist_foreach(callbacks, active ? call_init_cb : call_exit_cb, &res);
> > > +       return res;
> > > +}
> > > +
> > > +static int check_event(void)
> > > +{
> > > +       int res;
> > > +
> > > +       res = sd_login_monitor_flush(monitor);
> > > +       if (res < 0)
> > > +               return res;
> > > +       if (!monitoring_enabled)
> > > +               return 0;
> > > +       res = update();
> > > +       if (res < 0)
> > > +               return res;
> > > +
> > > +       return 0;
> > > +}
> > > +
> > > +
> > > +static gboolean event_handler(GIOChannel *source, GIOCondition condition,
> > > +                               gpointer data)
> > > +{
> > > +       int res;
> > > +
> > > +       res = check_event();
> > > +       if (res) {
> > > +               error("%s: %s", __func__, strerror(-res));
> > > +               return FALSE;
> > > +       }
> > > +
> > > +       return TRUE;
> > > +}
> > > +
> > > +static int logind_init(void)
> > > +{
> > > +       GIOChannel *channel;
> > > +       int events;
> > > +       int fd;
> > > +       int res;
> > > +
> > > +       monitor = NULL;
> > > +
> > > +       DBG("");
> > > +
> > > +       if (!monitoring_enabled)
> > > +               return 0;
> > > +
> > > +       uid = getuid();
> > > +
> > > +       res = sd_login_monitor_new("uid", &monitor);
> > > +       if (res < 0) {
> > > +               monitor = NULL;
> > > +               goto FAIL;
> > > +       }
> > > +
> > > +       // Check this after creating the monitor, in case of race conditions:
> > > +       res = update();
> > > +       if (res < 0)
> > > +               goto FAIL;
> > > +
> > > +       events = res = sd_login_monitor_get_events(monitor);
> > > +       if (res < 0)
> > > +               goto FAIL;
> > > +
> > > +       fd = res = sd_login_monitor_get_fd(monitor);
> > > +       if (res < 0)
> > > +               goto FAIL;
> > > +
> > > +       channel = g_io_channel_unix_new(fd);
> > > +
> > > +       g_io_channel_set_close_on_unref(channel, TRUE);
> > > +       g_io_channel_set_encoding(channel, NULL, NULL);
> > > +       g_io_channel_set_buffered(channel, FALSE);
> > > +
> > > +       event_source = g_io_add_watch(channel, events, event_handler, NULL);
> > > +
> > > +       g_io_channel_unref(channel);
> > > +
> > > +       return check_event();
> > > +
> > > +FAIL:
> > > +       sd_login_monitor_unref(monitor);
> > > +       monitoring_enabled = FALSE;
> > > +       active = TRUE;
> > > +       return res;
> > > +}
> > > +
> > > +static void logind_exit(void)
> > > +{
> > > +       if (event_source) {
> > > +               g_source_remove(event_source);
> > > +               event_source = 0;
> > > +       }
> > > +       sd_login_monitor_unref(monitor);
> > > +}
> > > +
> > > +static gint find_cb(gconstpointer a, gconstpointer b)
> > > +{
> > > +       return ((struct callback_pair *)a)->init_cb - (logind_init_cb)b;
> > > +}
> > > +
> > > +int logind_register(logind_init_cb init_cb, logind_exit_cb exit_cb)
> > > +{
> > > +       struct callback_pair *cbs;
> > > +
> > > +       if (!monitoring_enabled)
> > > +               return init_cb();
> > > +       if (callbacks == NULL) {
> > > +               int res;
> > > +
> > > +               res = logind_init();
> > > +               if (res) {
> > > +                       error("logind_init(): %s - login detection disabled",
> > > +                               strerror(-res));
> > > +                       return init_cb();
> > > +               }
> > > +       }
> > > +       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;
> > > +}
> > > +void logind_unregister(logind_init_cb init_cb, logind_exit_cb exit_cb)
> > > +{
> > > +       GSList *cb_node;
> > > +
> > > +       if (!monitoring_enabled)
> > > +               return exit_cb();
> > > +       if (active)
> > > +               exit_cb();
> > > +       cb_node = g_slist_find_custom(callbacks, init_cb, find_cb);
> > > +       if (cb_node != NULL)
> > > +               callbacks = g_slist_delete_link(callbacks, cb_node);
> > > +       if (callbacks == NULL)
> > > +               logind_exit();
> > > +}
> > > +
> > > +int logind_set(gboolean enabled)
> > > +{
> > > +       int res = 0;
> > > +
> > > +       if (monitoring_enabled == enabled)
> > > +               return 0;
> > > +
> > > +       monitoring_enabled = enabled;
> > > +       if (enabled) {
> > > +               active = FALSE;
> > > +               return update();
> > > +       }
> > > +
> > > +       active = TRUE;
> > > +       g_slist_foreach(callbacks, call_exit_cb, &res);
> > > +       return res;
> > > +}
> > > +
> > > +#endif
> > > diff --git a/obexd/src/logind.h b/obexd/src/logind.h
> > > new file mode 100644
> > > index 000000000..1a92a8b87
> > > --- /dev/null
> > > +++ b/obexd/src/logind.h
> > > @@ -0,0 +1,26 @@
> > > +/* SPDX-License-Identifier: GPL-2.0-or-later */
> > > +/*
> > > + *
> > > + *  Enable functionality only when the user is active
> > > + *
> > > + *  Copyright (C) 2007-2010  Marcel Holtmann <marcel@holtmann.org>
> > > + *
> > > + *
> > > + */
> > > +
> > > +#ifdef SYSTEMD
> > > +
> > > +typedef int (*logind_init_cb)(void);
> > > +typedef void (*logind_exit_cb)(void);
> > > +
> > > +int logind_register(logind_init_cb init_cb, logind_exit_cb exit_cb);
> > > +void logind_unregister(logind_init_cb init_cb, logind_exit_cb exit_cb);
> > > +int logind_set(gboolean enabled);
> > > +
> > > +#else
> > > +
> > > +#define logind_register(init_cb, exit_cb) init_cb()
> > > +#define logind_unregister(init_cb, exit_cb) exit_cb()
> > > +#define logind_set(enabled) 0
> > > +
> > > +#endif
> > > diff --git a/obexd/src/main.c b/obexd/src/main.c
> > > index ca95a70de..df150973e 100644
> > > --- a/obexd/src/main.c
> > > +++ b/obexd/src/main.c
> > > @@ -35,6 +35,7 @@
> > >  #include "../client/manager.h"
> > >
> > >  #include "log.h"
> > > +#include "logind.h"
> > >  #include "obexd.h"
> > >  #include "server.h"
> > >
> > > @@ -283,6 +284,9 @@ int main(int argc, char *argv[])
> > >
> > >         __obex_log_init(option_debug, option_detach);
> > >
> > > +       if (option_system_bus)
> > > +               logind_set(FALSE);
> > > +
> > >         DBG("Entering main loop");
> > >
> > >         main_loop = g_main_loop_new(NULL, FALSE);
> > > --
> > > 2.49.0
> >
> > Applying: obexd: Unregister profiles when the user is inactive
> > WARNING:MACRO_ARG_UNUSED: Argument 'exit_cb' is not used in function-like macro
> > #409: FILE: obexd/src/logind.h:22:
> > +#define logind_register(init_cb, exit_cb) init_cb()
> >
> > WARNING:MACRO_ARG_UNUSED: Argument 'init_cb' is not used in function-like macro
> > #410: FILE: obexd/src/logind.h:23:
> > +#define logind_unregister(init_cb, exit_cb) exit_cb()
> >
> > WARNING:MACRO_ARG_UNUSED: Argument 'enabled' is not used in function-like macro
> > #411: FILE: obexd/src/logind.h:24:
> > +#define logind_set(enabled) 0
>
> Those #define's are dummy implementations for use when systemd is disabled.
> Which of these would you rather silence the warnings with?
>
>     // inline functions:
>     static inline int logind_set(gboolean enabled) { return 0; }

I think inline is probably better, but I don't have a strong opinion
about the usage of the options below.

>     // comma operator:
>     #define logind_set(enabled) (enabled,0)
>
>     // varargs:
>     #define login_set(...) 0
>
> >
> > --
> > Luiz Augusto von Dentz
> >



-- 
Luiz Augusto von Dentz

[PATCH BlueZ v5 5/6] obexd: Support sd_login_monitor_get_timeout()

[Andrew Sayers]

The documentation for sd_login_monitor_get_timeout() implies the API
may need to be checked after some time, even if no events have been
received via the fd.

In practice, the implementation has always returned a dummy value,
and changing it now would cause enough breakage in other projects
to make it unlikely to ever happen.

Add a handler for that case, even though it can't currently
happen in the real world.

The API assumes we call poll() directly, so in theory it could change
the timeout based on some event that doesn't trigger a callback
(e.g. sending a signal to the service).  It's hard to see how we'd
handle that without running a poll() in a separate thread,
which would be a lot of complexity for an untestable edge case.
Don't try to handle that problem.

Cc: Pauli Virtanen <pav@iki.fi>
Signed-off-by: Andrew Sayers <kernel.org@pileofstuff.org>
---
 obexd/src/logind.c | 43 +++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 43 insertions(+)

diff --git a/obexd/src/logind.c b/obexd/src/logind.c
index ff2bf3219..a0eb62b1e 100644
--- a/obexd/src/logind.c
+++ b/obexd/src/logind.c
@@ -30,6 +30,7 @@ static int uid;
 static gboolean active = FALSE;
 static gboolean monitoring_enabled = TRUE;
 static guint event_source;
+static guint timeout_source;
 
 struct callback_pair {
 	logind_init_cb init_cb;
@@ -82,8 +83,11 @@ static int update(void)
 	return res;
 }
 
+static gboolean timeout_handler(gpointer user_data);
+
 static int check_event(void)
 {
+	uint64_t timeout_usec;
 	int res;
 
 	res = sd_login_monitor_flush(monitor);
@@ -95,6 +99,25 @@ static int check_event(void)
 	if (res < 0)
 		return res;
 
+	res = sd_login_monitor_get_timeout(monitor, &timeout_usec);
+	if (res < 0)
+		return res;
+
+	if (timeout_usec != (uint64_t)-1) {
+		uint64_t time_usec;
+		struct timespec ts;
+		guint interval;
+
+		res = clock_gettime(CLOCK_MONOTONIC, &ts);
+		if (res < 0)
+			return -errno;
+		time_usec = (uint64_t) ts.tv_sec * 1000000 + ts.tv_nsec / 1000;
+		if (time_usec > timeout_usec)
+			return check_event();
+		interval = (timeout_usec - time_usec + 999) / 1000;
+		timeout_source = g_timeout_add(interval, timeout_handler, NULL);
+	}
+
 	return 0;
 }
 
@@ -104,6 +127,11 @@ static gboolean event_handler(GIOChannel *source, GIOCondition condition,
 {
 	int res;
 
+	if (timeout_source) {
+		g_source_remove(timeout_source);
+		timeout_source = 0;
+	}
+
 	res = check_event();
 	if (res) {
 		error("%s: %s", __func__, strerror(-res));
@@ -113,6 +141,17 @@ static gboolean event_handler(GIOChannel *source, GIOCondition condition,
 	return TRUE;
 }
 
+static gboolean timeout_handler(gpointer user_data)
+{
+	int res;
+
+	res = check_event();
+	if (res)
+		error("%s: %s", __func__, strerror(-res));
+
+	return FALSE;
+}
+
 static int logind_init(void)
 {
 	GIOChannel *channel;
@@ -173,6 +212,10 @@ static void logind_exit(void)
 		g_source_remove(event_source);
 		event_source = 0;
 	}
+	if (timeout_source) {
+		g_source_remove(timeout_source);
+		timeout_source = 0;
+	}
 	sd_login_monitor_unref(monitor);
 }
 
-- 
2.49.0

[PATCH BlueZ v5 6/6] Revert "obexd: only run one instance at once"

[Andrew Sayers]

No longer needed now services can share resources.

This reverts commit 8d472b8758dcdd89bf13cf2fb06a8846e1f483a0.

Signed-off-by: Andrew Sayers <kernel.org@pileofstuff.org>
---
 obexd/src/obex.service.in | 9 ---------
 1 file changed, 9 deletions(-)

diff --git a/obexd/src/obex.service.in b/obexd/src/obex.service.in
index f269bc513..cf4d8c985 100644
--- a/obexd/src/obex.service.in
+++ b/obexd/src/obex.service.in
@@ -1,19 +1,10 @@
 [Unit]
 Description=Bluetooth OBEX service
-# This is a user-specific service, but bluetooth is a device-specific protocol.
-# Only run one instance at a time, even if multiple users log in at once:
-ConditionPathExists=!/run/lock/bluez/obexd.lock
-ConditionUser=!@system
 
 [Service]
 Type=dbus
 BusName=org.bluez.obex
 ExecStart=@PKGLIBEXECDIR@/obexd
 
-# If the service fails on the following line, please ensure
-# the bluez tmpfile has been installed in /usr/lib/tmpfiles.d/
-ExecStartPre=touch /run/lock/bluez/obexd.lock
-ExecStopPost=rm -f /run/lock/bluez/obexd.lock
-
 [Install]
 Alias=dbus-org.bluez.obex.service
-- 
2.49.0

Re: [PATCH BlueZ v5 0/6] obexd: unregister profiles when the user is inactive

[patchwork-bot+bluetooth]

Hello:

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

On Tue, 29 Apr 2025 15:14:10 +0100 you wrote:
> This is a follow-up to "obexd: only run one instance at once".
> Instead of refusing to run parallel services, it unregisters
> profiles when the user is inactive.  This avoids the need
> for tmpfiles, and avoids issues where the user with the
> obex service logs out, leaving obex disabled altogether.
> 
> Luiz previously suggested moving this to systemd, but I haven't had much
> luck getting the systemd devs to accept changes, and Pauli's mention of
> elogind (i.e. logind without systemd) suggests it's probably better
> to avoid the dependency anyway.
> 
> [...]

Here is the summary with links:
  - [BlueZ,v5,1/6] pbap: Support calling pbap_init() after pbap_exit()
    https://git.kernel.org/pub/scm/bluetooth/bluez.git/?id=9bf6bb654ff7
  - [BlueZ,v5,2/6] obexd/bluetooth: Support calling bluetooth_init() after bluetooth_exit()
    https://git.kernel.org/pub/scm/bluetooth/bluez.git/?id=29f3c07b8dc9
  - [BlueZ,v5,3/6] obexd: Support creating private system/session bus connections
    https://git.kernel.org/pub/scm/bluetooth/bluez.git/?id=237d818ef294
  - [BlueZ,v5,4/6] obexd: Unregister profiles when the user is inactive
    (no matching commit)
  - [BlueZ,v5,5/6] obexd: Support sd_login_monitor_get_timeout()
    (no matching commit)
  - [BlueZ,v5,6/6] Revert "obexd: only run one instance at once"
    (no matching commit)

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