search

bus1 / dbus-broker

Linux D-Bus Message Broker
https://github.com/bus1/dbus-broker/wiki
Apache License 2.0
667 stars 78 forks source link

launcher: propagate readable error message on service activation failure #277

Closed bluca closed 2 years ago

bluca commented 2 years ago

Example:

1) ConditionPathExists=/tmp/marker fails (note: with change in systemd to return skipped) 2) AssertPathExists=/tmp/marker fails 3) service immediately exits with failure before taking ownership of bus

root@image:~# busctl call com.ClientServerExample /com/ClientServerExample com.ClientServerExample Hello
Call failed: Could not activate remote peer: job result: skipped.
root@image:~# nano /etc/systemd/system/hello.service 
root@image:~# systemctl daemon-reload
root@image:~# busctl call com.ClientServerExample /com/ClientServerExample com.ClientServerExample Hello
Call failed: Could not activate remote peer: job result: assert.
root@image:~# nano /etc/systemd/system/hello.service 
root@image:~# systemctl daemon-reload
root@image:~# busctl call com.ClientServerExample /com/ClientServerExample com.ClientServerExample Hello
Call failed: Could not activate remote peer: job result: failed.

Does this sound sensible?

Fixes https://github.com/bus1/dbus-broker/issues/269

teg commented 2 years ago

However, how do we confer that information back to the caller? We have to use the pre-defined org.freedesktop.DBus.Error.* code, and we are only left with its own free-form field for additional information. Should be put our reverse-domain code there unmodified? This would allow callers to react to it. But it would misuse the field. Alternatively, we convert it into a free-form string as last step. Unsure what to do exactly. Comments?

Hmm, I think I would vote for the second option. Not as useful, but the alternative is quite hacky.

bluca commented 2 years ago

Ok, updated - I defined the errors as suggested, added enums and converters. PTAL!

bluca commented 2 years ago

@dvdhrm ping

bluca commented 2 years ago

another ping @teg @dvdhrm

dvdhrm commented 2 years ago

I am on 8month parental leave until March. I will defer any non-bugfixes until then. If you consider this PR critical, let me know and I will try to find some time for it.

bluca commented 2 years ago

It's not super urgent, and if you are on parental leave you definitely have more important things going on :-)

bluca commented 2 years ago

I am on 8month parental leave until March. I will defer any non-bugfixes until then. If you consider this PR critical, let me know and I will try to find some time for it.

@dvdhrm ping, in case you are back to work

bluca commented 2 years ago

@dvdhrm one more monthly ping. Anything else I should change here? Thanks!

bluca commented 2 years ago

Rebased to fix merge conflict

dvdhrm commented 2 years ago

I merged this manually. I did a follow-up commit which changes bus1_error_* to controller_name_error_*, to make sure we keep the namespacing. Additionally, I changed the signed type to unsigned, making value 0 be invalid. This is mostly to stay in line with our coding-style.

Thanks a lot for the patches! Very much appreciated!

bluca commented 2 years ago

Thanks - FYI if you had left that as feedback, I'd have been happy to update the PR myself

dvdhrm commented 2 years ago

Thanks - FYI if you had left that as feedback, I'd have been happy to update the PR myself

I know, I know. But you already patiently waited for so long, so I didn't want to nag too much on coding style.