Re: [PATCH] Enable DBus activation on non-systemd systems.

Re: [PATCH] Enable DBus activation on non-systemd systems.

[Mark Hindley]

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

Érico,

On Sat, Nov 13, 2021 at 03:03:56AM -0300, Érico Nogueira wrote:
> For things like IWD, I feel DBus activation is definitely the wrong
> path. It should be a system service of some sort, so it can connect to a
> network without first being queried by a user manually, and it shouldn't
> be activated automatically.

Thanks, but I find this a confusing statement.

AFAIU iwd does use DBus activation currently. The Debian changelog contains this
entry:

  * Recommend dbus (system bus)
    Interaction with iwd happens via dbus, i.e. as used by the iwctl
    command (or NetworkManager iwd backend).
    Packages using D-Bus usually doesn't describe this in their package
    relationships and implicitly rely on everyone having a bus available.
    People using iwd however seems to be confused about the lack of a
    startup script and fails to realize that "dbus activation" is the
    intended way to start iwd so hopefully making the dbus relationship more
    explicit will give a hint and help people figure this out.

So, given that DBus activation doesn't require systemd (but can make use of it),
I was trying to understand and suggest a fix for why iwd DBus activation doesn't
work in the non-systemd case.

Mark

Re: [PATCH] Enable DBus activation on non-systemd systems.

[@ 2021-11-13 6:03 UTC (permalink / raw)]

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

On Fri Nov 12, 2021 at 12:56 PM -03, Mark Hindley wrote:
> Marcel,
>
> Thanks
>
> On Fri, Nov 12, 2021 at 03:57:59PM +0100, Marcel Holtmann wrote:
> > > --- a/client/dbus-proxy.c
> > > +++ b/client/dbus-proxy.c
> > > @@ -855,13 +855,12 @@ bool dbus_proxy_init(void)
> > > 	l_dbus_set_disconnect_handler(dbus, dbus_disconnect_callback, NULL,
> > > 									NULL);
> > > 
> > > +	get_managed_objects();
> > > 	if (command_is_interactive_mode())
> > > 		l_dbus_add_service_watch(dbus, IWD_SERVICE,
> > > 						service_appeared_callback,
> > > 						service_disappeared_callback,
> > > 						NULL, NULL);
> > > -	else
> > > -		get_managed_objects();
> > 
> > I don’t understand this change. The whole point is to actually wait for the
> > service to appear.
>
> Yes, but it is a chicken and egg situation: plain DBus activation relies
> on the
> first call on the bus to trigger the activation[1]. The call at the end
> of
> service_appeared_callback() is never going to be reached as interactive
> iwct is
> just waiting having called service_disappeared_callback().
>
> > 
> > > 
> > > 	return true;
> > > }
> > > diff --git a/src/main.c b/src/main.c
> > > index 989665e4..8e3a5ca7 100644
> > > --- a/src/main.c
> > > +++ b/src/main.c
> > > @@ -191,9 +191,6 @@ static void request_name_callback(struct l_dbus *dbus, bool success,
> > > 		goto fail_exit;
> > > 	}
> > > 
> > > -	if (!l_dbus_object_manager_enable(dbus, "/"))
> > > -		l_warn("Unable to register the ObjectManager");
> > > -
> > > 	if (!l_dbus_object_add_interface(dbus, IWD_BASE_PATH,
> > > 						IWD_DAEMON_INTERFACE,
> > > 						NULL) ||
> > > @@ -240,6 +237,9 @@ static void dbus_ready(void *user_data)
> > > {
> > > 	struct l_dbus *dbus = user_data;
> > > 
> > > +	if (!l_dbus_object_manager_enable(dbus, "/"))
> > > +		l_warn("Unable to register the ObjectManager");
> > > +
> > 
> > This change is unclear to me. Why would I register the ObjectManager if I
> > can’t acquire the name. I don’t get how this is suppose to work. 
>
> Without this change, once the name is acquired, dbus still fails to find
> the
> ObjectManager path as it hasn't (yet) been enabled. So the first call to
> get_managed_objects() fails if it was also the one that caused the
> service
> activation.
>
> > This is the right thing to do.
>
> I wondered about the order here too. I know that they are different
> bindings and
> that it is deprecated, so it may well not be the most convincing
> argument, but
> GDBus docs specify this requirement[2] and use it in their examples[3]
> where
> object paths are registered on bus acquisition rather than name
> acquisition.
>
> > 
> > > 	l_dbus_name_acquire(dbus, "net.connman.iwd", false, false, false,
> > > 				request_name_callback, NULL);
> > > 
> > > diff --git a/src/net.connman.iwd.service b/src/net.connman.iwd.service.in
> > > similarity index 77%
> > > rename from src/net.connman.iwd.service
> > > rename to src/net.connman.iwd.service.in
> > > index d8ece4c3..a7cb7edd 100644
> > > --- a/src/net.connman.iwd.service
> > > +++ b/src/net.connman.iwd.service.in
> > > @@ -1,5 +1,5 @@
> > > [D-BUS Service]
> > > Name=net.connman.iwd
> > > -Exec=/bin/false
> > > +Exec=@libexecdir@/iwd
> > > User=root
> > > SystemdService=iwd.service
> > 
> > This change is not for upstream git tree. Use --disable-systemd-service and
> > then provide your own files in the distro packaging.
>
> But this isn't about distro specific packaging, I think it is a setup
> that works
> both with systemd or without it. Debian has both systemd and several
> other inits
> available so building with --disabled-systemd-service is not an option.

For things like IWD, I feel DBus activation is definitely the wrong
path. It should be a system service of some sort, so it can connect to a
network without first being queried by a user manually, and it shouldn't
be activated automatically. For example, in case a user accidentally
launches iwctl when wpa_supplicant was in use instead, for whatever
reason.

You can find the service file we use in Void Linux for IWD in [1]; it
should be compatible with any daemontools-like service manager.

[1] https://github.com/void-linux/void-packages/blob/7ba71aeed459e57f2c12cd6e4d1779297ee17550/srcpkgs/iwd/files/iwd/run

>
> Thanks
>
> Mark
>
> [1]
> https://dbus.freedesktop.org/doc/dbus-specification.html#message-bus-starting-services
>
> [2]
> https://www.freedesktop.org/software/gstreamer-sdk/data/docs/latest/gio/ch30s03.html
>
> [3]
> https://gitlab.gnome.org/GNOME/glib/-/blob/HEAD/gio/tests/gdbus-example-server.c
> _______________________________________________
> iwd mailing list -- iwd(a)lists.01.org
> To unsubscribe send an email to iwd-leave(a)lists.01.org

Re: [PATCH] Enable DBus activation on non-systemd systems.

[Mark Hindley]

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

Marcel,

Thanks

On Fri, Nov 12, 2021 at 03:57:59PM +0100, Marcel Holtmann wrote:
> > --- a/client/dbus-proxy.c
> > +++ b/client/dbus-proxy.c
> > @@ -855,13 +855,12 @@ bool dbus_proxy_init(void)
> > 	l_dbus_set_disconnect_handler(dbus, dbus_disconnect_callback, NULL,
> > 									NULL);
> > 
> > +	get_managed_objects();
> > 	if (command_is_interactive_mode())
> > 		l_dbus_add_service_watch(dbus, IWD_SERVICE,
> > 						service_appeared_callback,
> > 						service_disappeared_callback,
> > 						NULL, NULL);
> > -	else
> > -		get_managed_objects();
> 
> I don’t understand this change. The whole point is to actually wait for the
> service to appear.

Yes, but it is a chicken and egg situation: plain DBus activation relies on the
first call on the bus to trigger the activation[1].  The call at the end of
service_appeared_callback() is never going to be reached as interactive iwct is
just waiting having called service_disappeared_callback().

> 
> > 
> > 	return true;
> > }
> > diff --git a/src/main.c b/src/main.c
> > index 989665e4..8e3a5ca7 100644
> > --- a/src/main.c
> > +++ b/src/main.c
> > @@ -191,9 +191,6 @@ static void request_name_callback(struct l_dbus *dbus, bool success,
> > 		goto fail_exit;
> > 	}
> > 
> > -	if (!l_dbus_object_manager_enable(dbus, "/"))
> > -		l_warn("Unable to register the ObjectManager");
> > -
> > 	if (!l_dbus_object_add_interface(dbus, IWD_BASE_PATH,
> > 						IWD_DAEMON_INTERFACE,
> > 						NULL) ||
> > @@ -240,6 +237,9 @@ static void dbus_ready(void *user_data)
> > {
> > 	struct l_dbus *dbus = user_data;
> > 
> > +	if (!l_dbus_object_manager_enable(dbus, "/"))
> > +		l_warn("Unable to register the ObjectManager");
> > +
> 
> This change is unclear to me. Why would I register the ObjectManager if I
> can’t acquire the name. I don’t get how this is suppose to work. 

Without this change, once the name is acquired, dbus still fails to find the
ObjectManager path as it hasn't (yet) been enabled. So the first call to
get_managed_objects() fails if it was also the one that caused the service
activation.

> This is the right thing to do.

I wondered about the order here too. I know that they are different bindings and
that it is deprecated, so it may well not be the most convincing argument, but
GDBus docs specify this requirement[2] and use it in their examples[3] where
object paths are registered on bus acquisition rather than name acquisition.

> 
> > 	l_dbus_name_acquire(dbus, "net.connman.iwd", false, false, false,
> > 				request_name_callback, NULL);
> > 
> > diff --git a/src/net.connman.iwd.service b/src/net.connman.iwd.service.in
> > similarity index 77%
> > rename from src/net.connman.iwd.service
> > rename to src/net.connman.iwd.service.in
> > index d8ece4c3..a7cb7edd 100644
> > --- a/src/net.connman.iwd.service
> > +++ b/src/net.connman.iwd.service.in
> > @@ -1,5 +1,5 @@
> > [D-BUS Service]
> > Name=net.connman.iwd
> > -Exec=/bin/false
> > +Exec=@libexecdir@/iwd
> > User=root
> > SystemdService=iwd.service
> 
> This change is not for upstream git tree. Use --disable-systemd-service and
> then provide your own files in the distro packaging.

But this isn't about distro specific packaging, I think it is a setup that works
both with systemd or without it. Debian has both systemd and several other inits
available so building with --disabled-systemd-service is not an option.

Thanks

Mark

[1]  https://dbus.freedesktop.org/doc/dbus-specification.html#message-bus-starting-services

[2]  https://www.freedesktop.org/software/gstreamer-sdk/data/docs/latest/gio/ch30s03.html

[3]  https://gitlab.gnome.org/GNOME/glib/-/blob/HEAD/gio/tests/gdbus-example-server.c

Re: [PATCH] Enable DBus activation on non-systemd systems.

[Marcel Holtmann]

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

Hi Mark,

> I have been investigating why legacy DBus activation does not work for iwd on
> non-systemd systems. In Debian and Devuan (at least) this has required
> alternative means of starting the daemon by initscript be implemented.
> 
> However, DBus activation without systemd is possible, without any
> impact on systems running systemd.
> 
> For this to be supported there are 3 aspects that need to be addressed:
> 
> - net.connman.iwd.service: set Exec to iwd path.
> 
> - client/dbus-proxy.c: always call get_managed_objects() on init.
> 
> - src/main/c: register Object Manager before acquiring name otherwise the call
>   to get_managed_objects() which triggers activation fails with 'No matching
>   method found'.
> 
> Signed-off-by: Mark Hindley <mark(a)hindley.org.uk>
> ---
> client/dbus-proxy.c                                         | 3 +--
> src/main.c                                                  | 6 +++---
> src/{net.connman.iwd.service => net.connman.iwd.service.in} | 2 +-
> 3 files changed, 5 insertions(+), 6 deletions(-)
> rename src/{net.connman.iwd.service => net.connman.iwd.service.in} (77%)
> 
> diff --git a/client/dbus-proxy.c b/client/dbus-proxy.c
> index ab9fb5a5..15bb4128 100644
> --- a/client/dbus-proxy.c
> +++ b/client/dbus-proxy.c
> @@ -855,13 +855,12 @@ bool dbus_proxy_init(void)
> 	l_dbus_set_disconnect_handler(dbus, dbus_disconnect_callback, NULL,
> 									NULL);
> 
> +	get_managed_objects();
> 	if (command_is_interactive_mode())
> 		l_dbus_add_service_watch(dbus, IWD_SERVICE,
> 						service_appeared_callback,
> 						service_disappeared_callback,
> 						NULL, NULL);
> -	else
> -		get_managed_objects();

I don’t understand this change. The whole point is to actually wait for the service to appear.

> 
> 	return true;
> }
> diff --git a/src/main.c b/src/main.c
> index 989665e4..8e3a5ca7 100644
> --- a/src/main.c
> +++ b/src/main.c
> @@ -191,9 +191,6 @@ static void request_name_callback(struct l_dbus *dbus, bool success,
> 		goto fail_exit;
> 	}
> 
> -	if (!l_dbus_object_manager_enable(dbus, "/"))
> -		l_warn("Unable to register the ObjectManager");
> -
> 	if (!l_dbus_object_add_interface(dbus, IWD_BASE_PATH,
> 						IWD_DAEMON_INTERFACE,
> 						NULL) ||
> @@ -240,6 +237,9 @@ static void dbus_ready(void *user_data)
> {
> 	struct l_dbus *dbus = user_data;
> 
> +	if (!l_dbus_object_manager_enable(dbus, "/"))
> +		l_warn("Unable to register the ObjectManager");
> +

This change is unclear to me. Why would I register the ObjectManager if I can’t acquire the name. I don’t get how this is suppose to work. This is actually the right thing to do.

> 	l_dbus_name_acquire(dbus, "net.connman.iwd", false, false, false,
> 				request_name_callback, NULL);
> 
> diff --git a/src/net.connman.iwd.service b/src/net.connman.iwd.service.in
> similarity index 77%
> rename from src/net.connman.iwd.service
> rename to src/net.connman.iwd.service.in
> index d8ece4c3..a7cb7edd 100644
> --- a/src/net.connman.iwd.service
> +++ b/src/net.connman.iwd.service.in
> @@ -1,5 +1,5 @@
> [D-BUS Service]
> Name=net.connman.iwd
> -Exec=/bin/false
> +Exec=@libexecdir@/iwd
> User=root
> SystemdService=iwd.service

This change is not for upstream git tree. Use --disable-systemd-service and then provide your own files in the distro packaging. Long time ago we made the decision to only provide systemd integration. All other things like init scripts or custom distro specific files are left to the distro packages.

Regards

Marcel

[PATCH] Enable DBus activation on non-systemd systems.

[Mark Hindley]

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

I have been investigating why legacy DBus activation does not work for iwd on
non-systemd systems. In Debian and Devuan (at least) this has required
alternative means of starting the daemon by initscript be implemented.

However, DBus activation without systemd is possible, without any
impact on systems running systemd.

For this to be supported there are 3 aspects that need to be addressed:

 - net.connman.iwd.service: set Exec to iwd path.

 - client/dbus-proxy.c: always call get_managed_objects() on init.

 - src/main/c: register Object Manager before acquiring name otherwise the call
   to get_managed_objects() which triggers activation fails with 'No matching
   method found'.

Signed-off-by: Mark Hindley <mark(a)hindley.org.uk>
---
 client/dbus-proxy.c                                         | 3 +--
 src/main.c                                                  | 6 +++---
 src/{net.connman.iwd.service => net.connman.iwd.service.in} | 2 +-
 3 files changed, 5 insertions(+), 6 deletions(-)
 rename src/{net.connman.iwd.service => net.connman.iwd.service.in} (77%)

diff --git a/client/dbus-proxy.c b/client/dbus-proxy.c
index ab9fb5a5..15bb4128 100644
--- a/client/dbus-proxy.c
+++ b/client/dbus-proxy.c
@@ -855,13 +855,12 @@ bool dbus_proxy_init(void)
 	l_dbus_set_disconnect_handler(dbus, dbus_disconnect_callback, NULL,
 									NULL);
 
+	get_managed_objects();
 	if (command_is_interactive_mode())
 		l_dbus_add_service_watch(dbus, IWD_SERVICE,
 						service_appeared_callback,
 						service_disappeared_callback,
 						NULL, NULL);
-	else
-		get_managed_objects();
 
 	return true;
 }
diff --git a/src/main.c b/src/main.c
index 989665e4..8e3a5ca7 100644
--- a/src/main.c
+++ b/src/main.c
@@ -191,9 +191,6 @@ static void request_name_callback(struct l_dbus *dbus, bool success,
 		goto fail_exit;
 	}
 
-	if (!l_dbus_object_manager_enable(dbus, "/"))
-		l_warn("Unable to register the ObjectManager");
-
 	if (!l_dbus_object_add_interface(dbus, IWD_BASE_PATH,
 						IWD_DAEMON_INTERFACE,
 						NULL) ||
@@ -240,6 +237,9 @@ static void dbus_ready(void *user_data)
 {
 	struct l_dbus *dbus = user_data;
 
+	if (!l_dbus_object_manager_enable(dbus, "/"))
+		l_warn("Unable to register the ObjectManager");
+
 	l_dbus_name_acquire(dbus, "net.connman.iwd", false, false, false,
 				request_name_callback, NULL);
 
diff --git a/src/net.connman.iwd.service b/src/net.connman.iwd.service.in
similarity index 77%
rename from src/net.connman.iwd.service
rename to src/net.connman.iwd.service.in
index d8ece4c3..a7cb7edd 100644
--- a/src/net.connman.iwd.service
+++ b/src/net.connman.iwd.service.in
@@ -1,5 +1,5 @@
 [D-BUS Service]
 Name=net.connman.iwd
-Exec=/bin/false
+Exec=@libexecdir@/iwd
 User=root
 SystemdService=iwd.service
-- 
2.20.1