users: support OpenBSD using utmp #6406

jadijadi · 2024-05-16T18:57:16Z

OpenBSD uses the original utmp file format so the general users does not work there. By using the utmp-classic lib, we cna process the current users on the system from the /var/run/utmp and show the list.

fixes #5665

jadijadi · 2024-05-16T19:01:33Z

A few points:

Not sure if we have to commit the Cargo.lock
Based on my observation the native users on OpenBSD shows each name only once. (so jadi root regardless of different logins by jadi) This is different than the behaviour on Linux machines (that is jadi jadi jadi root root if 5 logins are there). Not sure which path we should go. I kept the Linux style for consistency in our tool but I'm open to keeping the OpenBSD version like the native one.
if this will be approved, I can kind of quickly work other similar tools (say who) using the same utmp-classic parser.

cakebaker · 2024-05-17T07:51:30Z

Can you please add zerocopy to the skip list in deny.toml or, as you are the author of utmp-classic, upgrade its zerocopy dependency?

sylvestre · 2024-05-17T08:19:30Z

tests/by-util/test_users.rs

+    new_ucmd!()
+        .args(&["basic_openbsd_utmp"])
+        .run()
+        .stdout_contains("jadi");


what means jadi ? :)

its my user name. no specific meaning. If not appropriate to use it, I can add a new user to my OpenBSD (say test_user), login and use that utmp file instead.

yes, please :)

sylvestre · 2024-05-17T08:21:18Z

src/uu/users/src/platform/openbsd.rs

 pub fn uumain(args: impl uucore::Args) -> UResult<()> {
-    let _matches = uu_app().try_get_matches_from(args)?;
+    let matches = uu_app()


a bunch of code is duplicated from the unix.rs
i would prefer that we have a single file

Agree. they are different only in small parts. Will try to use the same file and merge this into that using a os_name check when reading data.

done... tried to do it clean but please do not hesitate to comment & improve.

sylvestre · 2024-05-17T08:22:08Z

src/uu/users/Cargo.toml

@@ -16,6 +16,7 @@ path = "src/users.rs"

 [dependencies]
 clap = { workspace = true }
+utmp-classic = {version = "0.1.5"}


please define it in /Cargo.toml and use workspace = true
and make it openbsd only

jadijadi · 2024-05-18T08:27:41Z

Can you please add zerocopy to the skip list in deny.toml or, as you are the author of utmp-classic, upgrade its zerocopy dependency?

will work on it. The last time I tested the utmp-classic did not worked using the latest zeroconf. Obviously its cleaner to fix the code to work with the latest version. will work on that.

jadijadi · 2024-05-19T13:59:30Z

Can you please add zerocopy to the skip list in deny.toml or, as you are the author of utmp-classic, upgrade its zerocopy dependency?

done. upgraded to the latest zerocopy

lcheylus · 2024-05-21T14:40:37Z

tests/by-util/test_users.rs

Add spell-checker exception for testuser => prevent error in CI job "Style/spelling (ubuntu-latest, feat_os_unix)" : tests/by-util/test_users.rs:41:27 - Unknown word (testuser)

spell-checker:ignore (words) testuser

edited the sample utmp file to make it cleaner. Now it includes two logins for test, one for user and one for root. cleaner for tests and no need for spell-check ignore I imagine.

OK with the last version of your tests/fixtures/users/openbsd_utmp file:

test tests/by-util/test_users.rs::test_users_check_name_openbsd is OK to check user test

no need to add an exception for spell-check.

lcheylus · 2024-05-21T15:13:30Z

@jadijadi Congrats for your work to add support for OpenBSD on coreutils project. I will use your utmp-classic crate to fix uptime command on OpenBSD (see issue # #5664) (WIP in my own fork).

Some comments - review for this PR

Please, amend your commit message / title of this PR : "added to the openbsd" is not very understandable. I propose the message "users: support OpenBSD using utmp"
I merged your code in a local branch based on upstream/main :

Build OK on OpenBSD current/amd64 with Rust 1.77.2 :

$ cargo build --no-default-features --features users

$ ./target/debug/coreutils users -V
users 0.0.26
$ ./target/debug/coreutils users -h
Print the user names of users currently logged in to the current host.

Usage: ./target/debug/coreutils users [FILE]
(...)

Unit tests OK with cargo test

$ cargo test --no-default-features --features users
(...)
test test_users::test_invalid_arg ... ok
test test_users::test_users_check_name_openbsd ... ok
test test_users::test_users_no_arg ... ok

test result: ok. 29 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out; finished in 0.14s

Tests on OpenBSD vs GNU coreutils (install package coreutils-9.4p0 via pkg_add) :

With 1 logged user:

$ ./target/debug/coreutils users
fox
$ gusers
fox

With 3 logged users (user fox logged twice + root):

$ ./target/debug/coreutils users
fox fox root
$ gusers
fox fox root

=> same results for commands users with Rust uutils/coreutils and GNU coreutils :)

NB : with OpenBSD native command users, duplicate entries are removed in output (with 3 logged users: users => fox root). But we must keep the same output than GNU coreutils users command.

jadijadi · 2024-05-21T17:26:47Z

@jadijadi Congrats for your work to add support for OpenBSD on coreutils project. I will use your utmp-classic crate to fix uptime command on OpenBSD (see issue # #5664) (WIP in my own fork).

Great and thanks for the warm words. I'm also working on other utmp related tools on OpenBSD. Will send PRs after this one is finalized.

Please, amend your commit message / title of this PR : "added to the openbsd" is not very understandable. I propose the message "users: support OpenBSD using utmp"

done. thanks for the hint.

I merged your code in a local branch based on upstream/main :

Build OK on OpenBSD current/amd64 with Rust 1.77.2 :
...
With 3 logged users (user fox logged twice + root):
$ ./target/debug/coreutils users
fox fox root
$ gusers
fox fox root
=> same results for commands users with Rust uutils/coreutils and GNU coreutils :)

NB : with OpenBSD native command users, duplicate entries are removed in output (with 3 logged users: users => fox root). But we must keep the same output than GNU coreutils users command.

As you mentioned, I kept the GNU compatibility and showing all logged in users and not removing the duplicate user names.

lcheylus · 2024-05-21T19:10:45Z

Great and thanks for the warm words. I'm also working on other utmp related tools on OpenBSD. Will send PRs after this one is finalized.

I have a working implementation for uptime tool on OpenBSD using your utmp-classic crate. I will submit it as soon as this PR is merged.

$ ./target/debug/coreutils uptime -s
2024-05-21 12:36:04

$ ./target/debug/coreutils uptime && guptime
21:04:05 up   8:28, 1 user,  load average: 0.37, 0.48, 0.39
21:04:05  up   8:28,  1 user,  load average: 0.37, 0.48, 0.39

For who unsupported on OpenBSD, I haven't found solution to implement all features equivalent to GNU coreutils version. Some ("print dead processes") need infos from UTMPX, not available in UTMP file.

Please, amend your commit message / title of this PR : "added to the openbsd" is not very understandable. I propose the message "users: support OpenBSD using utmp"

done. thanks for the hint.

Thanks, please edit also the title of this PR ("users: support OpenBSD using utmp")

lcheylus · 2024-06-25T05:56:46Z

@sylvestre @cakebaker Please, could you do a final review of this PR and merge it if OK?

I'm working on a WIP to support uptime on OpenBSD and also use utmp-classic crate added in this PR.

cakebaker · 2024-06-25T14:55:48Z

src/uu/users/src/users.rs

+        let entries = match parse_from_path(&filename) {
+            Ok(data) => data,
+            Err(_) => Vec::new(),
+        };


I think it is more idiomatic to use unwrap_or instead of a match.

Suggested change

let entries = match parse_from_path(&filename) {

Ok(data) => data,

Err(_) => Vec::new(),

};

let entries = parse_from_path(&filename).unwrap_or(Vec::new());

Done as suggested.

cakebaker · 2024-06-25T15:08:01Z

src/uu/users/src/users.rs

+    #[cfg(not(target_os = "openbsd"))]
+    let default_path: &str = utmpx::DEFAULT_FILE;
+    #[cfg(target_os = "openbsd")]
+    let default_path: &str = "/var/log/wtmp";


There is a mismatch with the default path used in uumain: /var/run/utmp (my guess is that this is the correct path). It might make sense to define an OpenBSD specific const with the default path and then use this const in both places.

Thanks @cakebaker , fixed this based on your comment

cakebaker

With the exception of the two details it looks good from my point of view.

OpenBSD uses the original utmp file format so the general `users` does not work there. By using the `utmp-classic` lib, we cna process the current users on the system from the /var/run/utmp and show the list. fixes uutils#5665

github-actions · 2024-06-29T05:16:32Z

GNU testsuite comparison:

Skipping an intermittent issue tests/tail/inotify-dir-recreate (passes in this run but fails in the 'main' branch)

cakebaker · 2024-06-29T05:52:21Z

Thanks!

sylvestre reviewed May 17, 2024

View reviewed changes

jadijadi force-pushed the users-openbsd branch from 4ed9afd to e13c08a Compare May 19, 2024 13:42

lcheylus reviewed May 21, 2024

View reviewed changes

jadijadi force-pushed the users-openbsd branch from e13c08a to 89e4406 Compare May 21, 2024 17:27

jadijadi changed the title ~~users: added to the openbsd~~ users: support OpenBSD using utmp May 22, 2024

sylvestre force-pushed the users-openbsd branch from 89e4406 to c307fd1 Compare June 25, 2024 07:11

cakebaker reviewed Jun 25, 2024

View reviewed changes

jadijadi force-pushed the users-openbsd branch from c307fd1 to e6b3755 Compare June 29, 2024 04:46

users: support OpenBS using utmp

576341b

OpenBSD uses the original utmp file format so the general `users` does not work there. By using the `utmp-classic` lib, we cna process the current users on the system from the /var/run/utmp and show the list. fixes uutils#5665

jadijadi force-pushed the users-openbsd branch from e6b3755 to 576341b Compare June 29, 2024 04:49

cakebaker merged commit f451714 into uutils:main Jun 29, 2024
67 of 68 checks passed

lcheylus mentioned this pull request Jun 29, 2024

users: lint error needless_borrows_for_generic_args on OpenBSD #6508

Closed

BrewTestBot mentioned this pull request Nov 16, 2024

uutils-coreutils 0.0.28 Homebrew/homebrew-core#197947

Merged

moonfruit mentioned this pull request Nov 22, 2024

uutils-selected 0.0.28 moonfruit/homebrew-tap#178

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

users: support OpenBSD using utmp #6406

users: support OpenBSD using utmp #6406

jadijadi commented May 16, 2024

jadijadi commented May 16, 2024

cakebaker commented May 17, 2024

sylvestre May 17, 2024

jadijadi May 18, 2024

sylvestre May 18, 2024

jadijadi May 19, 2024

sylvestre May 17, 2024

jadijadi May 18, 2024

jadijadi May 19, 2024

sylvestre May 17, 2024

jadijadi commented May 18, 2024

jadijadi commented May 19, 2024

lcheylus May 21, 2024 •

edited

Loading

jadijadi May 21, 2024

lcheylus May 21, 2024

lcheylus commented May 21, 2024

jadijadi commented May 21, 2024

lcheylus commented May 21, 2024 •

edited

Loading

lcheylus commented Jun 25, 2024

cakebaker Jun 25, 2024

jadijadi Jun 29, 2024

cakebaker Jun 25, 2024

jadijadi Jun 29, 2024

cakebaker left a comment

github-actions bot commented Jun 29, 2024

cakebaker commented Jun 29, 2024

users: support OpenBSD using utmp #6406

users: support OpenBSD using utmp #6406

Conversation

jadijadi commented May 16, 2024

jadijadi commented May 16, 2024

cakebaker commented May 17, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jadijadi commented May 18, 2024

jadijadi commented May 19, 2024

lcheylus May 21, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lcheylus commented May 21, 2024

Some comments - review for this PR

jadijadi commented May 21, 2024

lcheylus commented May 21, 2024 • edited Loading

lcheylus commented Jun 25, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cakebaker left a comment

Choose a reason for hiding this comment

github-actions bot commented Jun 29, 2024

cakebaker commented Jun 29, 2024

lcheylus May 21, 2024 •

edited

Loading

lcheylus commented May 21, 2024 •

edited

Loading