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

users: Support for watching lastlog2 and wtpmdb on overview page #20472

Merged
merged 1 commit into from
May 22, 2024

Conversation

Lunarequest
Copy link
Contributor

@Lunarequest Lunarequest commented May 16, 2024

This follows up on the lastlog2 pr's to implement support for lastlog2. Lastlog2 slightly differs in behaviour from lastlog. Lastlog2 only prints user's who have already had entries in the lastlog2 database. Not having an entry is already treated as never logged in which matches the current behaviour

pkg/users/users.js Outdated Show resolved Hide resolved
pkg/users/users.js Outdated Show resolved Hide resolved
pkg/users/users.js Outdated Show resolved Hide resolved
pkg/users/users.js Outdated Show resolved Hide resolved
pkg/users/users.js Outdated Show resolved Hide resolved
@mvollmer
Copy link
Member

The subject of the commit should be more specific and talk about lastlog2, "users: Support for watching lastlog2 on overview page".

@Lunarequest Lunarequest force-pushed the lastlog2 branch 2 times, most recently from 3b65169 to d367f22 Compare May 20, 2024 08:00
@mvollmer
Copy link
Member

opensuse tests fail to build their rpm:

+ %tox
DEBUG: /var/tmp/rpm-tmp.UMP3Av: line 35: fg: no job control

The %tox macro probably doesn't exist on opensuse and then the shell tries to interpret "%tox" as a command... welcome to Unix.

@Lunarequest
Copy link
Contributor Author

opensuse tests fail to build their rpm:

+ %tox
DEBUG: /var/tmp/rpm-tmp.UMP3Av: line 35: fg: no job control

The %tox macro probably doesn't exist on opensuse and then the shell tries to interpret "%tox" as a command... welcome to Unix.

yes, we do not have the tox macro. A fix will be submitted upstream soon

@mvollmer
Copy link
Member

yes, we do not have the tox macro. A fix will be submitted upstream soon

Thanks!

Copy link
Member

@mvollmer mvollmer left a comment

Choose a reason for hiding this comment

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

Thanks! I found some more little things... Mostly I think we don't want to use export { ...}.

cockpit.spawn(["test", "-e", "/var/run/utmp"], { err: "ignore" }).then(() => {
setLastlogPath("/var/run/utmp");
getLastlogPath().then((path) => {
setLastlogPath(path);
}).catch(() => {
Copy link
Member

@mvollmer mvollmer May 20, 2024

Choose a reason for hiding this comment

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

I think we don't need to catch errors here since getLastLogPath is not expected to throw an exception. And this could also be async... :-) useEffect(async () => setLastLogPath(await getLastLogPath()), []) wdyt?

pkg/users/utils.js Outdated Show resolved Hide resolved
pkg/users/users.js Show resolved Hide resolved
Copy link
Member

@mvollmer mvollmer left a comment

Choose a reason for hiding this comment

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

Looks good to me, apart from the async useEffect issue.

@Nykseli
Copy link
Contributor

Nykseli commented May 20, 2024

Hi! I'm having issues seeing the inotify event of /var/lib/lastlog/lastlog2.db when the user logs out and the user list doesn't get updated. Using /var/lib/wtmpdb/wtmp.db seems to be more reliable. This also applies to https://github.com/cockpit-project/cockpit/blob/main/pkg/users/account-details.js#L107

Credit goes to @SludgeGirl for figuring this out!

@mvollmer
Copy link
Member

I'm having issues seeing the inotify event of /var/lib/lastlog/lastlog2.db when the user logs out and the user list doesn't get updated.

Yeah, I think there is no way aroung getting our integration tests to run on OpenSUSE. Right now I am happy to merge things that don't break the rest of our platforms, but whether the new code actually works as expected on OpenSUSE... the tests would help a lot to gain confidence in that. So let's prioritize getting them running, I'd say.

@mvollmer mvollmer changed the title users: support users on AccountPage users: Support for watching lastlog2 on overview page May 20, 2024
@Lunarequest Lunarequest force-pushed the lastlog2 branch 2 times, most recently from c68bb69 to dd11f07 Compare May 21, 2024 06:08
pkg/users/users.js Outdated Show resolved Hide resolved
@Lunarequest Lunarequest force-pushed the lastlog2 branch 2 times, most recently from 36c1421 to 059a8d3 Compare May 21, 2024 07:52
@Lunarequest Lunarequest changed the title users: Support for watching lastlog2 on overview page users: Support for watching lastlog2 and wtpmdb on overview page May 21, 2024
export async function getUtmpPath() {
try {
await cockpit.spawn(["test", "-e", "/var/run/utmp"], { err: "ignore" });
return "/var/run/utmp";
Copy link
Member

Choose a reason for hiding this comment

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

Related, with the new systemd rc we now have a race condition with watching utmp so I want to change it to use logind's dbus API and listen to the relevant signals. (This does not have to block this PR but might help)

systemd/systemd#32845

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We would like to get this included in the next release. If the changes for logind can be merged before than i think we can close this pr infavor of the logind pr

Copy link
Member

@mvollmer mvollmer left a comment

Choose a reason for hiding this comment

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

Thanks!

Comment on lines +169 to +170
LastLogPath = "lastlog2";
} catch (err1) {
Copy link
Contributor

Choose a reason for hiding this comment

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

These 2 added lines are not executed by any test.

Comment on lines +20 to +25
} catch (err1) {
try {
await cockpit.spawn(["test", "-e", "/var/lib/wtmpdb/wtmp.db"], { err: "ignore" });
return "/var/lib/wtmpdb/wtmp.db";
} catch (err2) {
return null;
Copy link
Contributor

Choose a reason for hiding this comment

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

These 6 added lines are not executed by any test.

@mvollmer mvollmer merged commit 6bf8d11 into cockpit-project:main May 22, 2024
80 checks passed
@mvollmer
Copy link
Member

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants