[RFC] A fallible from_kernel_errno with Result<Error> return #347
Conversation
This file contains 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
This comment has been minimized.
This comment has been minimized.
Currently, from_kernel_errno is an infallible function acting as a constructor for Error. In order to achieve its type invariant, We add a check in it which will prompt a warning and return Error::EINVL when errno given is invalid. While this approach ensures type invariant, it brings great ambiguities. When Error::EINVL is returned, the caller has no way to recognize whether it is a valid errno coming from the kernel or an error issued by the check. This tricky behavior may confuse developers and introduce subtle bugs. Since Error will be used in all respects of the kernel, It's definitely not a sound solution. This RFC proposes that we make from_kernel_errno return a Result<Error>. Thus, we have an explicit, clear, and fallible version of from_kernel_errno by which callers are able to know what really happened behind the scene. And it also provides certain flexibility. We pass the power to callers, they can decide how to deal with invalid `errno` case by case. Signed-off-by: Fox Chen <[email protected]>
foxhlchen
force-pushed
the
fallible-from_kernel_errno
branch
from
d0ac814
to
b11524b
Compare
|
Review of
|
nbdd0121
reviewed
Jun 5, 2021
| @@ -62,21 +62,16 @@ impl Error { | |||
|
|
|||
| /// Creates an [`Error`] from a kernel error code. | |||
| /// | |||
| /// It is a bug to pass an out-of-range `errno`. `EINVAL` would | |||
| /// It is a bug to pass an out-of-range `errno`. Err(`EINVAL`) would | |||
There was a problem hiding this comment.
Suggested change
| /// It is a bug to pass an out-of-range `errno`. Err(`EINVAL`) would | |
| /// It is a bug to pass an out-of-range `errno`. `Err(EINVAL)` would |
ojeda
pushed a commit
that referenced
this pull request
Jun 11, 2024
Add a test case to assert that the skb->pkt_type which was set from the BPF program is retained from the netkit xmit side to the peer's device at tcx ingress location. # ./vmtest.sh -- ./test_progs -t netkit [...] ./test_progs -t netkit [ 1.140780] bpf_testmod: loading out-of-tree module taints kernel. [ 1.141127] bpf_testmod: module verification failed: signature and/or required key missing - tainting kernel [ 1.284601] tsc: Refined TSC clocksource calibration: 3408.006 MHz [ 1.286672] clocksource: tsc: mask: 0xffffffffffffffff max_cycles: 0x311fd9b189d, max_idle_ns: 440795225691 ns [ 1.290384] clocksource: Switched to clocksource tsc #345 tc_netkit_basic:OK #346 tc_netkit_device:OK #347 tc_netkit_multi_links:OK #348 tc_netkit_multi_opts:OK #349 tc_netkit_neigh_links:OK #350 tc_netkit_pkt_type:OK Summary: 6/0 PASSED, 0 SKIPPED, 0 FAILED Signed-off-by: Daniel Borkmann <[email protected]> Link: https://lore.kernel.org/r/[email protected] Signed-off-by: Alexei Starovoitov <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Currently,
from_kernel_errno()is an infallible function acting as a constructor for Error. In order to achieve its type invariant, We add a check in it which will prompt a warning and returnError::EINVLwhenerrnogiven is invalid.While this approach ensures type invariant, it brings great ambiguities. When
Error::EINVLis returned, the caller has no way to recognize whether it is a validerrnocoming from the kernel or an error issued by the check. This tricky behavior may confuse developers and introduce subtle bugs. Since Error will be used in all respects of the kernel, It's definitely nota sound solution.
This RFC proposes that we make
from_kernel_errno()return aResult<Error>. Thus, we have an explicit, clear, and fallible version offrom_kernel_errno()by which callers are able to know what really happened behind the scene. And it also provides certain flexibility. We pass the power to callers, they can decide how to deal with invaliderrnocase by case.Note:
errno(e.g.EBUGorEIKE). This is also viable, however: 1) it needs interaction with upstream 2)from_kernel_errnois still an infallible function that can fail.Reference:
#324 #283
May influence:
#335