Mimic behavior of Solaris' pargs, penv, pwdx commands

[otis]

Summary

As a coding exercise and to help me ease the migration of some Solaris scripts, I've added functionality of pargs, penv and pwdx commands to procstat, that also might be of interest to someone else.

I tried to get most of procstat code.

[gbe]

usr.bin/procstat/procstat.1

118

I would start that paragraph with a 'The'.

738

'options' would maybe a more suitable description.

[gbe]

LGTM. It would be great if you could run the usual 'mandoc -Tlint' and 'igor' checks.

[otis]

root@b13:/usr/src/usr.bin/procstat # mandoc -Tlint procstat.1
root@b13:/usr/src/usr.bin/procstat # igor procstat.1
root@b13:/usr/src/usr.bin/procstat #

[kib]

Can you put sample output from the new commands into the review summary ?

usr.bin/procstat/procstat.1

127

I suggest to reorder description to de-empase Solaris part. First describe each of the command, then as the last sentence mention that the intent is to 'mimic the Solaris ...', as a hint to reader.

usr.bin/procstat/procstat.c

60

'cmp' is really a general-purpose flag field. I do not see why would you need one more field ('more'), and why do you need PS_MODE_NORMAL: only COMPAT needs treatment.

129

There is definitive value in adding these p* things as normal procstat commands. Then I am not even sure that hardlinks for solaris compat are very useful, but I do not object.

243

return (NULL). Style requires taking the returned value in braces. Please handle all places, I do not want to repeat it N times.

249

Continuation line should have indent +4 spaces.

250

I do not understand what are you trying to do there. Are you trying to skip path prefix. like '/PATH/penv' ? Then use strrchr() perhaps. Right now you accept something like 'ppenv' as well.

313

Excessive ().

314

usage_func = (const void *)cmd->usage.

But case of function to void * is not portable. Also I do not see a need in this hack, just check flag for cmd as needed in usage().

471

Extra ().

usr.bin/procstat/procstat.h

83

I suggest dropping '_compat' from the name, it is relevant for users that come from Solaris perhaps, but completely irrelevant for the implementation. Also order declarations alphabetically.

usr.bin/procstat/procstat_compat_penv.c

30 ↗

(On Diff #77017)

Look how the tag is added in other files (but not in procstat:

#include <sys/cdefs.h>
__FBSDID("$FreeBSD$");

Procstat seems to not use this way to add tags, but there is no reason why new files should not do that.

62 ↗

(On Diff #77017)

Indent should be +tab.

[otis]

I guess all review notes have been incorporated.

[otis]

root@b13:~ # ps 687
PID TT  STAT    TIME COMMAND
687  -  Is   0:02.55 /usr/sbin/cron -s
root@b13:~ # pargs 687
687:  cron
args[0]: /usr/sbin/cron
args[1]: -s
root@b13:~ # penv 687
687:  cron
envp[0]: PATH=/sbin:/bin:/usr/sbin:/usr/bin
envp[1]: PWD=/
envp[2]: HOME=/
envp[3]: RC_PID=22
envp[4]: BLOCKSIZE=K
root@b13:~ # pwdx 687
687:  /var/cron

[otis]

• Change args to argv

[kib]

usr.bin/procstat/Makefile

13

Order these two alphabetically, i.e. around ptlwpinfo.

usr.bin/procstat/procstat.c

7

New files definitely must get your copyright notice, but changes to existing files do not seems to warrant addition of the copyright holder. Our usual bar is 20% of code from the copyright holder.

30

These two lines with just '*' should be removed.

34

Can you extract that sweep into separate review ?

63

No space after '*'.

129

I probably miss something but I do not see how this suggestion was handled.

249

Why do you use strncmp there ? You are ignoring arbitrary suffixes of ca, so e.g. just 'p' as pprogname would match first command starting with 'p'.

usr.bin/procstat/procstat.h

34

No, for headers it is still correct to put $FreeBSD$ as comment.
Look at the kernel sources.

[kib]

D26475