refactor: ignore other return values when an error occurs - #5369
Conversation
we shouldn't trust `waitKill` when we got the error `EINVAL`. Suggested-by: Sebastiaan van Stijn <github@gone.nl> Co-authored-by: Sebastiaan van Stijn <github@gone.nl> Signed-off-by: lifubang <lifubang@acmcoder.com>
| if waitKill, err := filter.GetWaitKill(); err != nil { | ||
| if !errors.Is(err, unix.EINVAL) { | ||
| return 0, false, fmt.Errorf("unable to fetch SECCOMP_FILTER_FLAG_WAIT_KILLABLE_RECV bit: %w", err) | ||
| } | ||
| } else if waitKill { | ||
| flags |= uint(C.C_FILTER_FLAG_WAIT_KILLABLE_RECV) | ||
| } |
There was a problem hiding this comment.
If we really want to switch it over, I think better describes what we are doing, but I don't really mind too much either way.
| if waitKill, err := filter.GetWaitKill(); err != nil { | |
| if !errors.Is(err, unix.EINVAL) { | |
| return 0, false, fmt.Errorf("unable to fetch SECCOMP_FILTER_FLAG_WAIT_KILLABLE_RECV bit: %w", err) | |
| } | |
| } else if waitKill { | |
| flags |= uint(C.C_FILTER_FLAG_WAIT_KILLABLE_RECV) | |
| } | |
| waitKill, err := filter.GetWaitKill() | |
| if errors.Is(err, unix.EINVAL) { | |
| waitKill, err = false, nil | |
| } | |
| if err != nil { | |
| return 0, false, fmt.Errorf("unable to fetch SECCOMP_FILTER_FLAG_WAIT_KILLABLE_RECV bit: %w", err) | |
| } | |
| if waitKill { | |
| flags |= uint(C.C_FILTER_FLAG_WAIT_KILLABLE_RECV) | |
| } |
|
First of all I want to find out why we're getting EINVAL at all. See #5347 (comment) |
Proposed libseccomp-golang fix: seccomp/libseccomp-golang#127 With that, we won't have to add a special case for EINVAL |
rata
left a comment
rata
left a comment
There was a problem hiding this comment.
I think it's fine this or what @cyphar proposes. But the problem of returning EINVAL doesn't affect theoretically the other options too?
What about making this a helper function and calling it for each option we need. It could make filterFlags() simpler to read, and whenever we add a new filter flag, we will handle it correctly by using the helper.
What do you think?
Oh, I missed that. If the PR to handle that is merged, then it might be fine to just do this here. Sorry :-D |
4 participants
we shouldn't trust
waitKillwhen we got the errorEINVAL.Ref:
#5367 (comment)
#5367 (comment)