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.

Sign up for GitHub

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

Jump to bottom

Non-consuming RulesetCreated alternative #20

Merged
merged 3 commits into from
Nov 9, 2022
Merged

Non-consuming RulesetCreated alternative #20

merged 3 commits into from
Nov 9, 2022

Conversation

l0kod
Copy link
Member

@l0kod l0kod commented Oct 28, 2022

This is an alternative implementation of #19, as I described in #16.

I think it is simpler than #19, with no duplicated method declarations. The main drawback is that we need to import the new CreatedRulesetRef trait.

What do you think @andreaphylum and @cd-work ?

cd-work
cd-work approved these changes Oct 28, 2022
View reviewed changes
Copy link
Contributor

@cd-work cd-work left a comment

Choose a reason for hiding this comment

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

I'm a fan. This seems pretty simple and the additional import is negligible considering the Rust compiler recommends it anyway even if it's missing.

@l0kod
Copy link
Member Author

l0kod commented Oct 28, 2022

I think I'll also implement RulesetRef to get a consistent builder pattern, with clearly defined transitions.

@andreaphylum
Copy link

I like it better than #19! It looks simpler and more maintainable.

Only addition I'd recommend is sealing the trait, so it can't be implemented by other crates. It should be doable by renaming the current RulesetCreatedRef to RulesetCreatedRefInner, making it private, and creating a new pub trait RulesetCreatedRef: RulesetCreatedRefInner {}. Not super critical, but I think it'd be a good idea to avoid exposing unnecessary API surface.

@l0kod
Copy link
Member Author

l0kod commented Nov 4, 2022

I renamed the traits with *Attr (instead of *Ref), which makes more sense.

I'll do the same for the Compatible trait, but after its refactoring because it will be easier to implement AsMut<CompatLevel>.

@l0kod
Copy link
Member Author

l0kod commented Nov 4, 2022 •
edited
Loading

Only addition I'd recommend is sealing the trait, so it can't be implemented by other crates. It should be doable by renaming the current RulesetCreatedRef to RulesetCreatedRefInner, making it private, and creating a new pub trait RulesetCreatedRef: RulesetCreatedRefInner {}. Not super critical, but I think it'd be a good idea to avoid exposing unnecessary API surface.

It's a good suggestion, but it is almost the case because Ruleset and RulesetCreated contain private fields, which makes them impossible to implement outside this crate. An external type could only implement the related AsRef and AsMut traits for these internal types, but in this case the *Attr traits would still make sense.

@l0kod
ruleset: Move RulesetCreated methods to a new RulesetCreatedAttr trait
6cae7f2 
This enables to make add_rule(), add_rules() and set_no_new_privs()
available from a RulesetCreated reference.

Implement AsMut<RulesetCreated>, required by RulesetCreatedAttr.

Users need to import the RulesetCreatedAttr trait to call such methods.

Closes landlock-lsm#19

Signed-off-by: Mickaël Salaün <mic@digikod.net>
@l0kod
ruleset: Implement and use AsMut<Ruleset>
a797504 
This prepares for a more generic approach to use Ruleset.

Signed-off-by: Mickaël Salaün <mic@digikod.net>
@l0kod
ruleset: Move Ruleset methods to a new RulesetAttr trait
d583ef6 
This enables to make handle_access() available from a Ruleset reference,
similarly to the previous commit that added the RulesetCreatedAttr trait.

Implement AsRef<Ruleset> to make it consistent with RulesetCreatedAttr.

Users need to import the RulesetAttr trait to call such methods.

Signed-off-by: Mickaël Salaün <mic@digikod.net>
@l0kod l0kod merged commit d5bf7d8 into landlock-lsm:main Nov 9, 2022
@l0kod l0kod deleted the ruleset-ref branch November 9, 2022 19:43
@cd-work
Copy link
Contributor

cd-work commented Nov 10, 2022

@l0kod Do you think it would make sense to upload a new version to crates.io? The last 0.1.0 version on it seems to be more of a placeholder and it rather useless compared to all the progress the git repository has made. With the current master pushed to crates.io, the published version would actually be usable.

Or do you have any major concerns with people using the current version of this crate?

@l0kod
Copy link
Member Author

l0kod commented Nov 10, 2022

@l0kod Do you think it would make sense to upload a new version to crates.io? The last 0.1.0 version on it seems to be more of a placeholder and it rather useless compared to all the progress the git repository has made. With the current master pushed to crates.io, the published version would actually be usable.

Or do you have any major concerns with people using the current version of this crate?

The 0.1.0 on crates.io is indeed a placeholder. I want to merge #12 (with the new set_compatibility()) before creating a new version because this will also break the current API. I need to update the doc and rebase it but overall it is almost finished. Feel free to take a look. 😉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cd-work cd-work cd-work approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@l0kod @andreaphylum @cd-work