Move `ErrorKind`s into `error.rs` in their corresponding crates

[simonsan]

Currently ErrorKinds are all over the place, defined where they are needed. This isn't great for visibility and also importing can get quite tedious, as it's not clear on first sight what error types a crate exposes. Which usually (AFAIK) is being done by looking into error.rs.

Another thing to think about is to use one opaque error type and implement something like an ErrorMarker to be able to convert into that opaque error type:

// PublicError is public, but opaque and easy to keep compatible.
#[derive(Error, Debug)]
#[error(transparent)]
pub struct PublicError(#[from] ErrorRepr);

impl PublicError {
    // Accessors for anything we do want to expose publicly.
}

// Private and free to change across minor version of the crate.
#[derive(Error, Debug)]
enum ErrorRepr {
    ...
}

[yihuaf]

We don't use ErrorKind except std::io::ErrorKind.

[simonsan]

We don't use ErrorKind except std::io::ErrorKind.

Please reopen, ErrorKind was paraphrasing different ErrorKinds all over the place like https://github.com/containers/youki/blob/main/crates/libcgroups/src/stats.rs#L200

Other examples: https://github.com/containers/youki/blob/dfe6ee80bc780449fffcf22ecf6618c7c30791ae/crates/libcgroups/src/v2/util.rs#L12

https://github.com/containers/youki/blob/dfe6ee80bc780449fffcf22ecf6618c7c30791ae/crates/libcgroups/src/stub/v1/manager.rs#L3

https://github.com/containers/youki/blob/dfe6ee80bc780449fffcf22ecf6618c7c30791ae/crates/libcgroups/src/v2/cpu.rs#L26

https://github.com/containers/youki/blob/dfe6ee80bc780449fffcf22ecf6618c7c30791ae/crates/libcgroups/src/v2/devices/program.rs#L11

https://github.com/containers/youki/blob/dfe6ee80bc780449fffcf22ecf6618c7c30791ae/crates/libcontainer/src/syscall/mod.rs#L11

https://github.com/containers/youki/blob/dfe6ee80bc780449fffcf22ecf6618c7c30791ae/crates/libcgroups/src/v1/cpuacct.rs#L37

[yihuaf]

I see. I was confused with the ErrorKind mention. Apologize.

Would you like to take a shot and draft a PR for this?

[simonsan]

I see. I was confused with the ErrorKind mention. Apologize.

Would you like to take a shot and draft a PR for this?

Yes, can do. I think it should be decided beforehand though, if an opaque error type would be needed or if we just move all the errors into error.rs?

[yihuaf]

@simonsan Honestly, we don't have a good idea on this yet. We just recently migrated our error definitions from anyhow to structured errors using thiserror. We have not thought through all implications of proper error interface yet.

Can you point me to some readings or examples on opaque error type? Especially, if you can help us understand the trade offs between different approaches? Thanks.

[simonsan]

Something like this could be helpful: https://web.archive.org/web/20230128000646/http://www.tglman.com/posts/rust_lib_error_management.html

I think as a second step to the refactoring to thiserror we should collect the available error kinds in a central place within each crate (error.rs). So we get an overview.

From there on we can check what is available and which approaches would make sense. It's hard to say at this point to be honest, as I don't have an overview.

[zahash]

@yihuaf is this issue still open? if yes, can i be assigned? i do think having a error.rs in each crate does improve discoverability. but then again, defining errors closer to where they are actually used instead of a single error.rs file is also a good idea.

[utam0k]

@yihuaf friendly remind ;) If you find time to address, feel free to let me know.