perf: clean up error handling and bare fds#1444
Merged
lmb merged 1 commit intocilium:mainfrom Apr 23, 2024
Merged
Conversation
The perf reader was written before we had the sys.FD abstraction
and therefore still uses plain int to keep track of fds. This means
we don't get leak tracking during CI. It also relies on a subtle
interaction between defer and named return values which is easy to
break by accidentally shadowing err:
func ... (err error) {
poller, err := poller.New(...)
defer func() {
if err != nil {} // Refers to the second err
}()
}
Rework the code to use sys.FD and the closeOnError idiom used
elsewhere in the library.
Signed-off-by: Lorenz Bauer <[email protected]>
Contributor
Author
|
@florianl your intuition was right that the error handling path in the perf reader is wonky. It's really old code by now, I think this should be better. |
florianl
approved these changes
Apr 23, 2024
Contributor
florianl
left a comment
florianl
left a comment
There was a problem hiding this comment.
Thanks for the cleanup! I think it is easier now to follow.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The perf reader was written before we had the sys.FD abstraction and therefore still uses plain int to keep track of fds. This means we don't get leak tracking during CI. It also relies on a subtle interaction between defer and named return values which is easy to break by accidentally shadowing err:
Rework the code to use sys.FD and the closeOnError idiom used elsewhere in the library.