Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

core/service: store BUSERROR= & VARLINKERROR= received and show them through systemctl status #33430

Merged
merged 5 commits into from
Jun 20, 2024

Conversation

YHNdnzj
Copy link
Member

@YHNdnzj YHNdnzj commented Jun 20, 2024

Fixes: #6073

@YHNdnzj YHNdnzj added the pid1 label Jun 20, 2024
@github-actions github-actions bot added documentation sd-bus systemctl please-review PR is ready for (re-)review by a maintainer labels Jun 20, 2024
@YHNdnzj YHNdnzj force-pushed the buserror-notify branch 2 times, most recently from 785484c to eea6866 Compare June 20, 2024 14:11
@poettering
Copy link
Member

love it. thanks!

@poettering poettering added good-to-merge/waiting-for-ci 👍 PR is good to merge, but CI hasn't passed at time of review. Please merge if you see CI has passed and removed please-review PR is ready for (re-)review by a maintainer labels Jun 20, 2024
@poettering
Copy link
Member

I guess you can add "Fixes: #6073" somewhere

poettering added a commit to poettering/systemd that referenced this pull request Jun 20, 2024
varlinkctl has this nice feature that it sends the varlink error it gets
via sd_notify() to the caller. With systemd#33430 ths information is collected
and exposed in "systemctl status".

Let's make sure we can provide the same in busctl: also propagate errors
the same way.

With this and systemd#33430 we can comprehensively close systemd#6073
Copy link
Member

@yuwata yuwata left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Is it possible to add simple test cases?

@yuwata yuwata added good-to-merge/with-minor-suggestions and removed good-to-merge/waiting-for-ci 👍 PR is good to merge, but CI hasn't passed at time of review. Please merge if you see CI has passed labels Jun 20, 2024
poettering added a commit to poettering/systemd that referenced this pull request Jun 20, 2024
varlinkctl has this nice feature that it sends the varlink error it gets
via sd_notify() to the caller. With systemd#33430 ths information is collected
and exposed in "systemctl status".

Let's make sure we can provide the same in busctl: also propagate errors
the same way.

With this and systemd#33430 we can comprehensively close systemd#6073
@poettering
Copy link
Member

for writing a test case, consider just cherry-picking #33434 into this PR (and closing that PR then), and then fire off a systemd-run -p Notify=main busctl call … command line that calls something that fails (could be as easy as a service or method that definitely doesn exist), and see if systemctl status later reports it.

YHNdnzj and others added 4 commits June 20, 2024 19:03
varlinkctl has this nice feature that it sends the varlink error it gets
via sd_notify() to the caller. With previous commits this information
is collected and exposed in "systemctl status".

Let's make sure we can provide the same in busctl: also propagate errors
the same way.

With this we can comprehensively close systemd#6073
@YHNdnzj YHNdnzj added good-to-merge/waiting-for-ci 👍 PR is good to merge, but CI hasn't passed at time of review. Please merge if you see CI has passed and removed good-to-merge/with-minor-suggestions labels Jun 20, 2024
@YHNdnzj YHNdnzj force-pushed the buserror-notify branch 2 times, most recently from cba33f6 to 9ab9bff Compare June 20, 2024 17:56
@@ -2745,6 +2745,8 @@ node /org/freedesktop/systemd1/unit/avahi_2ddaemon_2eservice {
readonly s FileDescriptorStorePreserve = '...';
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

btw, downstreams asked us to always make changes to TODO in separate commits because they often backport patches and the TODO stuff is a major source of conflicts because it changes so often, but not what they care about for backports.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, TODO is mostly about new features, so I don't quite understand why they would backport those commits in the first place... I prefer not to split such changes out if this is only about backporting.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

well, people backport stuff for many reasons. i personally would be a lot more conservative, but then again i am not a package maintainer.

@poettering
Copy link
Member

lgtm.

@poettering poettering merged commit d42edbf into systemd:main Jun 20, 2024
43 of 44 checks passed
@github-actions github-actions bot removed the good-to-merge/waiting-for-ci 👍 PR is good to merge, but CI hasn't passed at time of review. Please merge if you see CI has passed label Jun 20, 2024
@YHNdnzj YHNdnzj deleted the buserror-notify branch June 20, 2024 21:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

A "BUSERROR" notify message is documented but not implemented
3 participants