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

tokio-boring: Add additional accessors to HandshakeError #57

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

tokio-boring: Add additional accessors to HandshakeError #57

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
d333fdb
tokio-boring: Add additional accessors to `HandshakeError`
hawkw Nov 4, 2021
986a8ca
return `Result`s from `into_$ERROR` methods
hawkw Nov 5, 2021
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
34 changes: 34 additions & 0 deletions tokio-boring/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -307,6 +307,40 @@ impl<S> HandshakeError<S> {
_ => None,
}
}

/// Returns a reference to the inner SSL error, if this error was caused by
/// a SSL error.
pub fn as_ssl_error(&self) -> Option<&boring::error::ErrorStack> {
match &self.0 {
ssl::HandshakeError::Failure(s) => s.error().ssl_error(),
ssl::HandshakeError::SetupFailure(ref s) => Some(s),
_ => None,
Comment on lines +315 to +317
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't this also handle HandshakeError::WouldBlock? Or is it impossible somehow for WouldBlock to be an SSL error?

Suggested change
ssl::HandshakeError::Failure(s) => s.error().ssl_error(),
ssl::HandshakeError::SetupFailure(ref s) => Some(s),
_ => None,
ssl::HandshakeError::Failure(s) | ssl::HandshakeError::WouldBlock(s) => s.error().ssl_error(),
ssl::HandshakeError::SetupFailure(ref s) => Some(s),

}
}

/// Consumes `self` and returns the inner I/O error by value, if this error
/// was caused by an I/O error.
pub fn into_io_error(self) -> Option<io::Error> {
Copy link
Contributor

Choose a reason for hiding this comment

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

FWIW I'm not sure that this is valuable for us... If it's not an Io error we lose the original error?

I guess we'd have to use something like if e.as_io_error().is_some() { e.into_io_error.unwrap() { ? that's pretty cumbersome

Copy link
Contributor Author

@hawkw hawkw Nov 4, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

I thought about adding an is_io/is_ssl, but I wasn't sure if it was necessary given that it would just be as_io_error().is_some(). I could add that as well, though. IMO it's still less cumbersome than the current code we have, which calls as_io_error and creates a new io::Error using values from the one that's passed by reference.

Unfortunately, we can't return Results rather than Options so that the original error can be reused if it couldn't be converted into an io::Error, because we have to call MidHandshakeSslStream::into_error, and (since MidHandshakeSslStream can only be constructed by the boring crate, not in tokio-boring), we can't ever get the MidHandshakeSslStream back...so we can't return the original Self type in this case.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@olix0r okay, I've modified these to return Result<io::Error, Self> and Result<ErrorStack, Self> so you can now write code like

error
    .into_ssl_error()
    .map(Into::into)
    .or_else(|error| error.into_io_error().map(Into::into))

or similar

Copy link
Contributor

Choose a reason for hiding this comment

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

awesome!

match self.0 {
ssl::HandshakeError::Failure(s) => s.into_error().into_io_error().ok(),
_ => None,
}
}

/// Consumes `self` and returns the inner SSL error by value, if this error
/// was caused by n SSL error.
pub fn into_ssl_error(self) -> Option<boring::error::ErrorStack> {
match self.0 {
// TODO(eliza): it's not great that we have to clone in this case,
// but `boring::ssl::Error` doesn't currently expose an
// `into_ssl_error` the way it does for `io::Error`s. We could add
// that in a separate PR, but adding that here would make
// `tokio-boring` depend on a new release of `boring`...
ssl::HandshakeError::Failure(s) => s.into_error().ssl_error().cloned(),
ssl::HandshakeError::SetupFailure(stack) => Some(stack),
_ => None,
}
}
}

impl<S> fmt::Debug for HandshakeError<S>
Expand Down