gadgets/run: Generalize logic #2012
Conversation
Signed-off-by: Mauricio Vásquez <mauriciov@microsoft.com>
mauriciovasquezbernal
requested review from
burak-ok,
blanquicet and
mqasimsarfraz
as code owners
There was a problem hiding this comment.
I will approve it but the more I read this code the more complex I found it...
Using this printMap only to know which buffer (ring of perf) we need to create is really complex as, in fine, we do not even use this map...
Isn't there an abstraction for these buffers (ring of perf) in cilium/ebpf that we could use?
pkg/gadgets/run/tracer/run.go
Outdated
| if printMap != nil { | ||
| var valueStruct *btf.Struct | ||
| var ok bool | ||
| valueStruct, ok = printMap.Value.(*btf.Struct) | ||
| if !ok { | ||
| return nil, fmt.Errorf("BPF map %q does not have BTF info for values", printMap.Name) | ||
| } | ||
|
|
||
| var valueStruct *btf.Struct | ||
| var ok bool | ||
| valueStruct, ok = m.Value.(*btf.Struct) | ||
| if !ok { | ||
| return nil, fmt.Errorf("BPF map %q does not have BTF info for values", m.Name) | ||
| return valueStruct, nil | ||
| } | ||
|
|
||
| return valueStruct, nil | ||
| return nil, fmt.Errorf("the gadget doesn't provide any compatible way to show information") |
There was a problem hiding this comment.
| if printMap != nil { | |
| var valueStruct *btf.Struct | |
| var ok bool | |
| valueStruct, ok = printMap.Value.(*btf.Struct) | |
| if !ok { | |
| return nil, fmt.Errorf("BPF map %q does not have BTF info for values", printMap.Name) | |
| } | |
| var valueStruct *btf.Struct | |
| var ok bool | |
| valueStruct, ok = m.Value.(*btf.Struct) | |
| if !ok { | |
| return nil, fmt.Errorf("BPF map %q does not have BTF info for values", m.Name) | |
| return valueStruct, nil | |
| } | |
| return valueStruct, nil | |
| return nil, fmt.Errorf("the gadget doesn't provide any compatible way to show information") | |
| if printMap == nil { | |
| return nil, fmt.Errorf("the gadget doesn't provide any compatible way to show information") | |
| } | |
| var valueStruct *btf.Struct | |
| var ok bool | |
| valueStruct, ok = printMap.Value.(*btf.Struct) | |
| if !ok { | |
| return nil, fmt.Errorf("BPF map %q does not have BTF info for values", printMap.Name) | |
| } | |
| return valueStruct, nil |
Do you really need to var? Cannot you use :=?
There was a problem hiding this comment.
Do you really need to var? Cannot you use :=?
Sure, it was a leftover of many reworks before.
Regarding your suggestion, I don't want to use early return on this case, as the logic after will be:
// does the program provide a print map?
printMap := getPrintMap()
if printMap != nil {
return here
}
// is the program an iterator?
// Logic here
// is the program a topper?
// Logic here
// the program isn't any of the above, error out
return nil, fmt.Errorf("the gadget doesn't provide any compatible way to show information")There was a problem hiding this comment.
Regarding your suggestion, I don't want to use early return on this case, as the logic after will be:
I do not understand.
The above function does not handle iterator or topper and the if; else if is done in a later function.
There was a problem hiding this comment.
It'll handle iterators in #1866. Please take into consideration this refactoring is mainly preparing for that PR, originally it was part of that PR but I decided to open a new one to merge is quickly.
Make logic more generic to support other kind of gadgets later on. Signed-off-by: Mauricio Vásquez <mauriciov@microsoft.com>
mauriciovasquezbernal
force-pushed
the
mauricio/cleanup-run-gadget
branch
from
f024810
to
6ad8cdb
Compare
Suggestions are welcome!
I know that we don't use this map yet, but if the code is already there I don't think it's worth removing it.
Unfortunately I don't think there is any, both provide a Read method but it doesn't have the same signature: Record is different for them: Could you take a look after my latest changes? |
This is for the writing part in eBPF code, but I like the API they have in
Maybe we can have a dedicated structure to abstract these two type of buffers. |
Looks good, I'll keep this in mind.
I think there should be a common interface provided by both, but perhaps better to be done in the cilium/ebpf library? |
Make logic more generic to support other kind of gadgets later on
Reviewing #1866 will take longer, so I'm opening this PR with some preparation work.