Skip to content

tests: (chcpu) add missing tests#4390

Open
cgoesche wants to merge 3 commits into
util-linux:masterfrom
cgoesche:chcpu_tests
Open

tests: (chcpu) add missing tests#4390
cgoesche wants to merge 3 commits into
util-linux:masterfrom
cgoesche:chcpu_tests

Conversation

@cgoesche
Copy link
Copy Markdown
Collaborator

No description provided.

@cgoesche cgoesche force-pushed the chcpu_tests branch 9 times, most recently from 408fc4a to 274988b Compare May 31, 2026 17:23
Comment thread sys-utils/chcpu.c Outdated
Comment thread sys-utils/chcpu.c Outdated
maxcpus = get_max_number_of_cpus();
if (maxcpus < 1)
errx(EXIT_FAILURE, _("cannot determine NR_CPUS; aborting"));

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

You get the maxcpus from the running system also on --sysroot. lscpu uses /sys/devices/system/cpu/kernel_max as the primary source and falls back to get_max_number_of_cpus() on a "live" system (see sys-utils/lscpu-cputype.c: lscpu_read_cpulists()).

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Addressed in the next commit aac71e4

@cgoesche
Copy link
Copy Markdown
Collaborator Author

cgoesche commented Jun 1, 2026

Along the way I fixed a couple other issues that I encountered while testing

@cgoesche cgoesche force-pushed the chcpu_tests branch 6 times, most recently from bf19354 to eaed5b3 Compare June 2, 2026 02:57
Comment thread sys-utils/chcpu.c Outdated
@karelzak
Copy link
Copy Markdown
Collaborator

karelzak commented Jun 2, 2026

@cgoesche I again see a big hammer in your hands ...;-(

It was originally about --sysroot and tests; the last missing was maxcpus initialization. Can we go back in time and do just --sysroot in this PR?

Please. :-)

@cgoesche
Copy link
Copy Markdown
Collaborator Author

cgoesche commented Jun 2, 2026

Yes, I sort of feared this would be a bit invasive, the reason I put everything in here is that I wanted to make sure the changes don't introduce regressions. But I can cherry pick these changes to another PR for sure and keep this PR only for the sysroot and tests 👍

Note that as I am testing the last enabled CPU warning as well, the tests will probably fail when I move the bug fixes to a different PR unless I keep it in here too.

Please let me know @karelzak

cgoesche added 3 commits June 2, 2026 07:45
This patch makes it possible to configure the CPUs on a Linux
system other than the one on which the chcpu command is called.
To achieve this users can simply define the root directory of
the sys/ directory with the --sysroot command line option. It
is also beneficial for regression tests, as these make use of
sysfs tar archive dumps.

To properly implement this new feature, the syspath initialization
had to be deferred until after all option arguments, especially
--sysroot, have been parsed, which simplified the path access check.

Along the way a small refactoring of the enable,disable,configure &
deconfigure option parsing had been done, more precisely the CPU list
option argument is now saved in a variable 'cpu_list_arg' to defer
the CPU list validity check until after all option arguments have
been parsed to simplify the logic. Lastly, getopt(3)'s 'optarg' variable
is used instead of argv[optind-1] to store the options argument, which
is more idiomatic and readable.

Signed-off-by: Christian Goeschel Ndjomouo <cgoesc2@wgu.edu>
Signed-off-by: Christian Goeschel Ndjomouo <cgoesc2@wgu.edu>
Signed-off-by: Christian Goeschel Ndjomouo <cgoesc2@wgu.edu>
@cgoesche
Copy link
Copy Markdown
Collaborator Author

cgoesche commented Jun 2, 2026

Note that as I am testing the last enabled CPU warning as well, the tests will probably fail when I move the bug fixes to a different PR unless I keep it in here too.

I updated the expected test output files for now, in a later iteration where this warning bug is fixed we can simply adjust the files again.

@cgoesche cgoesche mentioned this pull request Jun 2, 2026
@karelzak
Copy link
Copy Markdown
Collaborator

karelzak commented Jun 2, 2026

If you think we the context struct, do it, but in a way that is easy to review; that means just replace foo with cxt->foo and nothing else. In the next commit, you can add a new function, then add a new feature like --sysroot, fix a bug, etc. Keep the commit simple and easy to review.

(Yes, sometimes it's tricky and sometimes impossible.)

@cgoesche
Copy link
Copy Markdown
Collaborator Author

cgoesche commented Jun 2, 2026

If you think we the context struct, do it, but in a way that is easy to review; that means just replace foo with cxt->foo and nothing else. In the next commit, you can add a new function, then add a new feature like --sysroot, fix a bug, etc. Keep the commit simple and easy to review.

(Yes, sometimes it's tricky and sometimes impossible.)

I have kept this PR as simple to review as possible for you, we can work out the less related stuff later in #4396 :)

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.

2 participants