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

Load static maps #387

Merged
merged 1 commit into from
Sep 9, 2022
Merged

Load static maps #387

merged 1 commit into from
Sep 9, 2022

Conversation

astoycos
Copy link
Member

@astoycos astoycos commented Sep 6, 2022

fixes #385

This PR adds the new aya::maps::{from_pinned, from_fd} APIs. Which will allow users to load in and interact with BPF maps from either a raw file descriptor or map pin point.

The user can get a concrete aya map type to interact with like so

    let map_pin_path = Path::new("/sys/fs/bpf/basic-node-firewall/BLOCKLIST");

    let mut block_map = Map::from_pinned(map_pin_path)?; 

    let mut blocklist: HashMap<_, PacketFiveTuple, u32> =
    HashMap::try_from(&mut block_map)?;

I tested this locally with a pinned map, but was unsure exactly how to add unit tests....and would appreciate any feedback on that :)

@netlify
Copy link

netlify bot commented Sep 6, 2022 •
edited
Loading

✅ Deploy Preview for aya-rs-docs ready!

Built without sensitive environment variables

Name Link
🔨 Latest commit 8a9cbf1
🔍 Latest deploy log https://app.netlify.com/sites/aya-rs-docs/deploys/631a4acad4723200097b8bfc
😎 Deploy Preview https://deploy-preview-387--aya-rs-docs.netlify.app/user/src/aya/sys/bpf.rs
📱 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 settings.

@alessandrod alessandrod self-requested a review September 6, 2022 21:13
alessandrod
alessandrod previously requested changes Sep 6, 2022
View reviewed changes
Copy link
Collaborator

@alessandrod alessandrod left a comment

Choose a reason for hiding this comment

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

This looks great, thanks for working on it 😊 See comments

aya/src/bpf.rs Outdated Show resolved Hide resolved
aya/src/maps/mod.rs Outdated Show resolved Hide resolved
aya/src/maps/mod.rs Outdated Show resolved Hide resolved
aya/src/maps/mod.rs Outdated Show resolved Hide resolved
aya/src/maps/mod.rs Outdated Show resolved Hide resolved
aya/src/obj/mod.rs Outdated Show resolved Hide resolved
aya/src/programs/lirc_mode2.rs Outdated Show resolved Hide resolved
aya/src/sys/bpf.rs Outdated Show resolved Hide resolved
aya/src/sys/bpf.rs Outdated Show resolved Hide resolved
aya/src/sys/bpf.rs Outdated Show resolved Hide resolved
dave-tucker
dave-tucker reviewed Sep 6, 2022
View reviewed changes
Copy link
Member

@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.

Looking good! A couple of comments to address...

aya/src/maps/mod.rs Outdated Show resolved Hide resolved
aya/src/maps/mod.rs Show resolved Hide resolved
aya/src/maps/mod.rs Outdated
}

/// Loads a map from a rawfd.
/// We assume that the map is not pinned in this case.
Copy link
Member

Choose a reason for hiding this comment

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

I think we should elaborate on the use case a little here to avoid confusion.

/// Loads a map from a [`RawFd`].
///
/// If loading from a BPF Filesystem (bpffs) you should use [`Map::from_pinned`].
/// This API is intended for cases where you have received a valid BPF FD from some other means.
/// For example, you received an FD over Unix Domain Socket.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done

@dave-tucker
Copy link
Member

@astoycos please rebase to pick up #388 which should fix CI

@astoycos
Copy link
Member Author

astoycos commented Sep 8, 2022

I think I got to everything here, thanks @dave-tucker @alessandrod!

@astoycos
Add from_pinned and from_fd methods
8a9cbf1 
Add `from_pinned` to allow loading BPF maps
from pinned points in the bpffs and
`from_fd` to allow loading BPF maps from
RawFds aquired via some other means eg
a unix socket.

These functions return an
aya::Map which has not been used previously
but will be the future abstraction once
all bpf maps are represented as an enum.

Signed-off-by: Andrew Stoycos <astoycos@redhat.com>
@astoycos
Copy link
Member Author

astoycos commented Sep 8, 2022

Oops rebased!

dave-tucker
dave-tucker approved these changes Sep 9, 2022
View reviewed changes
Copy link
Member

@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.

LGTM

@dave-tucker dave-tucker dismissed alessandrod’s stale review September 9, 2022 00:26

all comments addressed

alessandrod
alessandrod reviewed Sep 9, 2022
View reviewed changes
Copy link
Collaborator

@alessandrod alessandrod left a comment

Choose a reason for hiding this comment

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

Just a couple of nits then it's ready to go!

aya/src/maps/mod.rs
CString::new(path.as_ref().to_string_lossy().into_owned()).map_err(|e| {
MapError::PinError {
name: None,
error: PinError::InvalidPinPath {
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: I think this should be PinError::InvalidPath

aya/src/maps/mod.rs
})?;

Ok(Map {
obj: obj::parse_map_info(info, crate::PinningType::ByName),
Copy link
Collaborator

Choose a reason for hiding this comment

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

add use statements for both parse_map_info and PinningType don't use paths

aya/src/maps/mod.rs
})?;

Ok(Map {
obj: obj::parse_map_info(info, crate::PinningType::None),
Copy link
Collaborator

Choose a reason for hiding this comment

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

add use statement for parse_map_info

aya/src/maps/mod.rs

self.fd = Some(fd);

Ok(fd)
}

/// Loads a map from a pinned path in bpffs.
pub fn from_pinned<P: AsRef<Path>>(path: P) -> Result<Map, MapError> {
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is from_pinned but in another PR we're adding from_path. We should agree on one.

@dave-tucker
Copy link
Member

Merging as-is. I'll fixup the final review nits and open a PR to rename all the pinning apis to use from_pin instead of from_pinned or from_path.

@dave-tucker dave-tucker merged commit eb26a6b into aya-rs:main Sep 9, 2022
dave-tucker added a commit to dave-tucker/aya that referenced this pull request Sep 9, 2022
@dave-tucker
aya: Fix review comments from aya-rs#387
de6fa98 
Signed-off-by: Dave Tucker <dave@dtucker.co.uk>
@dave-tucker dave-tucker mentioned this pull request Sep 9, 2022
Merged
@dave-tucker dave-tucker added feature A PR that implements a new feature or enhancement aya This is about aya (userspace) labels Feb 23, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@alessandrod alessandrod alessandrod left review comments

@dave-tucker dave-tucker dave-tucker approved these changes

Assignees
No one assigned
Labels
aya This is about aya (userspace) feature A PR that implements a new feature or enhancement
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Allow the user to load pre-existing BPF maps from a pin point or a Raw FD
3 participants
@astoycos @dave-tucker @alessandrod