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

Remove MapData::pinned. Make MapData::pin public #790

Merged
merged 2 commits into from
Sep 20, 2023

Conversation

dave-tucker
Copy link
Member

@dave-tucker dave-tucker commented Sep 19, 2023

This is 2 small changes.

The first removes tracking of whether or not a Map has been pinned from MapData.
This is unnecessary since Maps may be pinned multiple times to multiple locations on a BPF filesystem.

The second change makes MapData::pin part of the public API.
This is to solve a use-case where a user (in this case bpfd) may want to:

  • MapData::from_pin to open a pinned map from bpffs
  • MapData::pin to pin that object into another bpffs

Both operations should be easily accomplished without needing to cast a MapData into a concrete Map type - e.g aya::maps::HashMap.


This change is Reviewable

@netlify
Copy link

netlify bot commented Sep 19, 2023

Deploy Preview for aya-rs-docs ready!

Name Link
🔨 Latest commit 938f979
🔍 Latest deploy log https://app.netlify.com/sites/aya-rs-docs/deploys/6509ba5da3a7050007392c4b
😎 Deploy Preview https://deploy-preview-790--aya-rs-docs.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@mergify
Copy link

mergify bot commented Sep 19, 2023

Hey @alessandrod, this pull request changes the Aya Public API and requires your review.

@mergify mergify bot requested a review from alessandrod September 19, 2023 13:06
@mergify mergify bot added api/needs-review Makes an API change that needs review aya This is about aya (userspace) test A PR that improves test cases or CI labels Sep 19, 2023
Copy link
Member

@astoycos astoycos left a comment

Choose a reason for hiding this comment

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

LGTM

I'll let this merge before continuing on #783 Thanks!

Copy link
Member

@tamird tamird left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewed 3 of 3 files at r1, 2 of 2 files at r2, all commit messages.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @alessandrod and @dave-tucker)


-- commits line 4 at r1:
multiple

Code quote:

mutliple

-- commits line 12 at r2:
in the PR description you write

The second change makes MapData::pin part of the public API.
This is to solve a use-case where a user (in this case bpfd) may want to:

  • MapData::from_pin to open a pinned map from bpffs
  • MapData::pin to pin that object into another bpffs

Both operations should be easily accomplished without needing to cast a MapData into a concrete Map type - e.g aya::maps::HashMap.

Can this rationale be in the commit message too?


aya/src/maps/mod.rs line 559 at r1 (raw file):

    pub(crate) fn pin<P: AsRef<Path>>(&mut self, name: &str, path: P) -> Result<(), PinError> {
        use std::os::unix::ffi::OsStrExt as _;

nit: keep this blank line?


aya/src/maps/mod.rs line 572 at r2 (raw file):

    pub fn pin<P: AsRef<Path>>(&mut self, path: P) -> Result<(), PinError> {
        use std::os::unix::ffi::OsStrExt as _;
        let path = path.as_ref();

nit: keep this in its old position? or is there a reason it moved?

BPF objects can be pinned multiple times, to multiple different places.
Tracking whether or not a map is pinned in a bool is therefore not sufficient.
We could track this in a HashSet<PathBuf>, but there is really no reason
to track it at all.

Signed-off-by: Dave Tucker <[email protected]>
This is to solve a use-case where a user (in this case bpfd) may want
to:

- MapData::from_pin to open a pinned map from bpffs
- MapData::pin to pin that object into another bpffs

Both operations should be easily accomplished without needing to cast
a MapData into a concrete Map type - e.g aya::maps::HashMap.

Signed-off-by: Dave Tucker <[email protected]>
Copy link
Member Author

@dave-tucker dave-tucker left a comment

Choose a reason for hiding this comment

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

Comments addressed. Thanks.

Reviewable status: 1 of 3 files reviewed, 2 unresolved discussions (waiting on @alessandrod and @tamird)


-- commits line 4 at r1:

Previously, tamird (Tamir Duberstein) wrote…

multiple

Done.


-- commits line 12 at r2:

Previously, tamird (Tamir Duberstein) wrote…

in the PR description you write

The second change makes MapData::pin part of the public API.
This is to solve a use-case where a user (in this case bpfd) may want to:

  • MapData::from_pin to open a pinned map from bpffs
  • MapData::pin to pin that object into another bpffs

Both operations should be easily accomplished without needing to cast a MapData into a concrete Map type - e.g aya::maps::HashMap.

Can this rationale be in the commit message too?

Done.

Copy link
Member

@tamird tamird left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewed 2 of 2 files at r3, 2 of 2 files at r4, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @alessandrod)

@dave-tucker dave-tucker merged commit 42fd82e into aya-rs:main Sep 20, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api/needs-review Makes an API change that needs review aya This is about aya (userspace) test A PR that improves test cases or CI
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants