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.

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

Implement thiserror for libcontainer - Part 1 #1876

Merged
merged 7 commits into from
May 4, 2023

Conversation

yihuaf
Copy link
Collaborator

@yihuaf yihuaf commented May 3, 2023

This is a first attempt to add thiserror to libcontainer and all comments are welcome. Note, I decide to break this effort into multiple parts because a single PR would be too large to reasonably review. The aim is to start at the bottom turning the errors into thiserror while keeping the top level anyhow::Error until we finish this effort. By keeping the top level as anyhow::Error for the moment, we can easily turn thiserror into an anyhow::Error, but not vice versa.

A few notes. I tried my best to create appropriate error, but they are not perfect in any means. I would like in this PR to focus on the broader effort to make the transition, instead of focusing on the specifics of a single error. We can always make additional PR to fix.

I am not sure if I am using the libcgroups error correctly. For now, I just turned them into string messages. We may need to spend more effort since it currently only returns a std::error::Error + 'static trait, rather than a concrete type where to can expose structurally.

yihuaf added 7 commits May 3, 2023 16:59
Signed-off-by: yihuaf <yihuaf@unkies.org>
Signed-off-by: yihuaf <yihuaf@unkies.org>
Signed-off-by: yihuaf <yihuaf@unkies.org>
… etc.

Signed-off-by: yihuaf <yihuaf@unkies.org>
Signed-off-by: yihuaf <yihuaf@unkies.org>
Signed-off-by: yihuaf <yihuaf@unkies.org>
Signed-off-by: yihuaf <yihuaf@unkies.org>
@codecov-commenter
Copy link

codecov-commenter commented May 3, 2023

Codecov Report

Merging #1876 (17537e9) into main (ae408ad) will decrease coverage by 0.46%.
The diff coverage is 41.24%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1876      +/-   ##
==========================================
- Coverage   68.16%   67.70%   -0.46%     
==========================================
  Files         122      124       +2     
  Lines       14011    14205     +194     
==========================================
+ Hits         9550     9618      +68     
- Misses       4461     4587     +126     

@yihuaf yihuaf requested review from utam0k and a team May 3, 2023 18:31
@yihuaf yihuaf self-assigned this May 3, 2023
@@ -8,3 +8,109 @@ pub mod syscall;
pub mod test;

pub use syscall::Syscall;
#[derive(Debug, thiserror::Error)]
pub enum SyscallError {
Copy link
Member

Choose a reason for hiding this comment

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

👍

@utam0k
Copy link
Member

utam0k commented May 4, 2023

@squili May I ask you to review this PR?

Copy link
Contributor

@squili squili left a comment

Choose a reason for hiding this comment

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

This is great! I love how you structured the error ADTs!

@utam0k
Copy link
Member

utam0k commented May 4, 2023

Thanks @squili

@utam0k utam0k merged commit b51cf1a into containers:main May 4, 2023
@yihuaf yihuaf deleted the yihuaf/thiserror branch May 4, 2023 06:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants