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

Make AsMut unambiguous #52

Merged
merged 1 commit into from
Oct 3, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 11 additions & 3 deletions src/compat.rs
Original file line number Diff line number Diff line change
Expand Up @@ -277,6 +277,14 @@ impl Compatibility {
}
}

pub(crate) mod private {
use crate::CompatLevel;

pub trait OptionCompatLevelMut {
fn as_option_compat_level_mut(&mut self) -> &mut Option<CompatLevel>;
}
}
Comment on lines +280 to +286
Copy link
Contributor

Choose a reason for hiding this comment

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

This feels like it will be a compiler error at some point in the future.

Copy link
Member Author

Choose a reason for hiding this comment

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

This feels like it will be a compiler error at some point in the future.

What exactly and why?

Copy link
Contributor

Choose a reason for hiding this comment

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

The OptionCompatLevelMut is part of the public API as a constraint on Compatible, right? But it's not actually made public.

There is a private type in public interface warning in rustc, but I suppose it doesn't apply to this (yet?).

It's possible this will never get changed since it might be considered a "feature" to prevent others from implementing a trait defined in your library, but it still seems not ideal to me.

Copy link
Member Author

Choose a reason for hiding this comment

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

That may be a hack but there is no other way to make this work without exposing internal APIs. I did use this trick some years ago and it's still working the same. I don't think Rust will change that without an alternative.

Copy link
Contributor

Choose a reason for hiding this comment

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

It's up to you, but if I had to choose between the two solutions I'd tell downstream to just deal with it rather than introducing a hacky workaround like this. Especially since you'll also have to do this for every new AsRef thing you want to add in the future.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yep, that's not optimal, but we'll see if we need to add back the AsMut API.


/// Properly handles runtime unsupported features.
///
/// This guarantees consistent behaviors across crate users
Expand All @@ -292,7 +300,7 @@ impl Compatibility {
/// (e.g. applications carefully designed to only be run with a specific set of kernel features),
/// it may be required to error out if some of these features are not available
/// and will then not be enforced.
pub trait Compatible: Sized + AsMut<Option<CompatLevel>> {
pub trait Compatible: Sized + private::OptionCompatLevelMut {
/// To enable a best-effort security approach,
/// Landlock features that are not supported by the running system
/// are silently ignored by default,
Expand Down Expand Up @@ -396,7 +404,7 @@ pub trait Compatible: Sized + AsMut<Option<CompatLevel>> {
/// }
/// ```
fn set_compatibility(mut self, level: CompatLevel) -> Self {
*self.as_mut() = Some(level);
*self.as_option_compat_level_mut() = Some(level);
self
}

Expand Down Expand Up @@ -491,7 +499,7 @@ where
// Using a mutable reference is not required but it makes the code simpler (no double AsRef
// implementations for each Compatible types), and more importantly it guarantees
// consistency with Compatible::set_compatibility().
match self.as_mut() {
match self.as_option_compat_level_mut() {
None => parent_level.into(),
// Returns the most constrained compatibility level.
Some(ref level) => parent_level.into().max(*level),
Expand Down
18 changes: 14 additions & 4 deletions src/fs.rs
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
use crate::compat::private::OptionCompatLevelMut;
use crate::{
uapi, Access, AddRuleError, AddRulesError, CompatError, CompatLevel, CompatResult, CompatState,
Compatible, HandleAccessError, HandleAccessesError, PathBeneathError, PathFdError,
Expand Down Expand Up @@ -338,8 +339,14 @@ fn path_beneath_try_compat() {
}
}

impl<F> AsMut<Option<CompatLevel>> for PathBeneath<F> {
fn as_mut(&mut self) -> &mut Option<CompatLevel> {
impl<F> OptionCompatLevelMut for PathBeneath<F> {
fn as_option_compat_level_mut(&mut self) -> &mut Option<CompatLevel> {
&mut self.compat_level
}
}

impl<F> OptionCompatLevelMut for &mut PathBeneath<F> {
fn as_option_compat_level_mut(&mut self) -> &mut Option<CompatLevel> {
&mut self.compat_level
}
}
Expand All @@ -353,15 +360,18 @@ fn path_beneath_compatibility() {
let mut path = PathBeneath::new(PathFd::new("/").unwrap(), AccessFs::from_all(ABI::V1));
let path_ref = &mut path;

let level = path_ref.as_mut();
let level = path_ref.as_option_compat_level_mut();
assert_eq!(level, &None);
assert_eq!(
<Option<CompatLevel> as Into<CompatLevel>>::into(*level),
CompatLevel::BestEffort
);

path_ref.set_compatibility(CompatLevel::SoftRequirement);
assert_eq!(path_ref.as_mut(), &Some(CompatLevel::SoftRequirement));
assert_eq!(
path_ref.as_option_compat_level_mut(),
&Some(CompatLevel::SoftRequirement)
);

path.set_compatibility(CompatLevel::HardRequirement);
}
Expand Down
35 changes: 31 additions & 4 deletions src/ruleset.rs
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
use crate::compat::private::OptionCompatLevelMut;
use crate::{
uapi, Access, AccessFs, AddRuleError, AddRulesError, BitFlags, CompatLevel, CompatState,
Compatibility, Compatible, CreateRulesetError, RestrictSelfError, RulesetError, TryCompat,
Expand Down Expand Up @@ -284,8 +285,14 @@ impl Ruleset {
}
}

impl AsMut<Option<CompatLevel>> for Ruleset {
fn as_mut(&mut self) -> &mut Option<CompatLevel> {
impl OptionCompatLevelMut for Ruleset {
fn as_option_compat_level_mut(&mut self) -> &mut Option<CompatLevel> {
&mut self.compat.level
}
}

impl OptionCompatLevelMut for &mut Ruleset {
fn as_option_compat_level_mut(&mut self) -> &mut Option<CompatLevel> {
&mut self.compat.level
}
}
Expand All @@ -300,6 +307,20 @@ impl AsMut<Ruleset> for Ruleset {
}
}

// Tests unambiguous type.
#[test]
fn ruleset_as_mut() {
let mut ruleset = Ruleset::from(ABI::Unsupported);
let _ = ruleset.as_mut();

let mut ruleset_created = Ruleset::from(ABI::Unsupported)
.handle_access(AccessFs::Execute)
.unwrap()
.create()
.unwrap();
let _ = ruleset_created.as_mut();
}

pub trait RulesetAttr: Sized + AsMut<Ruleset> + Compatible {
/// Attempts to add a set of access rights that will be supported by this ruleset.
/// By default, all actions requiring these access rights will be denied.
Expand Down Expand Up @@ -372,8 +393,14 @@ fn ruleset_created_handle_access_or() {
));
}

impl AsMut<Option<CompatLevel>> for RulesetCreated {
fn as_mut(&mut self) -> &mut Option<CompatLevel> {
impl OptionCompatLevelMut for RulesetCreated {
fn as_option_compat_level_mut(&mut self) -> &mut Option<CompatLevel> {
&mut self.compat.level
}
}

impl OptionCompatLevelMut for &mut RulesetCreated {
fn as_option_compat_level_mut(&mut self) -> &mut Option<CompatLevel> {
&mut self.compat.level
}
}
Expand Down
Loading