copying argprinters funcs under libbpfgo

[mtcherni95]

Please note, after consulting with @itaysk , we decided that is better to close this issue with a two-phase commit.
In the first commit (this one) we will simply copy functions in libbpfgo so that tests will pass. In the second commit we will use libbpfgo funcs (under linuxhelpers package) and remove not relevant functions under tracee package (argprinters.go).

[itaysk]

Thanks @mtcherni95 . you should move the tests as well

[grantseltzer]

Since this is exported API we should use this opportunity to think about our naming conventions. All these functions start with 'Print' but none of them do any printing, they return strings. I would rather rename them to things like:

PrintOpenFlags -> OpenFlagsToString or SprintOpenFlags

Also the package name linuxhelpers I feel can be changed to just helpers, or maybe bpfhelpers. We can also just put them in the main libbpfgo package alongside TracePrint().

What do you think @mtcherni95 @itaysk ?

[itaysk]

good point. wdyt of ParseXXX?

I don't think all of the helpers should live in the same package as libbpfgo, I'm fine with renaming the package. @yanivagman wdyt?

[grantseltzer]

ParseXXX sounds good to me. And different package but with different name is also good by me.

[yanivagman]

Funny @grantseltzer that the names that you suggest where the ones used in the original tracee python version (e.g. open_flags_to_str) :-)
I agree that the Print prefix is misleading.
Sprint prefix is a good option (Parse feels too much, isn't it?)
Agree about the package - we should separate libbpf wrappers from other helpers functionality (including the new TracePrint() function). I don't think bpfhelpers is a good name, as not all of this stuff is related to bpf. helpers or linuxhelpers are fine with me

[itaysk]

I don't see the problem with Parse (Parse: analyze (a string or text) into logical syntactic components), and sprintf sounds too technical. but whatever you decide as long as it's in a separate package

[grantseltzer]

@mtcherni95 Could you rename all the functions to ParseXXX, and bring in TracePrint from libbpfgo?

[itaysk]

hi @mtcherni95 are you still working on this?

[grantseltzer]

Hi @mtcherni95 do you still want to work on this? If not is it cool with you if I pick it up to close this out?

[grantseltzer]

@itaysk @yanivagman

After talking with @mtcherni95 I went ahead and pushed the requested changes to this, I will follow up with the second PR described in Michael's original comment right after this merges.

[grantseltzer]

@simar7 Can you please take a look at this?

[simar7]

This file is named as argParsers_test.go but none of the functions are actually test functions themselves. If it's more of an example/toy program to demonstrate usage of TracePipe(), I would suggest renaming the file.

Otherwise we should make TracePipeListen() injectable with a param, something like: TracePipeListen(path string), path being something we can inject during a test. We can further use "/sys/kernel/debug/tracing/trace_pipe" as the default if no args are specified (the real world usage) to alleviate the need for passing in the path for the library user.

[grantseltzer]

Good idea on making that function testable, I'd like to go through a lot of these helpers and write tests, will do I that in a follow up PR (I swear, I will!)

I've gone ahead and renamed this file to example_tracelisten_test.go. The _test suffix is necessary for the example to show up in godoc, but it seems an example_ prefix is idiomatic in the standard library (for example: code and docs).

[itaysk]

the tests in tracee-ebpf/tracee/tracee_test.go are mostly testing these argprinters. I think we can move them here (argparsers_test.go)

also, TracePipeListen doesn't feel like it belongs in the argParsers file, can we move it into another helpers file? if we do that, I think it's ok to rename example_tracelisten_test.go to helpers_test.go (it's common in go to put examples with test right?). wdyt?

[grantseltzer]

@itaysk I don't follow, the only test I see in tracee_test.go is for readArgFromBuff().

I am all for separating out TracePipeListen to its own file and renaming the test file though. I'm about to open a PR with helpers for CO:RE stuff, I'll do that change as part of that PR.

[simar7]

Just left a clarification comment but otherwise 🚢 it!