Logind shutdown refactor #21838

lnussel · 2021-12-20T14:45:57Z

Did the refactor last to show where this is coming from. The previous commits could be picked separately. Esp the shutdown -c fix makes sense right away IMO.

lnussel · 2021-12-30T11:39:55Z

Added test case. It uses wraps nspawn and uses pexpect to log in on the console. Then spawn screen to have two ptys where the script can check the wall message.

marcosfrm · 2021-12-31T17:01:40Z

#213
#1227

lnussel · 2022-01-03T10:06:34Z

Ic. So I shall remove the commit here and potentially have separate discussion about it I guess

See: systemd/systemd#21838

See: systemd/systemd#21838 Follow-up to ec8068b.

mrc0mmand · 2022-01-03T22:03:07Z

I installed the necessary dependencies for TEST-69 in CentOS CI, but the test still keeps failing on CentOS Stream 8. Also, the deps need to be installed in Ubuntu CI as well (/cc @bluca) - python-pexpect and screen.

bluca · 2022-01-03T22:57:35Z

added and re-triggered the focal-amd64 run

keszybz

Thanks for working on this. Looks like a refactoring was overdue.

src/login/logind-utmp.c

src/login/logind-action.c

src/login/logind-dbus.c

src/login/logind-action.c

lnussel · 2022-01-17T16:13:05Z

updated based on previous comments

yuwata

Nice. Several coding style nits.

src/login/logind-dbus.c

src/systemctl/systemctl-compat-halt.c

src/login/logind-dbus.c

src/login/logind-utmp.c

test/test-shutdown.py

An elapse time of zero means NOW which should trigger a wall message.

The code at this point is not able to tell whether it was called as halt/poweroff/reboot or shutdown with time "now". The code also takes a shortcut to skip logind if called as root. That however means asking shutdown for immediate action won't trigger a wall message. As per systemd#8424 (comment) all commands should trigger a wall message. That simplifies the code as we can try logind first always.

Something calling directly into the dbus interface to request a shutdown may not bother turning wall messages on explicitly. This has the convenient side effect that no separate polkit auth is required to turn on wall messages. Was annoying as having a wall message is the default behavior of the commandline tools. Now it's the other way around ie eg systemctl reboot --no-wall requires auth to explicitly turn off the wall message.

The wall mechanism uses the scheduled_shutdown_type to determine what message to send so it needs to be filled in also for the cases that call for shutdown without schedule. It's really a hackish way. The overall code needs refacturing.

yuwata · 2022-01-25T15:45:32Z

Thanks. Most comments are addressed. Please do the rest of them.

lnussel · 2022-01-25T16:13:05Z

Working on it. GH fooled me by hiding he big diff so didn't see all :)

Avoid hardcoded strings and string compares related to shutdown actions. Instead put everything into a common structure. Reuse existing HandleAction as index since it's already exposed as property for the button handlers.

For shutdowns don't fall back to starting the target directly if talking to logind failed with auth failure. That would just lead to another polkit auth attempt.

Wraps nspawn to be able to use pexpect. The test logs in on the console and runs screen. In one screen window it types in shutdown commands and checks whether a wall message was sent to the other.

yuwata

Thank you! LGTM.

dnicolodi · 2022-01-25T18:26:20Z

src/login/logind-action.c

+HandleAction manager_handle_for_item(const ActionTableItem* a) {
+        if (a && a < action_table + ELEMENTSOF(action_table))
+                return a - action_table;
+        return _HANDLE_ACTION_INVALID;


This looks very hackish to me. I would simply include the enum value in a member of the ActionTableItem structure. At least I would add assert(a >= action_table && a < action_table + ELEMENTSOF(action_table)) here as calling this function with anything not satisfying these constraints is a misuse of the function.

Yes, there are tons of improvements that can be applied still and I didn't even get to the thing I actually wanted to do :-) As I wrote elsewhere the rabbit hole is deep. When is a pr considered enough of an improvement to merge it though? If I change this, will someone else chime in and demand yet another change?

OK. Let's merge this as is. If necessary, let's apply followups.

I would agree with you if this would be existing code. However, this is code you are introducing with this PR, thus I would much prefer if it would not add a questionable code pattern that would need yet another PR to clean up. However, I don't have a say in whether this PR should be accepted as it is or not.

At least, the function checks NULL, so it satisfies minimum criteria.

We usually introduce functions like the following way.

void function(Object *obj) { assert(obj); ...; }

Here, it avoids only case that obj == NULL. But it can easily triggers segfault or so by e.g.

Object obj; function((Object*) ((uint8_t*) &obj + 1));

The situation @dnicolodi concerns sounds a kind of such case for me.

Note, I do not say @dnicolodi's suggestion is bad. I think it is not a strong requirement for this PR being merged.
If you'd like, please submit a followup PR.

will do. Thank you!

poettering

Looks OK in general, just some minor things.

(Sorry for the post-merge review, been on parental leave the past months)

src/login/logind-dbus.c

src/login/logind-action.c

src/login/logind-action.h

src/login/logind-dbus.c

poettering · 2022-02-03T17:16:02Z

(I understand that some of the points I raised are already fixed in the meantime by later commits, ignor those then)

lnussel · 2022-02-07T10:08:09Z

Sure, no worries. Congrats, hope you had a good time :)

lnussel changed the title ~~Logind refactor~~ Logind shutdown refactor Dec 20, 2021

github-actions bot added the systemctl label Dec 20, 2021

lnussel force-pushed the logind-refactor branch 2 times, most recently from 6067b66 to ae2eb6b Compare December 20, 2021 18:31

lnussel force-pushed the logind-refactor branch from ae2eb6b to 2e876fc Compare December 30, 2021 11:38

lnussel force-pushed the logind-refactor branch 2 times, most recently from 40194b8 to 7c972f9 Compare December 30, 2021 13:51

lnussel force-pushed the logind-refactor branch from 7c972f9 to 8a05ecc Compare January 3, 2022 14:00

lnussel marked this pull request as ready for review January 3, 2022 14:47

mrc0mmand added a commit to mrc0mmand/systemd-centos-ci that referenced this pull request Jan 3, 2022

tree-wide: install screen required by TEST-69-SHUTDOWN

9f34d56

See: systemd/systemd#21838

mrc0mmand mentioned this pull request Jan 3, 2022

tree-wide: install screen required by TEST-69-SHUTDOWN systemd/systemd-centos-ci#452

Merged

mrc0mmand added a commit to systemd/systemd-centos-ci that referenced this pull request Jan 3, 2022

tree-wide: install screen required by TEST-69-SHUTDOWN

ec8068b

See: systemd/systemd#21838

mrc0mmand added a commit to mrc0mmand/systemd-centos-ci that referenced this pull request Jan 3, 2022

vagrant(arch): install python-pexpect required by TEST-69-SHUTDOWN

2630dbb

See: systemd/systemd#21838 Follow-up to ec8068b.

mrc0mmand mentioned this pull request Jan 3, 2022

vagrant(arch): install python-pexpect required by TEST-69-SHUTDOWN systemd/systemd-centos-ci#453

Merged

mrc0mmand added a commit to systemd/systemd-centos-ci that referenced this pull request Jan 3, 2022

vagrant(arch): install python-pexpect required by TEST-69-SHUTDOWN

507f987

See: systemd/systemd#21838 Follow-up to ec8068b.

keszybz reviewed Jan 4, 2022

View reviewed changes

src/login/logind-utmp.c Outdated Show resolved Hide resolved

src/login/logind-utmp.c Outdated Show resolved Hide resolved

src/login/logind-action.c Show resolved Hide resolved

src/login/logind-dbus.c Show resolved Hide resolved

src/login/logind-action.c Show resolved Hide resolved

keszybz reviewed Jan 4, 2022

View reviewed changes

src/login/logind-action.c Outdated Show resolved Hide resolved

lnussel force-pushed the logind-refactor branch from 8a05ecc to 4a9f0d4 Compare January 17, 2022 16:05

lnussel mentioned this pull request Jan 18, 2022

System V Compatbility Shutdown/Reboot Commands Should Honor Inhibitors and Send PrepareForShutdown DBus Signal When Run As Root #22058

Closed

bluca added login please-review labels Jan 19, 2022

yuwata requested changes Jan 23, 2022

View reviewed changes

yuwata added reviewed/needs-rework 🔨 PR has been reviewed and needs another round of reworks and removed please-review labels Jan 23, 2022

lnussel added 5 commits January 25, 2022 14:46

logind: fix wall message for immediate shutdowns

90b1ec0

An elapse time of zero means NOW which should trigger a wall message.

logind: require polkit auth for cancelling shutdowns

ec14fba

lnussel force-pushed the logind-refactor branch from 4a9f0d4 to bce6a95 Compare January 25, 2022 15:33

lnussel added 3 commits January 25, 2022 17:43

logind: refactor

5ed7347

Avoid hardcoded strings and string compares related to shutdown actions. Instead put everything into a common structure. Reuse existing HandleAction as index since it's already exposed as property for the button handlers.

systemctl: shutdown don't fallback on auth fail

38d55bf

For shutdowns don't fall back to starting the target directly if talking to logind failed with auth failure. That would just lead to another polkit auth attempt.

test: add shutdown test

48f3bc5

Wraps nspawn to be able to use pexpect. The test logs in on the console and runs screen. In one screen window it types in shutdown commands and checks whether a wall message was sent to the other.

lnussel force-pushed the logind-refactor branch from bce6a95 to 48f3bc5 Compare January 25, 2022 16:49

yuwata removed the reviewed/needs-rework 🔨 PR has been reviewed and needs another round of reworks label Jan 25, 2022

yuwata approved these changes Jan 25, 2022

View reviewed changes

yuwata added 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 Jan 25, 2022

yuwata added this to the v251 milestone Jan 25, 2022

dnicolodi reviewed Jan 25, 2022

View reviewed changes

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 good-to-merge/with-minor-suggestions labels Jan 26, 2022

yuwata merged commit 256ce2e into systemd:main Jan 31, 2022
7 of 8 checks passed

poettering reviewed Feb 3, 2022

View reviewed changes

lnussel mentioned this pull request Feb 8, 2022

logind: post review fixes #22445

Merged

Logind shutdown refactor #21838

Logind shutdown refactor #21838

lnussel commented Dec 20, 2021 •

edited

lnussel commented Dec 30, 2021

marcosfrm commented Dec 31, 2021

lnussel commented Jan 3, 2022

mrc0mmand commented Jan 3, 2022

bluca commented Jan 3, 2022

keszybz left a comment

lnussel commented Jan 17, 2022

yuwata left a comment

yuwata commented Jan 25, 2022

lnussel commented Jan 25, 2022

yuwata left a comment

dnicolodi Jan 25, 2022

lnussel Jan 31, 2022

yuwata Jan 31, 2022

dnicolodi Jan 31, 2022

yuwata Jan 31, 2022

yuwata Jan 31, 2022

lnussel Jan 31, 2022

poettering left a comment

poettering commented Feb 3, 2022

lnussel commented Feb 7, 2022

Logind shutdown refactor #21838

Logind shutdown refactor #21838

Conversation

lnussel commented Dec 20, 2021 • edited

lnussel commented Dec 30, 2021

marcosfrm commented Dec 31, 2021

lnussel commented Jan 3, 2022

mrc0mmand commented Jan 3, 2022

bluca commented Jan 3, 2022

keszybz left a comment

Choose a reason for hiding this comment

lnussel commented Jan 17, 2022

yuwata left a comment

Choose a reason for hiding this comment

yuwata commented Jan 25, 2022

lnussel commented Jan 25, 2022

yuwata left a comment

Choose a reason for hiding this comment

dnicolodi Jan 25, 2022

Choose a reason for hiding this comment

lnussel Jan 31, 2022

Choose a reason for hiding this comment

yuwata Jan 31, 2022

Choose a reason for hiding this comment

dnicolodi Jan 31, 2022

Choose a reason for hiding this comment

yuwata Jan 31, 2022

Choose a reason for hiding this comment

yuwata Jan 31, 2022

Choose a reason for hiding this comment

lnussel Jan 31, 2022

Choose a reason for hiding this comment

poettering left a comment

Choose a reason for hiding this comment

poettering commented Feb 3, 2022

lnussel commented Feb 7, 2022

lnussel commented Dec 20, 2021 •

edited