extrasafe icon indicating copy to clipboard operation
extrasafe copied to clipboard

Minor Error API Suggestions

Open TedDriggs opened this issue 2 years ago • 3 comments

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.

  1. Name the enum Error - this is consistent with how std::io::Error, reqwest::Error and others behave.
  2. Remove the word Error from the variant names, since it's redundant
  3. 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
    }
}

TedDriggs avatar Mar 04 '22 16:03 TedDriggs

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?

TedDriggs avatar Mar 04 '22 16:03 TedDriggs

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.

boustrophedon avatar Mar 04 '22 22:03 boustrophedon

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?

TedDriggs avatar Mar 04 '22 22:03 TedDriggs