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

Adopt thiserror for libcgroups #1872

Merged
merged 16 commits into from
May 3, 2023
Merged

Adopt thiserror for libcgroups #1872

merged 16 commits into from
May 3, 2023

Conversation

squili
Copy link
Contributor

@squili squili commented May 2, 2023

As prescribed in #1377

@codecov-commenter
Copy link

codecov-commenter commented May 2, 2023

Codecov Report

Merging #1872 (5d01bed) into main (7d9d091) will decrease coverage by 0.77%.
The diff coverage is 40.85%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1872      +/-   ##
==========================================
- Coverage   68.89%   68.13%   -0.77%     
==========================================
  Files         122      122              
  Lines       13717    14011     +294     
==========================================
+ Hits         9450     9546      +96     
- Misses       4267     4465     +198     

@squili squili marked this pull request as ready for review May 2, 2023 12:18
@squili squili changed the title Adopt thiserror Adopt thiserror for libcgroups May 2, 2023
Copy link
Collaborator

@yihuaf yihuaf left a comment

Choose a reason for hiding this comment

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

LGTM

/// Parses a file that is structed according to the nested keyed format
/// # Example
/// ```
/// use libcgroups::stats::parse_device_number;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do we need to keep the example here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The example was triggering doctets, which then failed because the function was made crate-only

#[error("failed to read {path}: {err}")]
Read { err: std::io::Error, path: PathBuf },
#[error("failed to create dir {path}: {err}")]
CreateDir { err: std::io::Error, path: PathBuf },
Copy link
Member

Choose a reason for hiding this comment

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

I love this WrappedIoError idea ❤️
If the dirs cannot be created in the libcgroup crates, does this result in the failure to create a new cgroup?
I believe it is more user-friendly to display a message such as "failed to create the cgroup" or something similar. WDYT?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's what errors higher up in the chain are for - they can be added to the higher-level error adts with more user-friendly messages

@@ -1,6 +1,6 @@
use std::fmt::Display;

#[derive(Hash, PartialEq, Eq, Debug, Clone)]
#[derive(Hash, PartialEq, Eq, Debug, Clone, Copy)]
Copy link
Member

@utam0k utam0k May 3, 2023

Choose a reason for hiding this comment

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

Why do we need Copy?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This enum is essentially just a fancy number, so the performance of passing by reference and copying are the same

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Specifically, this allows us to do *controller_type instead of controller_type.clone() to turn a reference into an owned value

@utam0k
Copy link
Member

utam0k commented May 3, 2023

It looks like that lint said the error. Please check them 🙏

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.

💯
I can't believe that you make this PR in such a short period of time 😲

@utam0k utam0k merged commit ae408ad into youki-dev:main May 3, 2023
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