Enforce OS errors are in the allowed range. #441
Conversation
src/error.rs
Outdated
| @@ -65,6 +65,19 @@ impl Error { | |||
| /// custom errors. | |||
| pub const CUSTOM_START: u32 = (1 << 31) + (1 << 30); | |||
|
|
|||
| #[cold] | |||
| pub(super) fn new_os(code: u32) -> Self { | |||
There was a problem hiding this comment.
In the future we might consider adding a public new_custom constructor and then deprecating the From<NonZeroU32> that does the non-range-checked conversion.
briansmith
force-pushed
the
b/new_os
branch
3 times, most recently
from
53d8f46
to
0af5ac5
Compare
|
I updated this to be cleaner. I also removed the |
|
What about my suggestion above? |
Sorry, I don't see it. Maybe it is still a draft? |
|
Oh, oops. You are right. |
src/error.rs
Outdated
| @@ -65,6 +65,20 @@ impl Error { | |||
| /// custom errors. | |||
| pub const CUSTOM_START: u32 = (1 << 31) + (1 << 30); | |||
|
|
|||
| #[allow(dead_code)] | |||
| #[cold] | |||
| pub(super) fn new_os(code: u32) -> Self { | |||
There was a problem hiding this comment.
Could you add some docs for this method?
I also think we can write it like this:
pub(crate) fn from_os_code(code: impl TryInto<u32>) -> Self {
code.try_into()
.ok()
.and_then(NonZeroU32::new)
.and_then(|code| {
if code.get() < Self::INTERNAL_START {
Some(Self(code))
} else {
None
}
})
.unwrap_or(Self::UNEXPECTED)
}This will make backend code a bit nicer.
I don't think we need to distinguish between ERRNO_NOT_POSITIVE and UNEXPECTED. Both cases should never happen in practice in the first place. Plus the former was used only for codes from errno.
There was a problem hiding this comment.
Could you add some docs for this method?
OK. But...
pub(crate) fn from_os_code(code: impl TryInto) -> Self {
Unfortunately that would allow passing a usize or similar, which really shouldn't be implicitly converted. So I think we should not do that.
I don't think we need to distinguish between ERRNO_NOT_POSITIVE and UNEXPECTED
I am OK with making that change.
There was a problem hiding this comment.
Unfortunately that would allow passing a usize or similar, which really shouldn't be implicitly converted.
Since it's a private API, I think it's fine to allow this. If you want to make this more explicit, you could use Error::from_os_code::<isize>(code) at call sites. Either way, I do not insist on it.
There was a problem hiding this comment.
Well, I think the main goal here is to ensure that the result is well-defined, rather than to increase convenience.
I do hope this API can be improved further and then made public to replace the From<NonZeroU32> implementation as discussed elsewhere.
There was a problem hiding this comment.
I added the documentation, moved the ERRNO_NOT_POSITIVE logic back to where it belongs, and renamed the function to be more like the analogous std::io::Error function.
briansmith
force-pushed
the
b/new_os
branch
2 times, most recently
from
5391cbc
to
52bcd6d
Compare
Avoid the `From<NonZeroU32>` implementation in favor of a constructor that centralizes all the range checking in one place. Consistently use `Self::UNEXPECTED` for out-of-rnage values. Besides being more consistent in the range checking, this also reduces the boilerplate in callers, which makes it easier to maintain the ports to less-common operating systems. This is a step towards deprecating the `impl From<NonZeroU32> for Error` implementation.
Avoid the
From<NonZeroU32>implementation in favor of a constructor that centralizes all the range checking in one place. Consistently useERRNO_NOT_POSITIVEfor nonpositive values andSelf::UNEXPECTEDfor too-large values.Besides being more consistent in the range checking, this also reduces the boilerplate in callers, which makes it easier to maintain the ports to less-common operating systems.