Add the ps command to sdb. by tony-zfs · Pull Request #274 · delphix/sdb

tony-zfs · 2021-04-06T03:29:29Z

Pull request checklist

Please check if your PR fulfills the following requirements:

[X ] Tests for the changes have been added (for bug fixes / features)
Docs have been reviewed and added / updated if needed (for bug fixes / features)
[X ] Build was run locally and any changes were pushed
[X ] Lint has passed locally and any fixes were made for failures

Pull request type

Please check the type of change your PR introduces:

Bugfix
[ X] Feature
Code style update (formatting, renaming)
Refactoring (no functional changes, no api changes)
Build related changes
Documentation content changes
Other (please describe):

What is the current behavior?

Issue Number: 248
#248

What is the new behavior?

Supports limited version of ps

Does this introduce a breaking change?

Yes
[X ] No

Other information

sdimitro

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.

sdimitro · 2021-04-06T15:41:35Z

sdb/commands/linux/ps.py

+from sdb.commands.stacks import Stacks
+
+
+def _cmdline(obj: drgn.Object) -> str:


Is this used anywhere?

sdimitro · 2021-04-06T16:00:11Z

sdb/commands/linux/ps.py

+        return ""
+
+
+class Ps(sdb.Locator, sdb.PrettyPrinter):


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.

Updated. Thanks for the advice.

sdimitro · 2021-04-06T16:03:30Z

sdb/commands/linux/ps.py

+                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', \


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.

sdimitro · 2021-04-06T16:05:34Z

sdb/commands/linux/ps.py

+    input_type = "struct task_struct *"
+    output_type = "struct task_struct *"
+
+    FIELDS: Dict[str, Callable[[drgn.Object], Union[str, int]]] = {


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?

sdimitro · 2021-04-06T16:08:47Z

sdb/commands/linux/ps.py

+                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, \


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.

sdimitro · 2021-04-06T16:15:11Z

sdb/commands/linux/ps.py

+                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, \


Looking at man ps I think the correct long-form for this is --pid.

sdimitro · 2021-04-06T16:24:14Z

sdb/commands/linux/ps.py

+        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(',')


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="+",.

Nice catch - I'll update.

sdimitro · 2021-04-06T16:27:21Z

tests/integration/test_linux_generic.py

+    "ps",
+    "ps -C bash",
+    "ps -p 4275",
+    "ps -e",
+    "ps -x",
+    "ps -o pid,ppid",
+    "ps -A",


Thanks for extending the test-suite!

sdimitro · 2021-04-06T16:30:38Z

sdb/commands/stacks.py


    @classmethod
-    def _init_parser(cls, name: str) -> argparse.ArgumentParser:
+    def _init_parser(cls, name: str) -> argparse.ArgumentParser():


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.

Thanks - it sure did!

sebroy · 2021-04-06T18:58:38Z

I would love to see sample output of the new subcommand as part of the PR.

tony-zfs · 2021-06-24T02:36:45Z

Updated. Is there anything else needed to get this to be accepted?

PaulZ-98 · 2021-06-25T18:40:14Z

I would love to see sample output of the new subcommand as part of the PR.

If you drill into the commit, there are sample outputs used in the automated testing of the new command. Look for tests/integration/data/regression_output/linux/ps -e and the like.

sdimitro suggested changes Apr 6, 2021

View reviewed changes

tony-zfs force-pushed the add_ps_support branch 2 times, most recently from f6d2531 to 27dac29 Compare April 13, 2021 16:20

Add the ps command to sdb.

6d01920

tony-zfs force-pushed the add_ps_support branch from 27dac29 to 6d01920 Compare April 19, 2021 02:13

Merge branch 'master' into add_ps_support

30a6a7c

dlpx-tfc-github-manager bot deleted the branch delphix:master January 5, 2023 22:05

		from sdb.commands.stacks import Stacks


		def _cmdline(obj: drgn.Object) -> str:

		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(',')

Conversation

tony-zfs commented Apr 6, 2021 • edited by palash-gandhi Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pull request checklist

Pull request type

What is the current behavior?

What is the new behavior?

Does this introduce a breaking change?

Other information

Uh oh!

sdimitro left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sebroy commented Apr 6, 2021

Uh oh!

tony-zfs commented Jun 24, 2021

Uh oh!

PaulZ-98 commented Jun 25, 2021

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

4 participants

tony-zfs commented Apr 6, 2021 •

edited by palash-gandhi

Loading