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

Minor Error API Suggestions #1

Open
TedDriggs opened this issue Mar 4, 2022 · 3 comments
Open

Minor Error API Suggestions #1

TedDriggs opened this issue Mar 4, 2022 · 3 comments

Comments

@TedDriggs
Copy link

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
Copy link
Author

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?

@boustrophedon
Copy link
Owner

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.

@TedDriggs
Copy link
Author

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?

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

No branches or pull requests

2 participants