Skip to content

Conversation

@pkannoju
Copy link

crash: port net command.

Please review and let me know your comments.

Signed-off-by: Praveen Kumar Kannoju <[email protected]>
Copy link
Owner

@osandov osandov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for working on this. In addition to the inline comments, I have a few high-level comments:

  • netdev_ipv4s(), netdev_ipv6s(), and neigh_table_for_each_neighbor() all seem really useful and should be public helpers in drgn.helpers.linux.net. You can do that in a separate PR to merge before this or as separate commits in this PR, your choice.
  • The crash command appears to allow combining -a with -s and -S, as well as multiple -s and -S options, and passing a task struct or the implicit current context to the -s and -S options. If we're going to support those options, we should allow that, too. That's a lot of complexity to support all at once, so maybe the initial version of this command can just support -a and the no option case, and then once that's landed we can add -s/-S.
  • The --drgn option is the main motivation for doing this porting work, so it'd be nice for the initial version of this command to support it.

Again, I appreciate your work on this!

print_table(rows)

def print_net_devices(prog: Program) -> None:
rows = [["NET_DEVICE", "NAME", "IP ADDRESS(ES)"]]
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The NET_DEVICE header seems to be centered. You can use CellFormat for that (which might require a type annotation to make mypy happy):

Suggested change
rows = [["NET_DEVICE", "NAME", "IP ADDRESS(ES)"]]
rows: List[Sequence[Any]] = [[CellFormat("NET_DEVICE", "^"), "NAME", "IP ADDRESS(ES)"]]

rows = [["NET_DEVICE", "NAME", "IP ADDRESS(ES)"]]
net_namespaces = network.for_each_net(prog)

for net_namespace in net_namespaces:
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Crash doesn't appear to iterate over all network namespaces. It seems like it defaults to the initial network namespace (init_net), so let's do the same.


for net_namespace in net_namespaces:
for name, dev in for_each_netdev(net_namespace):
net_device_ptr = hex(dev.dev.platform_data)
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From what I can tell, this can just use dev directly:

>>> dev.dev.platform_data == dev
True

(But see the comment below for how to format this better.)

ips = ", ".join(ip_list)
rows.append(
[
net_device_ptr,
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Crash doesn't include the 0x prefix, so use the x format directly:

Suggested change
net_device_ptr,
CellFormat(dev.value_(), "^x"),

rows.append(
[
net_device_ptr,
net_device_name,
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just added the netdev_name() helper that you can use for this. You can use escape_ascii_string() to handle potentially bogus strings (whereas .decode() could potentially throw an exception):

Suggested change
net_device_name,
escape_ascii_string(netdev_name(dev), escape_backslash=True),

Comment on lines +154 to +157
hw_type = str(hwtype_dict.get(int(neigh.dev.type)))
if int(neigh.dev.type) not in hwtype_dict.keys():
hw_type = "ARPHRD_UNKNOWN"
hw_type = hw_type[7:]
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is a simpler equivalent:

Suggested change
hw_type = str(hwtype_dict.get(int(neigh.dev.type)))
if int(neigh.dev.type) not in hwtype_dict.keys():
hw_type = "ARPHRD_UNKNOWN"
hw_type = hw_type[7:]
hw_type = hwtype_dict.get(int(neigh.dev.type), "ARPHRD_UNKNOWN")[7:]

dev_state,
]
)
neigh = neigh.next
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This doesn't seem to be serving any purpose.

Suggested change
neigh = neigh.next

Comment on lines +178 to +187
print(
"PID: "
+ str(pid)
+ " TASK: "
+ hex(task.value_())[2:]
+ " CPU: "
+ str(task_cpu(task))
+ " COMMAND: "
+ task.comm.string_().decode("utf-8")
)
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can use print_task_header() from drgn.commands.crash here.

Comment on lines +196 to +198
"FAMILY:TYPE",
"SOURCE-PORT",
"DESTINATION-PORT",
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't see these columns on my version of crash:

crash> net -S 1 | head
PID: 1        TASK: ffff95cb410d3080  CPU: 2    COMMAND: "systemd"
FD       SOCKET             SOCK
 8  ffff95cb42caf1c0  ffff95cc20fc7400

struct socket {
  state = SS_CONNECTED,
  type = 1,
  flags = 24,
  file = 0xffff95cc479059c0,
  sk = 0xffff95cc20fc7400,

Are they supposed to be there, or was this your addition?

print(
"%-5s %-20s %-20s %-20s %-20s %-20s"
% (
str(cnt),
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't this supposed to be the FD number, too?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants