Page MenuHomeFreeBSD

Mimic behavior of Solaris' pargs, penv, pwdx commands

Authored by otis on Sep 3 2020, 1:34 PM.



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.

Diff Detail

rS FreeBSD src repository - subversion
Automatic diff as part of commit; lint not applicable.
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

otis requested review of this revision.Sep 3 2020, 1:34 PM
gbe added inline comments.
118 ↗(On Diff #76612)

I would start that paragraph with a 'The'.

731 ↗(On Diff #76612)

'options' would maybe a more suitable description.

otis set the repository for this revision to rS FreeBSD src repository - subversion.
otis marked 2 inline comments as done.

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

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 #

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

127 ↗(On Diff #77017)

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.

61 ↗(On Diff #77017)

'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.

137 ↗(On Diff #77017)

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.

264 ↗(On Diff #77017)

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

270 ↗(On Diff #77017)

Continuation line should have indent +4 spaces.

271 ↗(On Diff #77017)

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.

335 ↗(On Diff #77017)

Excessive ().

336 ↗(On Diff #77017)

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().

497 ↗(On Diff #77017)

Extra ().

87 ↗(On Diff #77017)

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.

30 ↗(On Diff #77017)

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

#include <sys/cdefs.h>

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.

I guess all review notes have been incorporated.

otis marked 6 inline comments as done.Sep 14 2020, 9:44 PM
root@b13:~ # ps 687
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
13 ↗(On Diff #77029)

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

7 ↗(On Diff #77029)

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.

31 ↗(On Diff #77029)

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

35 ↗(On Diff #77029)

Can you extract that sweep into separate review ?

67 ↗(On Diff #77029)

No space after '*'.

264 ↗(On Diff #77029)

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'.

137 ↗(On Diff #77017)

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

34 ↗(On Diff #77029)

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

otis marked 7 inline comments as done.
This revision was not accepted when it landed; it landed in state Needs Review.Sep 18 2020, 12:59 PM
This revision was automatically updated to reflect the committed changes.