extrasafe
extrasafe copied to clipboard
Minor Error API Suggestions
Really excited to see this crate, and very impressed by your work so far.
Reading over the code, I see a few places where the ExtraSafeError
could be tweaked to align cleanly with Rust's naming practices.
- Name the enum
Error
- this is consistent with howstd::io::Error
,reqwest::Error
and others behave. - Remove the word
Error
from the variant names, since it's redundant - Make a struct called
ConditionalNoEffectError
which includes named accessors for the fields
#[derive(Debug, thiserror::Error)]
/// The error type produced by [`SafetyContext`]
pub enum Error {
#[error("extrasafe is only usable on Linux.")]
/// Error created when trying to apply filters on non-Linux operating systems. Should never
/// occur.
UnsupportedOS,
#[error(transparent)]
/// Error created when a simple rule would override a conditional rule.
ConditionalNoEffect(#[from] ConditionalNoEffectError),
#[error("A libseccomp error occured. {0:?}")]
/// An error from the underlying seccomp library.
Seccomp(#[from] libseccomp::error::SeccompError),
}
#[derive(Debug, thiserror::Error)]
#[error("A conditional rule on syscall `{syscall}` from RuleSet `{ruleset}` would be overridden by a simple rule from RuleSet `{overridden_by}`.")]
pub struct ConditionalNoEffectError {
syscall: syscalls::Sysno,
ruleset: &'static str,
overridden_by: &'static str,
}
impl ConditionalNoEffectError {
// Should this be pub, or pub(crate)?
pub fn new(syscall: syscalls::Sysno, ruleset: &'static str, overridden_by: &'static str) -> Self {
Self { syscall, ruleset, overridden_by }
}
pub fn syscall(&self) -> syscalls::Sysno {
self.syscall
}
pub fn ruleset(&self) -> &str {
self.ruleset
}
pub fn overridden_by(&self) -> &str {
self.overridden_by
}
}
Pulling this thread further - it looks like several methods promise to only return one of the variants, e.g. SafetyContext::enable
can only return the ConditionalNoEffect
variant. In those cases, should the return type of those functions be more specific?
Hi, thanks so much for your interest and taking the time to look through everything. I'm a bit busy right now but I'll try to find some time to go over your PRs.
I will say ahead that I would like to keep the amount of code to a minimum, so I think specifically stuff like pulling the ConditionalNoEffectError into its own struct is probably something that I don't think is worth having the extra code for unless you have a specific use-case for it.
I do think (from briefly looking) that the unsupported os code can be deduplicated though.
I will say ahead that I would like to keep the amount of code to a minimum, so I think specifically stuff like pulling the ConditionalNoEffectError into its own struct is probably something that I don't think is worth having the extra code for unless you have a specific use-case for it.
A dedicated error struct enables a narrower return type on SafetyContext::enable
, which seems worthwhile. You could probably ditch the accessors to bring the LOCs down if you wanted?