Add the ps command to sdb. #274
Conversation
There was a problem hiding this comment.
Hey @tony-zfs thank you for opening the PR!
From what I see the failures in the test-suite are from our frozed version of mypy and the use of -h option from argparse. I have some more details on both bundled together with the rest of my feedback.
sdb/commands/linux/ps.py
Outdated
| from sdb.commands.stacks import Stacks | ||
|
|
||
|
|
||
| def _cmdline(obj: drgn.Object) -> str: |
sdb/commands/linux/ps.py
Outdated
| return "" | ||
|
|
||
|
|
||
| class Ps(sdb.Locator, sdb.PrettyPrinter): |
There was a problem hiding this comment.
A lot of code in this class overlaps with threads.py. Instead of duplicating code in both places did you try the following:
[1] Extend the threads command to handle the extra columns that you added (e.g. uid) and also the column logic (e.g -o , -h, etc..)
[2] Make the ps command use the threads command under the hood and keep the arguments that make it look like the linux userland ps command (e.g. -p, --ppid, etc..).
If you want an example of layering a command on top of another command checkout the spa command. It is using the avl walker for its Locator/no-input method, and vdev for its pretty-printer.
As for generic use of the Table class and how to implement the sort-by-column functionality you can look at either the slubs or spl_kmem_caches.
There was a problem hiding this comment.
Updated. Thanks for the advice.
sdb/commands/linux/ps.py
Outdated
| help="Print only the process IDs of a command") | ||
| parser.add_argument('-x', '--x', action='store_true', \ | ||
| help="Show PID, TIME, CMD") | ||
| parser.add_argument('-h', action='store_true', \ |
There was a problem hiding this comment.
Unfortunately argparse doesn't allow us to use -h for anything other than the help message. Fortunately for us looking at man ps I see that that to skip the headers only the long form argument exists --no-headers and --no-heading.
| input_type = "struct task_struct *" | ||
| output_type = "struct task_struct *" | ||
|
|
||
| FIELDS: Dict[str, Callable[[drgn.Object], Union[str, int]]] = { |
There was a problem hiding this comment.
Not that it matters if we go ahead with layering the ps command on top of threads but I don't see task mentioned here. What if a user wants to specify it with the -o option?
sdb/commands/linux/ps.py
Outdated
| help="Show the output without headers.") | ||
| parser.add_argument('-p', '--p', type=str, \ | ||
| help="Select by process ID. Identical to --pid.") | ||
| parser.add_argument('-ppid', '--ppid', type=str, \ |
There was a problem hiding this comment.
Having the long-form and the short-form argument look the same doesn't look right. Looking at man ps it seems like only the long-form is supported --ppid. If you really want a short-form I don't think -P is taken, so we could use that.
In addition, like -p/--pid the type of argument can be a list of integers.
sdb/commands/linux/ps.py
Outdated
| help="Show PID, TIME, CMD") | ||
| parser.add_argument('-h', action='store_true', \ | ||
| help="Show the output without headers.") | ||
| parser.add_argument('-p', '--p', type=str, \ |
There was a problem hiding this comment.
Looking at man ps I think the correct long-form for this is --pid.
sdb/commands/linux/ps.py
Outdated
| pids = [int(x) for x in self.args.p.split(',')] if self.args.p else [] | ||
| ppids = [int(x) for x in self.args.ppid.split(',') |
There was a problem hiding this comment.
what happens here when we get faulty input? (e.g. "asdf, asdfasdf")
If the result is that we blow up maybe it may be easier to switch from the ps --pid 1,2,3 to ps --pid 1 2 3 notation (both are supported from the userland ps utility in Linux from what I can tell). This would mean that we'd need to change the argument type to something like type=int, nargs="+",.
| "ps", | ||
| "ps -C bash", | ||
| "ps -p 4275", | ||
| "ps -e", | ||
| "ps -x", | ||
| "ps -o pid,ppid", | ||
| "ps -A", |
There was a problem hiding this comment.
Thanks for extending the test-suite!
sdb/commands/stacks.py
Outdated
| @@ -152,7 +152,7 @@ def __init__(self, | |||
| self.match_state = "" | |||
|
|
|||
| @classmethod | |||
| def _init_parser(cls, name: str) -> argparse.ArgumentParser: | |||
| def _init_parser(cls, name: str) -> argparse.ArgumentParser(): | |||
There was a problem hiding this comment.
Pressumably mypy bothered you about this change. We've found it to be extremely inconsistent with false-positives being introduced at every new release thus we've frozed our version of mypy in our Github Actions to 0.730 - https://github.com/delphix/sdb/blob/master/.github/workflows/main.yml#L100 . You may want to do the same in your python environment for sdb locally.
|
I would love to see sample output of the new subcommand as part of the PR. |
tony-zfs
force-pushed
the
add_ps_support
branch
2 times, most recently
from
f6d2531
to
27dac29
Compare
tony-zfs
force-pushed
the
add_ps_support
branch
from
27dac29
to
6d01920
Compare
|
Updated. Is there anything else needed to get this to be accepted? |
If you drill into the commit, there are sample outputs used in the automated testing of the new command. Look for |
Pull request checklist
Please check if your PR fulfills the following requirements:
Pull request type
Please check the type of change your PR introduces:
What is the current behavior?
Issue Number: 248
#248
What is the new behavior?
Does this introduce a breaking change?
Other information