joelagnel / bpfd

BPFd (Deprecated, please see README.md) : Berkeley Packet Filter daemon (BPFd). Makes it possible to run BCC tools across systems.
Apache License 2.0
95 stars 23 forks source link

Revamp the parsing logic for user inputs on stdin #29

Closed jcanseco closed 6 years ago

jcanseco commented 6 years ago

This centralizes the parsing logic for user inputs on stdin into one component, 'cmd_parsers.c', and encapsulates the relevant data (i.e. the BPFd command and its arguments) into one data structure, the 'user_inputs' struct. This effectively leads to the removal of all the confusing pointer manipulation logic in the main while loop in bpfd.c that dealt with user input parsing.

This also leads to the reduction of side effects caused by the PARSE macros, since the PARSE macros themselves no longer need to do any string tokenization using strtok().

Additionally, this also eliminates the PARSE_FIRST macros, and fixes the bug where BPFd was unable to accept commands with less than 2 arguments, forcing users to add an additional dummy argument.

Signed-off-by: Jazel Canseco jcanseco@google.com

Fixes #16

joelagnel commented 6 years ago

Please share any details on test coverage for this patch?

jcanseco commented 6 years ago

It's the same as the original PR that I accidentally closed. I'll copy-paste it below:

Ran the following BCC tools on the HiKey board as a smoke test:

I also ran the following on ShellRemote:

All of the above seemed to work fine.

I tried filetop.py as well. It wasn't showing anything, but now I recall that it's probably due to the kernel on my hikey board; it was a issue we never actually fixed.

I also tried hardirqs.py, but I'm not sure how to generate a hardware interrupt considering I don't have any hardware peripherals for my HiKey board like touch screens.

jcanseco commented 6 years ago

Also in case you're wondering, I already propagated the usym symbol changes from PR #26 to this PR.

joelagnel commented 6 years ago

Jazel, what do you mean by "it was a issue we never actually fixed" ? filetop should be working fine. If not, there is a bug in your patch. For hardirqs.py, you don't need to "generate" an interrupts. Interrupts are flying in all the time, and the tool should be able to show you something by default

jcanseco commented 6 years ago

Reminder: we discussed this in person the other day. Recall that "filetop.py not working" is an issue with my HiKey kernel, and not due to these patches or the BCC/BPFd software in general. We concluded that I try running filetop.py on your HiKey board in order to test it, but I am still waiting for your board to be available.

I also tried running hardirqs.py once again, but it's still not showing a "correct" result. Right now it doesn't crash or anything, but it just shows an empty table. That is, I get the following:

# ./hardirqs.py
Tracing hard irq event time... Hit Ctrl-C to end.                                                                                                                                                                                                                                                                                                                        
^C                                                                                                                                                                                                                                                                                                                                                                       
HARDIRQ                    TOTAL_usecs

When I should be getting something like the following:

# ./hardirqs.py
Tracing hard irq event time... Hit Ctrl-C to end.                                                                                                                                                                                                                                                                                                                        
^C                                                                                                                                                                                                                                                                                                                                                                       
HARDIRQ                    TOTAL_usecs
callfuncsingle0                      2
callfuncsingle5                      5
callfuncsingle6                      5
callfuncsingle7                     21
blkif                               66

There's a chance that this might be an issue unique to my HiKey kernel as well, which would mean that I'd have to test this on your board as well.

joelagnel commented 6 years ago

Can you please do some debugging to find out why hardirqs.py isn't showing you anything? So far you haven't provided any additional information other than "table is empty". That's not very helpful

joelagnel commented 6 years ago

Also please provide any additional evidence that the issues you are seeing happen even without this patch so we can merge/close the PR (and also please create additional issues in the issue tracker for the Hikey issues you're seeing along with additional debug data (obtained from BCC_REMOTE_DEBUG=1 and/or debug flags set in BCC init.py, and kernel logs)

jcanseco commented 6 years ago

Regarding your first point, I'd like to first borrow your board to verify that the issue with hardirqs.py isn't specific to my board, just like how filetop.py works for your board but not mine. It's hard to know whether debugging this issue would be a fruitful exercise when I don't even know if this issue is unique to my board or not.

Regarding your second point: to be honest, I'm not quite sure what other evidence you need since the outputs are identical both with and without the patch (i.e. empty tables for both hardirqs.py and filetop.py). I also think it's better to first find out if this behavior is unique to my board or not before creating GitHub issues, since the root cause may nothing to do with BPFd or BCC at all.

In other words...

  1. I need your board
  2. I believe this patch can merged since the behavior I've observed occurs both with and without the patch, which means the cause is not this patch