Page MenuHomeFreeBSD

Add nproc(1)
ClosedPublic

Authored by mjg on Feb 4 2023, 11:35 PM.
Tags
None
Referenced Files
F84115410: D38386.id116799.diff
Sun, May 19, 3:08 PM
F84113929: D38386.id116699.diff
Sun, May 19, 2:37 PM
Unknown Object (File)
Sat, May 11, 9:59 AM
Unknown Object (File)
Sat, May 11, 5:21 AM
Unknown Object (File)
Fri, May 10, 11:01 AM
Unknown Object (File)
Thu, May 9, 2:36 PM
Unknown Object (File)
Tue, May 7, 9:18 PM
Unknown Object (File)
Fri, May 3, 4:25 AM

Details

Summary
This program prints the number of CPU threads it can run on, while
respecting cpusets (or not, depending on switches).

It aims to be compatible with nproc as found in GNU coreutils.

Diff Detail

Repository
rG FreeBSD src repository
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

mjg requested review of this revision.Feb 4 2023, 11:35 PM
mjg updated this revision to Diff 116511.
mjg created this revision.
mjg updated this revision to Diff 116515.

Minor style improvements, nitpicking and questions.

bin/nproc/nproc.c
43

This is an invitation for a discussion more than a legitimate suggestion, but why do we seem to prefer fprintf calls (and multiple of them) in base userland instead of a single fputs (or fputs_unlocked maybe) call?

68

I can imagine you had a reason not to do it like this, but I can't think of anything specific.

95

Wouldn't a break do the same? It'd look more consistent and idiomatic, in my opinion.
Or call exit(0) like you do in case OPT_HELP.
And/Or make help call exit(0) like version does, then replace calls to usage with calls to help and remove usage altogether.

If I'm not wrong about this, I think it's worth fixing to make the new program a bit more consistent from the start.

129
bin/nproc/nproc.c
95

I missed that usage exits with 1. But still you could parameterize help and call it directly, and then remove usage.

TODO: write the manpage

Consider taking this as a starting point: https://reviews.freebsd.org/P553

bin/nproc/nproc.c
68

I can imagine you had a reason not to do it like this, but I can't think of anything specific.

You've explained this, but in the comment for the whole function, so I missed it.

mjg edited the summary of this revision. (Show Details)
  • add manpage from pstef@
  • xref from cpuset.1
des added inline comments.
bin/nproc/nproc.c
43

Premature optimization. The compiler will replace the fprintf() call with more efficient code if appropriate.

  • relicense to 2-clause bsd
bin/nproc/Makefile
2

This is not needed.

debdrup added inline comments.
bin/nproc/Makefile
6

I think you're missing the directive to install the manual page?

bin/nproc/Makefile
6

No, it's implicit, as long as it goes in expected section.

bin/nproc/nproc.c
77

(you'll have to #include <sysexits.h> at the top)

91
120
151
bin/nproc/nproc.1
45

You'll need this to prevent mandoc from inserting random line break within the author section.

emaste added inline comments.
bin/nproc/nproc.c
90

Maybe take a page from lld
LLD 14.0.5 (FreeBSD llvmorg-14.0.5-0-gc12386ae247c-1400004) (compatible with GNU linkers)

e.g. something like
(compatible with GNU coreutils 8.32)?

but just a little bikeshed point

bin/nproc/nproc.c
90

that would miss the point, i noted in the ocmment this is whitespace-compatible with the original:

nproc (GNU coreutils) 8.32

if some funny program decides to --version and grab the version, they will get a sane number

bin/nproc/nproc.c
90

but some silly program may equally do something like nproc --version | grep -q "GNU coreutils"

  • drop src.mk
  • license the manpage as 2 clause bsd, per pstef's permission
bin/nproc/nproc.c
77

other tools like ls don't do it, so i'm going to scratch this bit

bin/nproc/nproc.c
90

that would be most peculiar

des requested changes to this revision.Feb 7 2023, 5:23 PM
des added inline comments.
bin/nproc/nproc.c
77

Just because older code gets it wrong doesn't mean we shouldn't get it right in new code.

This revision now requires changes to proceed.Feb 7 2023, 5:23 PM
bin/nproc/nproc.c
90

not really, the reason lld has the peculiar version string is that libtool does essentially this (I think it may just grep for GNU though)

bin/nproc/nproc.c
90

but the code cannot claim to be gnu coreutils and even then, grabbing the version number from it runs into the whitespace issue i mentioned.

  • use sysexit
bin/nproc/nproc.c
77

well i'm not going to die on *that* hill

mjg marked 12 inline comments as done.Feb 8 2023, 12:28 AM
bin/nproc/nproc.c
131

My question about fprintf() remains unanswered, but it is not a blocker.

bin/nproc/nproc.c
90–93

The code didn't check whether the result of strtol would fit into the int ignore. This means a user could cause an overflow, which is UB. In reality this means that very large values might wrap around into the accepted range and the program would run successfully without doing what the user intended.
It also didn't check whether the argument consists only of digits.
strtonum simplifies all this and provides a nice error message. If you want to go down this route, you need to declare const char *errstr and include limits.h for INT_MAX.

  • plug return 0
  • use strtonum
mjg marked 3 inline comments as done.Feb 8 2023, 7:30 PM
mjg added inline comments.
bin/nproc/nproc.c
90–93

fair point, thanks!

pstef added inline comments.
bin/nproc/nproc.1
2

I doubt this has any use in manual pages. It is used in C sources in hope that an indent run will not reformat the comment.

49

I wonder if this won't be confusing. FreeBSD 14 will be the first FreeBSD to have nproc in base, but it wasn't the first system to provide it (to us it's obvious, but we're not writing manual pages just for ourselves).

This revision is now accepted and ready to land.Feb 8 2023, 7:45 PM
This revision was automatically updated to reflect the committed changes.
mjg marked an inline comment as done.