Search code, repositories, users, issues, pull requests...

        let info = bpf_map_get_info_by_fd(self.fd.as_fd())?;
        Ok(
            unsafe { std::str::from_utf8(std::mem::transmute(info.name.as_slice())).unwrap() }

is there a reason we can't use CStr::from_from_bytes_until_nul here? or even CStr::from_ptr which would avoid the neeed for the transmute.

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

/// iteration can't be performed, for example the caller does not have the necessary privileges,
/// a single item will be yielded containing the error that occurred.
pub fn loaded_maps() -> impl Iterator<Item = Result<MapData, MapError>> {

this needs a test.

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

    iter_map_ids()
        .map(|id| {
            let id = id?;

nit: might be clearer as id.and_then(bpf_map_get_fd_by_id).and_then(MapData::from_fd)

astoycos

Thanks for the PR @preuss-adam!

So some general design things as mentioned in the review...specifically

There's kind of two separate APIs here

A wrapper around the kernel's bpf_map_info struct
Aya's Map object

I think it would be cool to have a from_map_info API as well as the from_id one you've provided here.

Also we probably should add more methods on MapInfo for the relevant/useful fields from https://elixir.bootlin.com/linux/latest/source/include/uapi/linux/bpf.h#L6358

I'm thinking at the very least for the following:

struct bpf_map_info {
	__u32 type;
	__u32 id;
	__u32 key_size;
	__u32 value_size;
	__u32 max_entries;
	__u32 map_flags;
	char  name[BPF_OBJ_NAME_LEN];

lastly we'll need some testing coverage for all of this functionality. Which ultimately should include

Loading an aya::Map from a map ID
Listing all aya::map::MapInfos
Calling all methods on aya::map::MapInfos
Converting a MapInfo into a Map

If you want to break this work into multiple PRs as well that's completely fine.

astoycos · 2023-10-25T14:49:02Z

aya/src/maps/mod.rs

+/// next map id, get the map fd, or the [`MapData`] fail. In cases where
+/// iteration can't be performed, for example the caller does not have the necessary privileges,
+/// a single item will be yielded containing the error that occurred.
+pub fn loaded_maps() -> impl Iterator<Item = Result<MapData, MapError>> {


So I think we want to align with the api we provide for programs on this one. Therefore we should probably return a MapInfo which looks like

/// Provides information about a loaded Map, like name, id and statistics #[derive(Debug)] pub struct MapInfo(bpf_map_info);

rather then reading directly into MapData which wasn't designed to actually reflect all of the kernel state.

Sounds good. Removed this for now and will do this in follow up PR.

astoycos · 2023-10-25T15:16:46Z

aya/src/maps/mod.rs

@@ -612,6 +618,16 @@ impl MapData {
        fd
    }

+    /// Returns the name of the map
+    pub fn name(&self) -> Result<String, MapError> {


again let's use the same method as ProgramInfo.name() here https://github.com/aya-rs/aya/blob/main/aya/src/programs/mod.rs#L1004

Can't quite make it like this yet, as we don't have the struct to reference. Will do as part of follow up PR once we have MapInfo(bpf_map_info);.

astoycos · 2023-10-25T15:22:35Z

aya/src/sys/bpf.rs

+pub(crate) fn bpf_map_get_fd_by_id(map_id: u32) -> Result<OwnedFd, SyscallError> {
+    let mut attr = unsafe { mem::zeroed::<bpf_attr>() };
+
+    attr.__bindgen_anon_6.__bindgen_anon_1.prog_id = map_id;


Suggested change

attr.__bindgen_anon_6.__bindgen_anon_1.prog_id = map_id;

attr.__bindgen_anon_6.__bindgen_anon_1.map_id = map_id;

Oops. Fixed

mergify · 2023-10-26T21:12:30Z

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

preuss-adam · 2023-10-26T21:14:18Z

Thank you both for the review comments. For now, I've pulled out the last commit with loaded_maps as that will require a bit more reworking. The other two commits have been updated.

tamird · 2023-10-26T21:47:14Z

Thank you both for the review comments. For now, I've pulled out the last commit with loaded_maps as that will require a bit more reworking. The other two commits have been updated.

Could you respond to the comments (in reviewable, where applicable, please)?

preuss-adam

Reviewable status: 0 of 3 files reviewed, 9 unresolved discussions (waiting on @alessandrod, @astoycos, and @tamird)

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

Previously, tamird (Tamir Duberstein) wrote…

missing period.

Done.

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

Previously, tamird (Tamir Duberstein) wrote…

this is public API, you'll need to run cargo +nightly xtask public-api --bless to update the API fixtures.

Done.

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

Previously, tamird (Tamir Duberstein) wrote…

missing period.

Done.

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

Previously, tamird (Tamir Duberstein) wrote…

this needs a test.

Done.

Previously, tamird (Tamir Duberstein) wrote…

is there a reason we can't use CStr::from_from_bytes_until_nul here? or even CStr::from_ptr which would avoid the neeed for the transmute.

Done.

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

Previously, tamird (Tamir Duberstein) wrote…

this needs a test.

This will come in as part of the follow up PR. Commit removed for now.

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

Previously, tamird (Tamir Duberstein) wrote…

nit: might be clearer as id.and_then(bpf_map_get_fd_by_id).and_then(MapData::from_fd)

Done

tamird

Reviewed 3 of 3 files at r2, 1 of 1 files at r3, all commit messages.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @alessandrod and @astoycos)

Previously, preuss-adam wrote…

Done.

I think we should reconcile this with https://github.com/aya-rs/aya/blob/main/aya/src/programs/mod.rs#L1004 as @astoycos suggested. It's weird that we have different implementations for these things.

preuss-adam

Should be all addressed now.

Reviewable status: 0 of 3 files reviewed, 4 unresolved discussions (waiting on @alessandrod, @astoycos, and @tamird)

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

Previously, preuss-adam wrote…

Can't quite make it like this yet, as we don't have the struct to reference. Will do as part of follow up PR once we have MapInfo(bpf_map_info);.

Done. New commit on this PR.

Previously, tamird (Tamir Duberstein) wrote…

I think we should reconcile this with https://github.com/aya-rs/aya/blob/main/aya/src/programs/mod.rs#L1004 as @astoycos suggested. It's weird that we have different implementations for these things.

Done. New commit on this PR.

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

Previously, preuss-adam wrote…

Sounds good. Removed this for now and will do this in follow up PR.

Done. New commit on this PR.

tamird

Reviewed 3 of 3 files at r4, all commit messages.
Reviewable status: all files reviewed, 7 unresolved discussions (waiting on @alessandrod, @astoycos, and @preuss-adam)

Previously, preuss-adam wrote…

Done. New commit on this PR.

I think I don't follow. The implementation still seems to be duplicated rather than shared.

aya/src/maps/mod.rs line 661 at r4 (raw file):

        let info = MapInfo::new_from_fd(fd.as_fd())?;
        Ok(Self {
            obj: parse_map_info(info.0, PinningType::None),

consider writing let MapInfo(info) = MapInfo::new_from_fd(...)? to avoid this .0.

aya/src/maps/mod.rs line 719 at r4 (raw file):

    }

    /// Returns information about a loaded map with the [`MapInfo`] structure.

consider avoiding repeating the signature in the commit. also "a map" is weird, it's this map, isn't it?

    }

    /// The name of the map as a &str. If the name was not valid unicode, None is returned.

s/was/is/

preuss-adam

Reviewable status: all files reviewed, 7 unresolved discussions (waiting on @alessandrod, @astoycos, and @tamird)

Previously, tamird (Tamir Duberstein) wrote…

I think I don't follow. The implementation still seems to be duplicated rather than shared.

Depends how you interpret "same method" :) In an earlier version, it did it completely differently. I could put the common code into util if you think that would be cleaner?

aya/src/maps/mod.rs line 661 at r4 (raw file):

Previously, tamird (Tamir Duberstein) wrote…

consider writing let MapInfo(info) = MapInfo::new_from_fd(...)? to avoid this .0.

fixed

aya/src/maps/mod.rs line 719 at r4 (raw file):

Previously, tamird (Tamir Duberstein) wrote…

consider avoiding repeating the signature in the commit. also "a map" is weird, it's this map, isn't it?

yes, fixed.

Previously, tamird (Tamir Duberstein) wrote…

s/was/is/

fixed

tamird

Reviewed 1 of 1 files at r5, all commit messages.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @alessandrod, @astoycos, and @preuss-adam)

Previously, preuss-adam wrote…

Depends how you interpret "same method" :) In an earlier version, it did it completely differently. I could put the common code into util if you think that would be cleaner?

Yeah, I think it would be preferable to avoid duplication.

Previously, preuss-adam wrote…

fixed

on second thought, do we need this method? the caller just as easily do this, right?

preuss-adam

Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @alessandrod, @astoycos, and @tamird)

Previously, tamird (Tamir Duberstein) wrote…

on second thought, do we need this method? the caller just as easily do this, right?

Not really. Only reason is ProgramInfo has it.

preuss-adam

Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @alessandrod, @astoycos, and @tamird)

Previously, tamird (Tamir Duberstein) wrote…

Yeah, I think it would be preferable to avoid duplication.

done

tamird

Reviewed 3 of 3 files at r6, all commit messages.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @alessandrod, @astoycos, and @preuss-adam)

        .unwrap_or(0);

    // The name field is defined as [std::os::raw::c_char; 16]. c_char may be signed or

this comment is talking about something at quite some distance. maybe the signature of this function should be bpf_name: &[std::os::raw::c_char; 16] so that this contract is obvious? then you can just talk about the cast and why it's done.

Previously, preuss-adam wrote…

Not really. Only reason is ProgramInfo has it.

will let @astoycos weigh in on whether either method makes sense.

preuss-adam · 2023-11-15T14:52:20Z

@astoycos Any other thoughts on this PR? What are the next steps to be able to submit? Thanks.

astoycos

Sorry for the delay I was at Kubcon NA last week 😢 , This is looking really good to me just a few small things, thanks for the hard work especially with the unit tests 🥳 (I'll have to go back and add the same for the loaded_programs api)

One last thing can you also add a smoke test like https://github.com/aya-rs/aya/blob/main/test/integration-test/src/tests/smoke.rs#L74 It should be super simple

will let @astoycos weigh in on whether either method makes sense.

Providing the map name as a &str just makes it easier for the user, I like keeping it consistent with the ProgramInfo API 👍

Lastly looks like we need a cargo xtask public-api --bless

Thanks!

aya/src/maps/mod.rs

astoycos · 2023-11-15T16:11:00Z

aya/src/util.rs

@@ -364,6 +364,18 @@ pub(crate) fn bytes_of_slice<T: Pod>(val: &[T]) -> &[u8] {
    unsafe { slice::from_raw_parts(val.as_ptr().cast(), size) }
 }

+pub(crate) fn bytes_of_bpf_name(bpf_name: &[i8]) -> &[u8] {


This won't be i8 for all architectures unfortunately (I've run into this before)

I think you'd either need to make use of generics OR just duplicate the logic for now unless @tamird Knows of a rusty way around this :)

Or maybe use a ffi c type?

Done. Works well with the ffi c type.

This adds support to loading maps by fd similarly to the way programs can be loaded by fd.

preuss-adam · 2023-11-16T16:47:20Z

@astoycos No problem. Changes are pushed. Thanks.

This makes the APIs for loading maps and programs more similar.

astoycos

LGTM

Thanks for all the hard work @preuss-adam I'm looking forward to this

Just need @alessandrod To checkout the API changes

tamird

Reviewed 5 of 5 files at r7, 4 of 4 files at r8, 3 of 3 files at r9, all commit messages.
Dismissed @astoycos from 5 discussions.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @alessandrod, @astoycos, and @preuss-adam)

aya/src/util.rs line 367 at r6 (raw file):

Previously, preuss-adam wrote…

Done. Works well with the ffi c type.

Do we need the leading :: on ::core?

Previously, tamird (Tamir Duberstein) wrote…

this comment is talking about something at quite some distance. maybe the signature of this function should be bpf_name: &[std::os::raw::c_char; 16] so that this contract is obvious? then you can just talk about the cast and why it's done.

Ping this is still unresolved.

tamird

Reviewed 1 of 1 files at r10, all commit messages.
Dismissed @astoycos from a discussion.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @alessandrod, @astoycos, and @preuss-adam)

Previously, tamird (Tamir Duberstein) wrote…

Ping this is still unresolved.

I still think the comment shouldn't repeat the API, and there's no "name field" in sight. I think you just want to say that c_char can be signed or unsigned - though I still don't understand why that leads to us using from_raw_parts...as opposed to what?

preuss-adam

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @alessandrod, @astoycos, and @tamird)