Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix some more error handling #163

Merged
merged 2 commits into from
May 6, 2022
Merged

Fix some more error handling #163

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
29b078d
Run gofmt
javierhonduco May 4, 2022
20c5ee9
Fix more error handling
javierhonduco May 4, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 0 additions & 1 deletion helpers/btfhub.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,2 +1 @@
package helpers

75 changes: 36 additions & 39 deletions libbpfgo.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -566,30 +566,30 @@ func (b *BPFMap) SetType(mapType MapType) error {

func (b *BPFMap) Pin(pinPath string) error {
path := C.CString(pinPath)
errC := C.bpf_map__pin(b.bpfMap, path)
ret, errC := C.bpf_map__pin(b.bpfMap, path)
C.free(unsafe.Pointer(path))
if errC != 0 {
return fmt.Errorf("failed to pin map %s to path %s: %w", b.name, pinPath, syscall.Errno(-errC))
if ret != 0 {
return fmt.Errorf("failed to pin map %s to path %s: %w", b.name, pinPath, errC)
}
return nil
}

func (b *BPFMap) Unpin(pinPath string) error {
path := C.CString(pinPath)
errC := C.bpf_map__unpin(b.bpfMap, path)
ret, errC := C.bpf_map__unpin(b.bpfMap, path)
C.free(unsafe.Pointer(path))
if errC != 0 {
return fmt.Errorf("failed to unpin map %s from path %s: %w", b.name, pinPath, syscall.Errno(-errC))
if ret != 0 {
return fmt.Errorf("failed to unpin map %s from path %s: %w", b.name, pinPath, errC)
}
return nil
}

func (b *BPFMap) SetPinPath(pinPath string) error {
path := C.CString(pinPath)
errC := C.bpf_map__set_pin_path(b.bpfMap, path)
ret, errC := C.bpf_map__set_pin_path(b.bpfMap, path)
C.free(unsafe.Pointer(path))
if errC != 0 {
return fmt.Errorf("failed to set pin for map %s to path %s: %w", b.name, pinPath, syscall.Errno(-errC))
if ret != 0 {
return fmt.Errorf("failed to set pin for map %s to path %s: %w", b.name, pinPath, errC)
}
return nil
}
Expand All @@ -600,9 +600,9 @@ func (b *BPFMap) SetPinPath(pinPath string) error {
// Note: for ring buffer and perf buffer, maxEntries is the
// capacity in bytes.
func (b *BPFMap) Resize(maxEntries uint32) error {
errC := C.bpf_map__set_max_entries(b.bpfMap, C.uint(maxEntries))
if errC != 0 {
return fmt.Errorf("failed to resize map %s to %v: %w", b.name, maxEntries, syscall.Errno(-errC))
ret, errC := C.bpf_map__set_max_entries(b.bpfMap, C.uint(maxEntries))
if ret != 0 {
return fmt.Errorf("failed to resize map %s to %v: %w", b.name, maxEntries, errC)
Copy link
Contributor

@grantseltzer grantseltzer May 4, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why don't we want to use syscall.Errno(-errC)? In these cases you're using %w for errC, which is an int.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The second argument's type of any CGO call is error, which is the errno already.

It's confusing because Go doesn't require us to bind all the return values of a function, and before, as errC was the sole function return value we were reading, it was in reality the return value, which is a C.int, and not what we want.

That's why passing it to syscall.Errno always resulted in -1, EPERM(#147) [1]

[1]: For completeness, it returns -1on failure before libbpf 1.0, which hasn't been released yet, or if LibbpfStrictModeDirectErrs is not used (https://github.com/libbpf/libbpf/wiki/Libbpf-1.0-migration-guide).

Hope this makes sense, let me know if this is not what you meant!

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah yes of course, my confusion. I'd like to standardize naming of CGO errors that are int's as errorCode and the second return of type error as errno. I can handle that in the future.

}
return nil
}
Expand Down Expand Up @@ -634,10 +634,7 @@ func (b *BPFMap) GetPinPath() string {

func (b *BPFMap) IsPinned() bool {
isPinned := C.bpf_map__is_pinned(b.bpfMap)
if isPinned == C.bool(true) {
return true
}
return false
return isPinned == C.bool(true)
}

func (b *BPFMap) KeySize() int {
Expand All @@ -660,9 +657,9 @@ func (b *BPFMap) GetValue(key unsafe.Pointer) ([]byte, error) {
value := make([]byte, b.ValueSize())
valuePtr := unsafe.Pointer(&value[0])

errC := C.bpf_map_lookup_elem(b.fd, key, valuePtr)
if errC != 0 {
return nil, fmt.Errorf("failed to lookup value %v in map %s: %w", key, b.name, syscall.Errno(-errC))
ret, errC := C.bpf_map_lookup_elem(b.fd, key, valuePtr)
if ret != 0 {
return nil, fmt.Errorf("failed to lookup value %v in map %s: %w", key, b.name, errC)
}
return value, nil
}
Expand Down Expand Up @@ -890,9 +887,9 @@ func (b *BPFMap) DeleteKey(key unsafe.Pointer) error {
// bpfmap.Update(keyPtr, valuePtr)
//
func (b *BPFMap) Update(key, value unsafe.Pointer) error {
errC := C.bpf_map_update_elem(b.fd, key, value, C.BPF_ANY)
if errC != 0 {
return fmt.Errorf("failed to update map %s: %w", b.name, syscall.Errno(-errC))
ret, errC := C.bpf_map_update_elem(b.fd, key, value, C.BPF_ANY)
if ret != 0 {
return fmt.Errorf("failed to update map %s: %w", b.name, errC)
}
return nil
}
Expand Down Expand Up @@ -953,10 +950,10 @@ func (it *BPFMapIterator) Err() error {

func (m *Module) GetProgram(progName string) (*BPFProg, error) {
cs := C.CString(progName)
prog, errno := C.bpf_object__find_program_by_name(m.obj, cs)
prog, errC := C.bpf_object__find_program_by_name(m.obj, cs)
C.free(unsafe.Pointer(cs))
if prog == nil {
return nil, fmt.Errorf("failed to find BPF program %s: %w", progName, errno)
return nil, fmt.Errorf("failed to find BPF program %s: %w", progName, errC)
}

return &BPFProg{
Expand All @@ -982,21 +979,21 @@ func (p *BPFProg) Pin(path string) error {
}

cs := C.CString(absPath)
errC := C.bpf_program__pin(p.prog, cs)
ret, errC := C.bpf_program__pin(p.prog, cs)
C.free(unsafe.Pointer(cs))
if errC != 0 {
return fmt.Errorf("failed to pin program %s to %s: %w", p.name, path, syscall.Errno(-errC))
if ret != 0 {
return fmt.Errorf("failed to pin program %s to %s: %w", p.name, path, errC)
}
p.pinnedPath = absPath
return nil
}

func (p *BPFProg) Unpin(path string) error {
cs := C.CString(path)
errC := C.bpf_program__unpin(p.prog, cs)
ret, errC := C.bpf_program__unpin(p.prog, cs)
C.free(unsafe.Pointer(cs))
if errC != 0 {
return fmt.Errorf("failed to unpin program %s to %s: %w", p.name, path, syscall.Errno(-errC))
if ret != 0 {
return fmt.Errorf("failed to unpin program %s to %s: %w", p.name, path, errC)
}
p.pinnedPath = ""
return nil
Expand Down Expand Up @@ -1148,17 +1145,17 @@ func (p *BPFProg) GetType() BPFProgType {

func (p *BPFProg) SetAutoload(autoload bool) error {
cbool := C.bool(autoload)
errC := C.bpf_program__set_autoload(p.prog, cbool)
if errC != 0 {
return fmt.Errorf("failed to set bpf program autoload: %w", syscall.Errno(-errC))
ret, errC := C.bpf_program__set_autoload(p.prog, cbool)
if ret != 0 {
return fmt.Errorf("failed to set bpf program autoload: %w", errC)
}
return nil
}

func (p *BPFProg) SetTracepoint() error {
errC := C.bpf_program__set_tracepoint(p.prog)
if errC != 0 {
return fmt.Errorf("failed to set bpf program as tracepoint: %w", syscall.Errno(-errC))
ret, errC := C.bpf_program__set_tracepoint(p.prog)
if ret != 0 {
return fmt.Errorf("failed to set bpf program as tracepoint: %w", errC)
}
return nil
}
Expand All @@ -1184,10 +1181,10 @@ func (p *BPFProg) AttachGeneric() (*BPFLink, error) {
// the BPF program to. To attach to a kernel function specify attachProgFD as 0
func (p *BPFProg) SetAttachTarget(attachProgFD int, attachFuncName string) error {
cs := C.CString(attachFuncName)
errC := C.bpf_program__set_attach_target(p.prog, C.int(attachProgFD), cs)
ret, errC := C.bpf_program__set_attach_target(p.prog, C.int(attachProgFD), cs)
C.free(unsafe.Pointer(cs))
if errC != 0 {
return fmt.Errorf("failed to set attach target for program %s %s %w", p.name, attachFuncName, syscall.Errno(-errC))
if ret != 0 {
return fmt.Errorf("failed to set attach target for program %s %s %w", p.name, attachFuncName, errC)
}
return nil
}
Expand Down
2 changes: 1 addition & 1 deletion libbpfgo_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,7 +68,7 @@ func Test_LoadAndAttach(t *testing.T) {
},
},
{
prog: "socket_connect",
prog: "socket_connect",
attachFn: func(prog *BPFProg, name string) (*BPFLink, error) {
if name != "" {
// to make the check for attaching with "foo" happy
Expand Down