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.
Details
Details
- Reviewers
kib - Group Reviewers
manpages - Commits
- rS365880: Add pargs, penv, pwdx commands and aliases to procstat(1).
Diff Detail
Diff Detail
- Repository
- rS FreeBSD src repository - subversion
- Lint
Lint Not Applicable - Unit
Tests Not Applicable
Event Timeline
Comment Actions
LGTM. It would be great if you could run the usual 'mandoc -Tlint' and 'igor' checks.
Comment Actions
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 #
Comment Actions
Can you put sample output from the new commands into the review summary ?
| usr.bin/procstat/procstat.1 | ||
|---|---|---|
| 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. |
| usr.bin/procstat/procstat.c | ||
| 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 (). |
| usr.bin/procstat/procstat.h | ||
| 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. |
| 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. |
Comment Actions
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
| usr.bin/procstat/Makefile | ||
|---|---|---|
| 13 ↗ | (On Diff #77029) | Order these two alphabetically, i.e. around ptlwpinfo. |
| usr.bin/procstat/procstat.c | ||
| 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. |
| usr.bin/procstat/procstat.h | ||
| 34 ↗ | (On Diff #77029) | No, for headers it is still correct to put $FreeBSD$ as comment. |