fix: move saving logic outside captor package #25
Conversation
Signed-off-by: Alessio Greggi <ale_grey_91@hotmail.it>
Signed-off-by: Alessio Greggi <ale_grey_91@hotmail.it>
| @@ -35,13 +40,29 @@ by passing the function name symbol and the binary args. | |||
| `, | |||
| Example: " harpoon -f main.doSomething ./command arg1 arg2 ...", | |||
| Run: func(cmd *cobra.Command, args []string) { | |||
| opts := captor.CaptureOptions{ | |||
|
|
|||
| functionSymbolList := strings.Split(functionSymbols, ",") | |||
There was a problem hiding this comment.
I'm always worried when I see such code
And empty string will lead to a slice of 1 string with empty string in it.
So []string{""}
I would either check the length of functionSymbols before splitting, or I would skip in the for loop when strings.TrimSpace(item) == ""
I'm unsure if it can happen with your cost. But I cannot help myself to raise this to your attention
There was a problem hiding this comment.
Couldn't agree more with you. I'll add some checks to avoid that sometimes like so could happen. Thanks for raising the issue.
There was a problem hiding this comment.
Since the --function flag is marked as required, we should never face with an empty value in input.
So for now I would skip any control and add them in the future.
cmd/capture.go
Outdated
| syscalls, err := captor.Capture(functionSymbol, args, captureOpts) | ||
| if err != nil { | ||
| fmt.Printf("error capturing syscall: %v", err) | ||
| os.Exit(1) |
There was a problem hiding this comment.
This is a bad pattern.
No os.Exit should be found outside main.go
It's outside the scope of your PR, and could be addressed in another one.
But here I would use RunE not Run because RunE signature supports returning an error.
So the os.Exit can be found in main.go only
This way you can test your code, and you will have one code to handle things
There was a problem hiding this comment.
Please note it could also mean you could use errors wrapping
So instead of this
fmt.Printf("error capturing syscall: %v", err)
You would use return fmt.Errorf("error capturing syscall: %w", err)
This way the fmt.Printis only present in main.go
There was a problem hiding this comment.
I'm removing this bad pattern from capture.go, but for the other files I'll open another PR.
I don't want to fill this PR with too many commits out of scope.
| } | ||
| exitFuncProbe, err := bpfModule.GetProgram(uprobeExitFunc) | ||
| if err != nil { | ||
| fmt.Printf("error loading program (%s): %v\n", uprobeExitFunc, err) | ||
| os.Exit(-1) | ||
| return nil, fmt.Errorf("error loading program (%s): %v", uprobeExitFunc, err) |
There was a problem hiding this comment.
All errors should be wrapped with %w, not %v
|
PR and commit message talk about "captore" while it should be capture |
This was intentionally named captor since is the package name, where the Capture function is implemented. Not sure this is the right name for describing "someone who captures something" but this was the expected meaning. |
|
Captor then, but it's connoted I think, maybe catcher then? |
I think I get what you didn't get, and that created confusion. I was not reporting than captor was a bad name at first. I reported you used captorE with a trailing "e" in PR and commit message |
alegrey91
changed the title
I see! thanks for clarifying ;) |
Signed-off-by: Alessio Greggi <ale_grey_91@hotmail.it>
Signed-off-by: Alessio Greggi <ale_grey_91@hotmail.it>
|
Thanks for the review @ccoVeille. |
|
Yes, you should take a look at other files and refactor them too. |
Fix #21