Page MenuHomeFreeBSD

Move cpuset's parselist function into libutil
AcceptedPublic

Authored by bapt on Oct 31 2017, 2:39 PM.

Details

Reviewers
swills
jeff
jilles
Group Reviewers
manpages
Summary

Too allow to add cpuset(2) functionality to more utilities than just cpuset(1)
move the parselist code into libutil

While here, make the code a little more "library" friendly, by returning a range
of various errors so that the consumer can check for them and report appropriate
error message to the users

(One of the planed usage is the jail(8) utility)

Diff Detail

Repository
rS FreeBSD src repository
Lint
Lint OK
Unit
No Unit Test Coverage
Build Status
Buildable 12656
Build 12927: arc lint + arc unit

Event Timeline

bapt created this revision.Oct 31 2017, 2:39 PM
bcr added a subscriber: bcr.Oct 31 2017, 5:07 PM

Fixes for the man page.

lib/libutil/cpuset.3
54

You need to do a line break after a sentence stop.

61

s/succesfull/successful/

71

s/CPU/CPUs/

jilles added a subscriber: jilles.Oct 31 2017, 11:08 PM

Note that all my comments on the cpuset_parselist() implementation also apply to the original version in cpuset.c; therefore, they could be fixed in a separate commit.

A unit test would be nice.

lib/libutil/cpuset.3
42

The actual implementation has one asterisk with mask, which matches the below description.

60

The Er macro seems meant for errno values only. Perhaps use Dv instead of Bq Er.

71

This should mention CPU_SETSIZE; otherwise someone may think it is about the number of CPUs in the system.

lib/libutil/cpuset.c
40

should use const char *list (which also needs adding some const below)

44

Perhaps use an unsigned type to avoid undefined behaviour on overflow (see below).

56

It is annoying but the way to pass a char to isdigit and similar macros is to cast it to unsigned char first.

57–61

atoi causes undefined behaviour on overflow; consider strtoul or an open-coded loop.

lib/libutil/libutil.h
205

I don't like depending on the include order like this but it is convention in this file.

bapt updated this revision to Diff 35102.Nov 11 2017, 2:43 PM

Fix manpage issue
Change prototype to using const.
Other changes deserves a dedicated commit that I will do later

bapt updated this revision to Diff 35103.Nov 11 2017, 2:43 PM

Remove files that crept in

bapt marked 7 inline comments as done.Nov 11 2017, 3:08 PM

see D13046 for the tests

lib/libutil/cpuset.c
44

This is will be fixed in a another commit

56

in another commit

57–61

in a separated commit

jeff added inline comments.Nov 11 2017, 8:55 PM
lib/libutil/cpuset.c
46–51

This behavior should really be split off separately from the list handling. I would like to be able to use the list handling code for domains, which also won't be cpumask_t. Instead I would pass in the bitset type and size that cpumask is derived from. So eliminating cpu specific code from this function will make it more useful and generic in general. Other code man choose to derive bit manipulation functions from the bitset macros in the future and this would be compatible.

jilles accepted this revision.Nov 22 2017, 11:01 PM

Accepted with a minor change to the man page.

lib/libutil/cpuset.3
72–73

This sentence is incomplete. It could be spliced to the previous sentence with a comma, or formulated differently.

lib/libutil/cpuset.c
46–51

I think the cpuset_parselist() API is useful as is; the more general ranges-of-numbers code can be extracted out in another commit.

This revision is now accepted and ready to land.Nov 22 2017, 11:01 PM
wblock added a subscriber: wblock.Mar 2 2018, 8:46 PM
wblock added inline comments.
lib/libutil/cpuset.3
72–73
The maximum number of CPUs is
.Va CPU_SETSIZE .