usr.bin/procstat: Switch procstat from subcommand flags to verbs

[kdrakehp_zoho.com]

Summary

• Use an enumerated value instead of separate flags for commands
• Look for a verb if no command flag is set
• Lookup the "xocontainer" value based on the command
• Document the new command verbs in the man-page

Test Plan

I checked that each of the new commands produces the same output as
their command flag counterparts, and that all of the tests passed.

[brooks]

Note to other reviewers. This change is at my suggestion and it's from the ideas page (https://wiki.freebsd.org/IdeasPage?highlight=%28procstat%29#Switch_procstat_from_subcommand_flags_to_verbs). The motivation to switch to subcommands comes from conflict after conflict with CheriBSD additions to procstat. It's also more natural for the various exclusive modes to be subcommands and the vastly larger namespace of whole words make more room for expansion vs single character flags.

usr.bin/procstat/procstat.1

51 ↗

(On Diff #28821)

I think the new command based versions should go first so people get used to them.

usr.bin/procstat/procstat.c

107 ↗

(On Diff #28821)

How many of these we want is an interesting question. Some shortcuts seem useful, but too many cloud up the namespace.

152 ↗

(On Diff #28821)

There should probably be a list of commands some where (even if that's just another command).

440 ↗

(On Diff #28821)

I think we want to reject -C on files to encourage migration to the new syntax. Maybe check in cmdopt_files()?

518 ↗

(On Diff #28821)

style(9) permits long while bodies like this without braces, but I strongly prefer adding the braces to prevent future errors.

[allanjude]

Thank you for working on this

usr.bin/procstat/procstat.c

107 ↗

(On Diff #28821)

I agree. For some of them we might be able to just use substrings instead of having 'tsig' and 'tsigs', 'fd' and 'fds' etc

152 ↗

(On Diff #28821)

+1

[kdrakehp_zoho.com]

• Move the new command verb usages above those of the command flags
• Add a list of commands to the usage message
• Add around additional loop bodies
• Move more command data to the main table, to be more inline with other sub-command implementations in the tree (geom, camcontrol, etc.)

[kdrakehp_zoho.com]

• Correct the order of upper/lower case options in the getopt string

[kdrakehp_zoho.com]

• Restage changes after r318968

[kdrakehp_zoho.com]

• Add support for looking up plural and/or substring commands
• Use function pointers instead of enumerated values
• Reorder the switch statements for getopt(3)

[brooks]

usr.bin/procstat/procstat.1

94 ↗

(On Diff #29106)

I'm really not a fan of substring support. Shell integrated command line completion is a much better approach to this problem and doesn't pollute the namespace by making things like procstat a valid.

[kdrakehp_zoho.com]

usr.bin/procstat/procstat.1

94 ↗

(On Diff #29106)

Looking back on it I tend to agree.

usr.bin/procstat/procstat.c

107 ↗

(On Diff #28821)

I think that the common idioms can be kept: "bin" as in "/bin", "env" as in "/usr/bin/env",
and "args" is a fairly common usage. The other shortcuts were mostly referring to
internal names ("xocontainer" & function names).

Questions regarding names that fall outside of the "idiom" glob:

• Should the "fd" shortcut be kept?
• Should a longer version of "tsignals" be provided?
• Should a short version of "credentials" be provided?
• Should all of the "xocontainer" names be supported?

[kdrakehp_zoho.com]

Remove substring matching

[brooks]

Sorry for letting this sit so long. I'm finally getting back to this now that I can run FreeBSD 12 binaries on my build systems.

Other than the two comments above I think we're ready to go.

One thing that would be a good idea would be to add some tests using subcommands.

usr.bin/procstat/procstat.c

64 ↗

(On Diff #29121)

I think the list should be sorted.

112 ↗

(On Diff #29121)

the sizeof bits should use the nitems macro.

[kdrakehp_zoho.com]

• Sort the command table
• Use the `nitems' macro
• Add subcommands to the existing tests

[kdrakehp_zoho.com]

• Add ``#!/bin/sh'' to the test script

[brooks]

In D10916#242208, @kdrakehp_zoho.com wrote:

• Add ``#!/bin/sh'' to the test script

This change should be reverted. This isn't an sh script.

IMO the decision to name atf-sh scripts .sh was a mistake. We should probably name them .atf or something, but that's beyond the scope of this change. We should also ensure they don't have execute permission in svn.

[kdrakehp_zoho.com]

• Remove ``#!/bin/sh'' from the test script, reverting the previous change
• Unset the execute bit on the test script

[brooks]

Great! This looks ready to go. I'll apply to a tree and commit shortly.

[brooks]

Arg. I was ready to go, but ran into conflicts with a change to usage() by @ngie. I'm not sure how best to blend the result. If we add that expansion to the subcommands the result will got from unwieldy to incomprehensible (not to mention not fitting on an 80 column display).

In some ways the best answer might be to remove usage display of the old style command flags.

[brooks]

In D10916#242246, @brooks wrote:

Arg. I was ready to go, but ran into conflicts with a change to usage() by @ngie. I'm not sure how best to blend the result. If we add that expansion to the subcommands the result will got from unwieldy to incomprehensible (not to mention not fitting on an 80 column display).

Rather, not fitting on a 45-row terminal. Width is fine, but the message was getting long already.

[kdrakehp_zoho.com]

In D10916#242248, @brooks wrote:

In D10916#242246, @brooks wrote:

Arg. I was ready to go, but ran into conflicts with a change to usage() by @ngie. I'm not sure how best to blend the result. If we add that expansion to the subcommands the result will got from unwieldy to incomprehensible (not to mention not fitting on an 80 column display).

Rather, not fitting on a 45-row terminal. Width is fine, but the message was getting long already.

The command specific flags could be embedded in the sub-command flag list: ... -f [-C] | -i [-n] | -j [-n] | -k [-k] ...
Also, the -L sub-command flag can not be mixed with other sub-commands because of how flags are checked.

If embedding the flags is acceptable then that would reduce the line count of the new usage message, and add one one additional line to this diff's usage message.

[ngie]

In D10916#242248, @brooks wrote:

In D10916#242246, @brooks wrote:

Arg. I was ready to go, but ran into conflicts with a change to usage() by @ngie. I'm not sure how best to blend the result. If we add that expansion to the subcommands the result will got from unwieldy to incomprehensible (not to mention not fitting on an 80 column display).

Rather, not fitting on a 45-row terminal. Width is fine, but the message was getting long already.

Ack -- sorry! I think your changes are more suitable -- I was trying to clarify -L usage in previous commits, which involved clarifying how things needed to be called in general since it wasn't very clear.

usr.bin/procstat/procstat.1

20 ↗

(On Diff #31072)

Please run make manlint on the changes.

108 ↗

(On Diff #31072)

subcommand is the proper spelling.

155 ↗

(On Diff #31072)

thread(s) seems like more a natural way to write this.

94 ↗

(On Diff #29106)

I'm a bit curious about substring support -- will substrings be accidentally matched with the old specification mechanism if there's a program that starts with sig, etc?

usr.bin/procstat/procstat.c

107–108 ↗

(On Diff #31072)

Consolidate declarations on one line?

128 ↗

(On Diff #31072)

This relies on the sorting you did above -- should this be enforced a bit better via qsort or a relevant comment?

204–207 ↗

(On Diff #31072)

Consolidate the int/size_t declarations on one line?

252 ↗

(On Diff #31072)

Should this initialization be committed separately?

476 ↗

(On Diff #31072)

getopt(3) supports not specifying an option string? I didn't know that!

556 ↗

(On Diff #31072)

(picking a random item) should argv be const char *argv[] (as a hint to the compiler)?

usr.bin/procstat/procstat_files.c

317 ↗

(On Diff #31072)

(picking a random line) these bitwise AND/OR truth checks are actually a style(9) regression. It should be done using an idiom like:

if ((procstat_opts & PS_OPT_CAPABILITIES) != 0)

usr.bin/procstat/tests/procstat_test.sh

58 ↗

(On Diff #31072)

It would be nice to add more tests beforehand to ensure that code doesn't regress in areas that aren't currently being tested after the change.

[kdrakehp_zoho.com]

• Update usage
• Correct the order of `cmd_table'
• Remove unnecessary hyphens
• Correct `manlint' errors
• Make suggested changes to `procstat.1'
• Consolidate some variable declarations
• Add additional `const' specifiers
• Change flag tests to comparisons

[kdrakehp_zoho.com]

• Correct an overly long line

[brooks]

Merge to FreeBSD head prior to commit to SVN.

[brooks]

Thank you for doing this work! Sorry it took me so long to commit it.