search

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 #28

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 removal of the PARSE macros, which were riddled with side effects and suffered in readability as a result. More importantly, the replacement of the PARSE macros with more readable functions also fixes a bug that prevented BPFd from accepting user input that had less than two arguments, forcing users to add an additional dummy argument for commands that only needed one real argument.

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

jcanseco commented 6 years ago

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 I think it might be better if you approve PR #26 first. Once that's approved, I'll apply these changes to the user symbol lookup logic as well.

joelagnel commented 6 years ago

I don't think this simplifies anything. You need to pass more arguments to the parse function, check for error conditions after calling them. How is that simplying anything? Overall I find it quite horrible :-(. I would have at least considered it if it made the code simpler. Further you can easily solve the passing extra argument problem by other methods and can also get rid of the PARSE_FIRST api by just keep track of whether first argument is parsed or not in a variable. I suggest park this for now and move on to low hanging fruit clean ups than a major change and get back to this later. It's not like we don't have argument parsing support at all right now.

On Wed, Mar 14, 2018, 12:20 PM Jazel Canseco notifications@github.com wrote:

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 removal of the PARSE macros, which were riddled with side effects and suffered in readability as a result. More importantly, the replacement of the PARSE macros with more readable functions also fixes a bug that prevented BPFd from accepting user input that had less than two arguments, forcing users to add an additional dummy argument for commands that only needed one real argument.

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

You can view, comment on, or merge this pull request online at:

https://github.com/joelagnel/bpfd/pull/28 Commit Summary

  • Revamp the parsing logic for user inputs on stdin

File Changes

Patch Links:

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/joelagnel/bpfd/pull/28, or mute the thread https://github.com/notifications/unsubscribe-auth/AACSVOeBovkFdCRzwehWH0FMVRL4vD6Pks5teW1ogaJpZM4SrB3r .

jcanseco commented 6 years ago

Woops didn't mean to close this. New PR: #29