-
Notifications
You must be signed in to change notification settings - Fork 8
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
Replace enumflags2 with bitflags #23
base: main
Are you sure you want to change the base?
Conversation
By removing thiserror I can reduce the compile time to ~2s with most of the time taken by compiling libc (~1.5s). It does require manually writing a lot of boilerplate previously written by thiserror though, so I'm not sure if it is worth removing thiserror. In any case the diff required is this: diff --git a/Cargo.toml b/Cargo.toml
index 22d98db..966cddd 100644
--- a/Cargo.toml
+++ b/Cargo.toml
@@ -17,11 +17,9 @@ readme = "README.md"
[dependencies]
bitflags = "1.3.2"
libc = "0.2.133"
-thiserror = "1.0"
[dev-dependencies]
anyhow = "1.0"
-landlock = { path = "." }
lazy_static = "1"
strum = "0.24"
strum_macros = "0.24"
diff --git a/src/errors.rs b/src/errors.rs
index 3047c3b..56f8da5 100644
--- a/src/errors.rs
+++ b/src/errors.rs
@@ -1,20 +1,62 @@
use crate::{Access, AccessFs};
-use std::io;
+use std::error::Error;
use std::path::PathBuf;
-use thiserror::Error;
+use std::{fmt, io};
/// Maps to all errors that can be returned by a ruleset action.
-#[derive(Debug, Error)]
+#[derive(Debug)]
#[non_exhaustive]
pub enum RulesetError {
- #[error(transparent)]
- HandleAccesses(#[from] HandleAccessesError),
- #[error(transparent)]
- CreateRuleset(#[from] CreateRulesetError),
- #[error(transparent)]
- AddRules(#[from] AddRulesError),
- #[error(transparent)]
- RestrictSelf(#[from] RestrictSelfError),
+ HandleAccesses(HandleAccessesError),
+ CreateRuleset(CreateRulesetError),
+ AddRules(AddRulesError),
+ RestrictSelf(RestrictSelfError),
+}
+
+impl Error for RulesetError {
+ fn source(&self) -> Option<&(dyn Error + 'static)> {
+ match self {
+ RulesetError::HandleAccesses(err) => Error::source(err),
+ RulesetError::CreateRuleset(err) => Error::source(err),
+ RulesetError::AddRules(err) => Error::source(err),
+ RulesetError::RestrictSelf(err) => Error::source(err),
+ }
+ }
+}
+
+impl fmt::Display for RulesetError {
+ fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
+ match self {
+ RulesetError::HandleAccesses(err) => fmt::Display::fmt(err, f),
+ RulesetError::CreateRuleset(err) => fmt::Display::fmt(err, f),
+ RulesetError::AddRules(err) => fmt::Display::fmt(err, f),
+ RulesetError::RestrictSelf(err) => fmt::Display::fmt(err, f),
+ }
+ }
+}
+
+impl std::convert::From<HandleAccessesError> for RulesetError {
+ fn from(source: HandleAccessesError) -> Self {
+ RulesetError::HandleAccesses(source)
+ }
+}
+
+impl std::convert::From<CreateRulesetError> for RulesetError {
+ fn from(source: CreateRulesetError) -> Self {
+ RulesetError::CreateRuleset(source)
+ }
+}
+
+impl std::convert::From<AddRulesError> for RulesetError {
+ fn from(source: AddRulesError) -> Self {
+ RulesetError::AddRules(source)
+ }
+}
+
+impl std::convert::From<RestrictSelfError> for RulesetError {
+ fn from(source: RestrictSelfError) -> Self {
+ RulesetError::RestrictSelf(source)
+ }
}
#[test]
@@ -28,23 +70,71 @@ fn ruleset_error_breaking_change() {
}
/// Identifies errors when updating the ruleset's handled access-rights.
-#[derive(Debug, Error)]
+#[derive(Debug)]
#[non_exhaustive]
pub enum HandleAccessError<T>
where
T: Access,
{
- #[error(transparent)]
- Compat(#[from] CompatError<T>),
+ Compat(CompatError<T>),
}
-#[derive(Debug, Error)]
+impl<T> Error for HandleAccessError<T>
+where
+ T: Access,
+ CompatError<T>: Error,
+ Self: fmt::Debug + fmt::Display,
+{
+ fn source(&self) -> Option<&(dyn Error + 'static)> {
+ match self {
+ HandleAccessError::Compat(err) => Error::source(err),
+ }
+ }
+}
+
+impl<T> fmt::Display for HandleAccessError<T>
+where
+ T: Access,
+ CompatError<T>: fmt::Display,
+{
+ fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
+ match self {
+ HandleAccessError::Compat(err) => fmt::Display::fmt(err, f),
+ }
+ }
+}
+
+impl<T> std::convert::From<CompatError<T>> for HandleAccessError<T>
+where
+ T: Access,
+{
+ fn from(source: CompatError<T>) -> Self {
+ HandleAccessError::Compat(source)
+ }
+}
+
+#[derive(Debug)]
#[non_exhaustive]
pub enum HandleAccessesError {
- #[error(transparent)]
Fs(HandleAccessError<AccessFs>),
}
+impl Error for HandleAccessesError {
+ fn source(&self) -> Option<&(dyn Error + 'static)> {
+ match self {
+ HandleAccessesError::Fs(err) => Error::source(err),
+ }
+ }
+}
+
+impl fmt::Display for HandleAccessesError {
+ fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
+ match self {
+ HandleAccessesError::Fs(err) => fmt::Display::fmt(err, f),
+ }
+ }
+}
+
// Generically implement for all the access implementations rather than for the cases listed in
// HandleAccessesError (with #[from]).
impl<A> From<HandleAccessError<A>> for HandleAccessesError
@@ -57,34 +147,103 @@ where
}
/// Identifies errors when creating a ruleset.
-#[derive(Debug, Error)]
+#[derive(Debug)]
#[non_exhaustive]
pub enum CreateRulesetError {
/// The `landlock_create_ruleset()` system call failed.
- #[error("failed to create a ruleset: {source}")]
#[non_exhaustive]
CreateRulesetCall { source: io::Error },
/// Missing call to [`RulesetAttr::handle_access()`](crate::RulesetAttr::handle_access).
- #[error("missing handled access")]
MissingHandledAccess,
}
+impl Error for CreateRulesetError {
+ fn source(&self) -> Option<&(dyn Error + 'static)> {
+ match self {
+ CreateRulesetError::CreateRulesetCall { source, .. } => Some(source),
+ CreateRulesetError::MissingHandledAccess { .. } => None,
+ }
+ }
+}
+
+impl fmt::Display for CreateRulesetError {
+ fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
+ match self {
+ CreateRulesetError::CreateRulesetCall { source } => {
+ write!(f, "failed to create a ruleset: {source}",)
+ }
+ CreateRulesetError::MissingHandledAccess {} => {
+ write!(f, "missing handled access")
+ }
+ }
+ }
+}
+
/// Identifies errors when adding a rule to a ruleset.
-#[derive(Debug, Error)]
+#[derive(Debug)]
#[non_exhaustive]
pub enum AddRuleError<T>
where
T: Access,
{
/// The `landlock_add_rule()` system call failed.
- #[error("failed to add a rule: {source}")]
#[non_exhaustive]
- AddRuleCall { source: io::Error },
+ AddRuleCall {
+ source: io::Error,
+ },
/// The rule's access-rights are not all handled by the (requested) ruleset access-rights.
- #[error("access-rights not handled by the ruleset: {incompatible:?}")]
- UnhandledAccess { access: T, incompatible: T },
- #[error(transparent)]
- Compat(#[from] CompatError<T>),
+ UnhandledAccess {
+ access: T,
+ incompatible: T,
+ },
+ Compat(CompatError<T>),
+}
+
+impl<T> Error for AddRuleError<T>
+where
+ T: Access,
+ CompatError<T>: Error,
+ Self: fmt::Debug + fmt::Display,
+{
+ fn source(&self) -> Option<&(dyn Error + 'static)> {
+ match self {
+ AddRuleError::AddRuleCall { source, .. } => Some(source),
+ AddRuleError::UnhandledAccess { .. } => None,
+ AddRuleError::Compat(err) => Error::source(err),
+ }
+ }
+}
+
+impl<T> fmt::Display for AddRuleError<T>
+where
+ T: Access,
+ T: fmt::Debug,
+ CompatError<T>: fmt::Display,
+{
+ fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
+ match self {
+ AddRuleError::AddRuleCall { source } => {
+ write!(f, "failed to add a rule: {source}",)
+ }
+ AddRuleError::UnhandledAccess {
+ access: _,
+ incompatible,
+ } => write!(
+ f,
+ "access-rights not handled by the ruleset: {incompatible:?}",
+ ),
+ AddRuleError::Compat(err) => fmt::Display::fmt(err, f),
+ }
+ }
+}
+
+impl<T> std::convert::From<CompatError<T>> for AddRuleError<T>
+where
+ T: Access,
+{
+ fn from(source: CompatError<T>) -> Self {
+ AddRuleError::Compat { 0: source }
+ }
}
// Generically implement for all the access implementations rather than for the cases listed in
@@ -100,32 +259,89 @@ where
/// Identifies errors when adding rules to a ruleset thanks to an iterator returning
/// Result<Rule, E> items.
-#[derive(Debug, Error)]
+#[derive(Debug)]
#[non_exhaustive]
pub enum AddRulesError {
- #[error(transparent)]
Fs(AddRuleError<AccessFs>),
}
-#[derive(Debug, Error)]
+impl Error for AddRulesError {
+ fn source(&self) -> Option<&(dyn Error + 'static)> {
+ match self {
+ AddRulesError::Fs(err) => Error::source(err),
+ }
+ }
+}
+
+impl fmt::Display for AddRulesError {
+ fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
+ match self {
+ AddRulesError::Fs(err) => fmt::Display::fmt(err, f),
+ }
+ }
+}
+
+#[derive(Debug)]
#[non_exhaustive]
pub enum CompatError<T>
where
T: Access,
{
- #[error(transparent)]
- PathBeneath(#[from] PathBeneathError),
- #[error(transparent)]
- Access(#[from] AccessError<T>),
+ PathBeneath(PathBeneathError),
+ Access(AccessError<T>),
}
-#[derive(Debug, Error)]
+impl<T> Error for CompatError<T>
+where
+ T: Access,
+ AccessError<T>: Error,
+ Self: fmt::Debug + fmt::Display,
+{
+ fn source(&self) -> Option<&(dyn Error + 'static)> {
+ match self {
+ CompatError::PathBeneath(err) => Error::source(err),
+ CompatError::Access(err) => Error::source(err),
+ }
+ }
+}
+
+impl<T> fmt::Display for CompatError<T>
+where
+ T: Access,
+ AccessError<T>: fmt::Display,
+{
+ fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
+ match self {
+ CompatError::PathBeneath(err) => fmt::Display::fmt(err, f),
+ CompatError::Access(err) => fmt::Display::fmt(err, f),
+ }
+ }
+}
+
+impl<T> std::convert::From<PathBeneathError> for CompatError<T>
+where
+ T: Access,
+{
+ fn from(source: PathBeneathError) -> Self {
+ CompatError::PathBeneath(source)
+ }
+}
+
+impl<T> std::convert::From<AccessError<T>> for CompatError<T>
+where
+ T: Access,
+{
+ fn from(source: AccessError<T>) -> Self {
+ CompatError::Access(source)
+ }
+}
+
+#[derive(Debug)]
#[non_exhaustive]
pub enum PathBeneathError {
/// To check that access-rights are consistent with a file descriptor, a call to
/// [`RulesetCreatedAttr::add_rule()`](crate::RulesetCreatedAttr::add_rule)
/// looks at the file type with an `fstat()` system call.
- #[error("failed to check file descriptor type: {source}")]
#[non_exhaustive]
StatCall { source: io::Error },
/// This error is returned by
@@ -133,14 +349,39 @@ pub enum PathBeneathError {
/// if the related PathBeneath object is not set to best-effort,
/// and if its allowed access-rights contain directory-only ones
/// whereas the file descriptor doesn't point to a directory.
- #[error("incompatible directory-only access-rights: {incompatible:?}")]
DirectoryAccess {
access: AccessFs,
incompatible: AccessFs,
},
}
-#[derive(Debug, Error)]
+impl Error for PathBeneathError {
+ fn source(&self) -> Option<&(dyn Error + 'static)> {
+ match self {
+ PathBeneathError::StatCall { source, .. } => Some(source),
+ PathBeneathError::DirectoryAccess { .. } => None,
+ }
+ }
+}
+
+impl fmt::Display for PathBeneathError {
+ fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
+ match self {
+ PathBeneathError::StatCall { source } => {
+ write!(f, "failed to check file descriptor type: {source}",)
+ }
+ PathBeneathError::DirectoryAccess {
+ access: _,
+ incompatible,
+ } => write!(
+ f,
+ "incompatible directory-only access-rights: {incompatible:?}",
+ ),
+ }
+ }
+}
+
+#[derive(Debug)]
// Exhaustive enum
pub enum AccessError<T>
where
@@ -148,51 +389,166 @@ where
{
/// The access-rights set is empty, which doesn't make sense and would be rejected by the
/// kernel.
- #[error("empty access-right")]
Empty,
/// The access-rights set was forged with the unsafe `BitFlags::from_bits_unchecked()` and it
/// contains unknown bits.
- #[error("unknown access-rights (at build time): {unknown:?}")]
Unknown { access: T, unknown: T },
/// The best-effort approach was (deliberately) disabled and the requested access-rights are
/// fully incompatible with the running kernel.
- #[error("fully incompatible access-rights: {access:?}")]
Incompatible { access: T },
/// The best-effort approach was (deliberately) disabled and the requested access-rights are
/// partially incompatible with the running kernel.
- #[error("partially incompatible access-rights: {incompatible:?}")]
PartiallyCompatible { access: T, incompatible: T },
}
-#[derive(Debug, Error)]
+impl<T> Error for AccessError<T>
+where
+ T: Access,
+ Self: fmt::Debug + fmt::Display,
+{
+}
+
+impl<T> fmt::Display for AccessError<T>
+where
+ T: Access,
+ T: fmt::Debug,
+{
+ fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
+ match self {
+ AccessError::Empty {} => write!(f, "empty access-right"),
+ AccessError::Unknown { access: _, unknown } => write!(
+ f,
+ "unknown access-rights (at build time): {unknown:?}",
+ unknown = unknown
+ ),
+ AccessError::Incompatible { access } => write!(
+ f,
+ "fully incompatible access-rights: {access:?}",
+ access = access
+ ),
+ AccessError::PartiallyCompatible {
+ access: _,
+ incompatible,
+ } => write!(
+ f,
+ "partially incompatible access-rights: {incompatible:?}",
+ incompatible = incompatible
+ ),
+ }
+ }
+}
+
+#[derive(Debug)]
#[non_exhaustive]
pub enum RestrictSelfError {
/// The `prctl(PR_SET_NO_NEW_PRIVS, 1, 0, 0, 0)` system call failed.
- #[error("failed to set no_new_privs: {source}")]
#[non_exhaustive]
SetNoNewPrivsCall { source: io::Error },
/// The `landlock_restrict_self() `system call failed.
- #[error("failed to restrict the calling thread: {source}")]
#[non_exhaustive]
RestrictSelfCall { source: io::Error },
}
-#[derive(Debug, Error)]
+impl Error for RestrictSelfError {
+ fn source(&self) -> Option<&(dyn Error + 'static)> {
+ match self {
+ RestrictSelfError::SetNoNewPrivsCall { source, .. } => Some(source),
+ RestrictSelfError::RestrictSelfCall { source, .. } => Some(source),
+ }
+ }
+}
+
+impl fmt::Display for RestrictSelfError {
+ fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
+ match self {
+ RestrictSelfError::SetNoNewPrivsCall { source } => {
+ write!(f, "failed to set no_new_privs: {source}",)
+ }
+ RestrictSelfError::RestrictSelfCall { source } => {
+ write!(f, "failed to restrict the calling thread: {source}",)
+ }
+ }
+ }
+}
+
+#[derive(Debug)]
#[non_exhaustive]
pub enum PathFdError {
/// The `open()` system call failed.
- #[error("failed to open \"{path}\": {source}")]
#[non_exhaustive]
OpenCall { source: io::Error, path: PathBuf },
}
+impl Error for PathFdError {
+ fn source(&self) -> Option<&(dyn Error + 'static)> {
+ match self {
+ PathFdError::OpenCall { source, .. } => Some(source),
+ }
+ }
+}
+
+impl fmt::Display for PathFdError {
+ fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
+ match self {
+ PathFdError::OpenCall { source, path } => {
+ write!(
+ f,
+ "failed to open \"{path}\": {source}",
+ path = path.display()
+ )
+ }
+ }
+ }
+}
+
#[cfg(test)]
-#[derive(Debug, Error)]
+#[derive(Debug)]
pub enum TestRulesetError {
- #[error(transparent)]
- Ruleset(#[from] RulesetError),
- #[error(transparent)]
- PathFd(#[from] PathFdError),
- #[error(transparent)]
- File(#[from] std::io::Error),
+ Ruleset(RulesetError),
+ PathFd(PathFdError),
+ File(std::io::Error),
+}
+
+
+#[cfg(test)]
+impl Error for TestRulesetError {
+ fn source(&self) -> Option<&(dyn Error + 'static)> {
+ match self {
+ TestRulesetError::Ruleset(err) => Error::source(err),
+ TestRulesetError::PathFd(err) => Error::source(err),
+ TestRulesetError::File(err) => Error::source(err),
+ }
+ }
+}
+
+#[cfg(test)]
+impl fmt::Display for TestRulesetError {
+ fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
+ match self {
+ TestRulesetError::Ruleset(err) => fmt::Display::fmt(err, f),
+ TestRulesetError::PathFd(err) => fmt::Display::fmt(err, f),
+ TestRulesetError::File(err) => fmt::Display::fmt(err, f),
+ }
+ }
+}
+#[cfg(test)]
+
+impl std::convert::From<RulesetError> for TestRulesetError {
+ fn from(source: RulesetError) -> Self {
+ TestRulesetError::Ruleset(source)
+ }
+}
+
+#[cfg(test)]
+impl std::convert::From<PathFdError> for TestRulesetError {
+ fn from(source: PathFdError) -> Self {
+ TestRulesetError::PathFd(source)
+ }
+}
+
+#[cfg(test)]
+impl std::convert::From<std::io::Error> for TestRulesetError {
+ fn from(source: std::io::Error) -> Self {
+ TestRulesetError::File(source)
+ }
} |
Commit 282090a switched from bitflags to enumflags2 to be more ergonomic. While your FYI, as you may have seen in the comments, I'd like to wrap this type to forbid calling |
8eabdd0
to
0c123b8
Compare
This avoids a proc macro and replaces the BitFlags wrapper type with directly implementing all methods on AccessFs This saves about 25% off the 11.5s compiling from scratch took before this PR on a big machine. This likely has a bigger impact when compiling as part of a bigger project where there is less available parallelism to mask the compilation of enumflags2_derive. Signed-off-by: Björn Roy Baron <[email protected]>
0c123b8
to
6e5a800
Compare
I can write one again. Edit: done
Maybe the Edit: has been suggested before, but was rejected: bitflags/bitflags#269 |
Signed-off-by: Björn Roy Baron <[email protected]>
fcdab23
to
e7f530b
Compare
Great! Can you please merge both commits, refresh perf numbers and add a "Breaking change" warning explaining how to migrate? In theory this is not a breaking change with the previous version but in practice people are already using the main branch. I should probably release a 0.2 version soon, and keep the #12 (and #28) changes for the 0.3. The macro definition should probably be moved to the access.rs file, but I'm not sure if it's allowed by rustc.
We should wrap the bitflags in our own type to avoid public FYI, I plan to change these Access* types to improve the ease of use of the API (e.g. automatic access right inference according to a ruleset's ABI). This PR helps by getting rid of |
Release a new crate version for current users before bringing more invasive API breaking changes (see landlock-lsm#12 and landlock-lsm#23). The API is not ready to be stable yet but perfect is the enemy of good. Switch to the 2021 Rust edition. Only keep myself as the current maintainer. Thanks Vincent for the collaboration in early development! Signed-off-by: Mickaël Salaün <[email protected]>
Release a new crate version for current users before bringing more invasive API breaking changes (see landlock-lsm#12 and landlock-lsm#23). The API is not ready to be stable yet but perfect is the enemy of good. Switch to the 2021 Rust edition. Only keep myself as the current maintainer. Thanks Vincent for the collaboration in early development! Add four crate categories: - api-bindings - os::linux-apis - virtualization - filesystem Signed-off-by: Mickaël Salaün <[email protected]>
Release a new crate version for current users before bringing more invasive API breaking changes (see landlock-lsm#12 and landlock-lsm#23). The API is not ready to be stable yet but perfect is the enemy of good. Switch to the 2021 Rust edition. Remove the "authors" field: https://rust-lang.github.io/rfcs/3052-optional-authors-field.html Add four crate categories: - api-bindings - os::linux-apis - virtualization - filesystem Signed-off-by: Mickaël Salaün <[email protected]>
Release a new crate version for current users before bringing more invasive API breaking changes (see landlock-lsm#12 and landlock-lsm#23). The API is not ready to be stable yet but perfect is the enemy of good. Remove the "authors" field: https://rust-lang.github.io/rfcs/3052-optional-authors-field.html Add four crate categories: - api-bindings - os::linux-apis - virtualization - filesystem Signed-off-by: Mickaël Salaün <[email protected]>
This avoids a proc macro and replaces the BitFlags wrapper type with
directly implementing all methods on AccessFs
This saves about 25% off the 11.5s compiling from scratch took
before this PR on a big machine. This likely has a bigger impact when
compiling as part of a bigger project where there is less available
parallelism to mask the compilation of enumflags2_derive.
Signed-off-by: Björn Roy Baron [email protected]
Builds on top of #22 as this PR slightly conflicts with it.