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

Implemented thiserror for libcontainer - Part 4 #1912

Merged
merged 10 commits into from
May 15, 2023

Conversation

yihuaf
Copy link
Collaborator

@yihuaf yihuaf commented May 14, 2023

This is the second to last PR. I converted most of the module in crate libcontainer to use thiserror. The missing 2 are container and workload. Both of these 2 requires a bit more thinking. The workload requires us to define the Executor type with an Error associated type. The container crate will be the main interface exposed by the libcontainer crate. It is easier to review these two changes in a different PR.

Note, some of the errors are lumped into a other error. Instead, we use tracing to log the context of the error at the source where the error happens. This is a contrast from previously using anyhow where the error context are stored in the error itself and logged at the top level.

Other tag along:

  • Moved libseccomp related code in the process mod into a seccomp_listener mod, so we can centralize the libseccomp feature related code into a single place. Previously, we have to litter libseccomp feature switch all over the place.
  • Moved some functions from utils if they are not shared by multiple module. This can simplify error handling so we don't have to create error in utils just for a single function.

@codecov-commenter
Copy link

codecov-commenter commented May 14, 2023

Codecov Report

Merging #1912 (171b3c2) into main (91b476a) will decrease coverage by 1.90%.
The diff coverage is 36.78%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1912      +/-   ##
==========================================
- Coverage   66.96%   65.06%   -1.90%     
==========================================
  Files         126      127       +1     
  Lines       14164    14637     +473     
==========================================
+ Hits         9485     9524      +39     
- Misses       4679     5113     +434     

@yihuaf yihuaf requested review from utam0k and a team May 14, 2023 19:21
@yihuaf yihuaf self-assigned this May 14, 2023
Signed-off-by: yihuaf <[email protected]>
@yihuaf yihuaf force-pushed the yihuaf/thiserror branch 2 times, most recently from 164b9f5 to e986826 Compare May 15, 2023 03:09
Signed-off-by: yihuaf <[email protected]>
Copy link
Member

@utam0k utam0k left a comment

Choose a reason for hiding this comment

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

👍 👍 👍

@utam0k utam0k merged commit 9ebebc2 into youki-dev:main May 15, 2023
@yihuaf yihuaf deleted the yihuaf/thiserror branch June 13, 2023 20:19
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.

3 participants