API, control, netstack: error refactoring

[nrc]

An attempt at refactoring some of our errors into a more handling-oriented approach. I'm a bit unsure about some of the names, descriptions, and categorisations, so feedback on that stuff in particular (as well as the rest of the approach) would be welcome!

[npry]

Directionally I'm on board, I think the current shape has some awkwardness around the specific composition of structs and enums (mostly in ts_control). The main hangup is that I'm not sure why we're nesting enum-in-enum there. As I mention in one of my comments, it feels to me like it wants to be a struct Error with a flat ErrorKind enum, so I'm curious why you laid it out that way

[nrc]

Directionally I'm on board, I think the current shape has some awkwardness around the specific composition of structs and enums (mostly in ts_control).

Yeah, a few places did feel pretty awkward and I definitely think this can be improved.

The main hangup is that I'm not sure why we're nesting enum-in-enum there. As I mention in one of my comments, it feels to me like it wants to be a struct Error with a flat ErrorKind enum, so I'm curious why you laid it out that way

So the idea here is that the top-level variants for each variant reflect possible recovery strategies - e.g., in ts_control, MachineNotAuthorized is lifted out because there is a specific user action to fix it, Protocol is meant to be an 'expected' error, where the user might need to re-check some parameters and then retry, and Internal is something that can't reasonably be recovered from (i.e., the user could maybe retry once, then report to the end-user and abort). Within those, ProtocolPhase is meant to further narrow down the issue for better error recovery, and ErrorKind and ConnectionPhase are meant to be purely informational so that the error that might be reported or logged can be more informative than just 'Internal error: internal error'.

This could all be better documented! And some of the names can definitely be improved.

Hopefully that makes sense why there are nested enums and not a struct.

[nrc]

Latest commit makes some of the suggested changes. I haven't updated docs to correspond or applied suggestions which I'd like to discuss further.

[nrc]

Updated the PR following our chat last week. The changes are in a separate commit. I think I got everything we discussed (and comments here), but let me know if I missed anything. I'm much happier with the new errors.

There is one question inline (marked REVIEW) about some IO errors which I didn't know how to handle (either because they are likely to be network errors but not always or are network errors but retrying won't help (I think)).

There is also one question in a reply above because of my bad memory.

[npry]

bunch of small nits, but the blocker is that imo NetworkError in its various incarnations probably want to split along the lines of issues <= L4 and > L4, i.e. is the network itself down, or is this a higher-layer problem? the former can be handled by retries or possibly out-of-band online checks, where an http 400 or a tls negotiation failure can't be resolved the same way (not retryable). probably this latter case can be handled as an Internal variant for now?

edit: otherwise, lgtm!

[nrc]

@npry all comments addressed. Thank you for helping me iterate on this!