rustls / rustls-ffi

Use Rustls from any language
Other
124 stars 31 forks source link

Breaking changes in upstream error types. #297

Closed cpu closed 1 year ago

cpu commented 1 year ago

There were several upstream breaking API changes in Rustls related to error types:

The upstream changes will have to be adapted to error.rs and client.rs.

cpu commented 1 year ago

@jsha Do you have any thoughts on how the rustls_result enum should adapt to the upstream changes: https://github.com/rustls/rustls-ffi/blob/55df12216f0004af12c5967725a0b46d9c4bd466/src/error.rs#L137-L152

For e.g. CertInvalidEncoding = 7117. The upstream InvalidCertificateEncoding entry in the Error enum this was being mapped to previously was removed and now we have InvalidCertificate(x) where x is something more specific like CertificateError::BadEncoding.

A couple questions I have:

I'm trying to implement some of these changes without having had time to do a lot of reading of the existing code, or the git blame. Any guidance is welcome :-)

jsha commented 1 year ago

Should all new additions use distinct u32 values not previously used, leaving gaps for the removed types?

Yes, please.

Should there be a u32 per more specific CertificateError variant, or just for the top level InvalidCertificate?

Yes. The error design here is more-or-less "combinatorial explosion," since I looked at the error enum in rustls at the time and decided that splitting out errors for sub-variants was reasonable in scope.

For the Other sub-variant, we will have to drop the Arc<_> and any information in it, which is a bummer but the best approach for now.

I think sometime soonish we may want to rework errors in the FFI binding to accommodate more complex error types like Other and General without dropping useful information on the floor. But that can come after the next release; it will be a significant change.

cpu commented 1 year ago

I just noticed there was another PR that touched error content I missed in my original scan: https://github.com/rustls/rustls/pull/1198 Added to the list in the PR desc.

I'm almost done adapting all the changes. I might run out of time today before I have a cleaned up PR ready for review but if that happens I should have something to share early tomorrow.

cpu commented 1 year ago

Fixed with #303