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

aya-obj: Remove name from ProgramSection #720

Merged
merged 1 commit into from
Aug 2, 2023

Conversation

dave-tucker
Copy link
Member

@dave-tucker dave-tucker commented Aug 2, 2023

The name here is never used as we get the program name from the symbol table instead.


This change is Reviewable

@netlify
Copy link

netlify bot commented Aug 2, 2023

Deploy Preview for aya-rs-docs ready!

Name Link
🔨 Latest commit cca9b8f
🔍 Latest deploy log https://app.netlify.com/sites/aya-rs-docs/deploys/64ca6e4e2702440008c9042c
😎 Deploy Preview https://deploy-preview-720--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 mergify bot added aya-obj Relating to the aya-obj crate test A PR that improves test cases or CI labels Aug 2, 2023
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 r1, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @dave-tucker)


aya-obj/src/obj.rs line 198 at r1 (raw file):

/// - if the section name does not contain any slashes,
///   then the program name is just that section name;
/// - if there are some slashes, the name is `section_name.rsplitn(2, '/')[0]`,

for my own edification: in libbpf-land the presence of slashes is used to specify default attachment points, right? in other words, might we want the name back in the future?


aya-obj/src/obj.rs line 341 at r1 (raw file):

                    }
                }
                _ => {

s/_/name/?


aya-obj/src/obj.rs line 396 at r1 (raw file):

            "xdp.frags" => Xdp { name, frags: true },
            "tp_btf" => BtfTracePoint { name },
            _ if kind.starts_with("tracepoint") || kind.starts_with("tp") => {

nit: s/_/kind/ so I don't have to go up 10 lines to see that kind is the thing being matched on

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.

Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @dave-tucker)


aya-obj/src/obj.rs line 280 at r1 (raw file):

            "socket" => SocketFilter,
            "sk_msg" => SkMsg,
            "sk_skb" => match &*name {

here and everywhere: I think this can be name rather than &*name.

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.

Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @tamird)


aya-obj/src/obj.rs line 198 at r1 (raw file):

Previously, tamird (Tamir Duberstein) wrote…

for my own edification: in libbpf-land the presence of slashes is used to specify default attachment points, right? in other words, might we want the name back in the future?

We might want to do some more parsing in future, but it will never be `name, which Aya used as workaround for the lack of support for multiple programs in the same ELF section - something that was added to libbpf 0.2 onwards.

The format is described here:
https://www.kernel.org/doc/html/latest/bpf/libbpf/program_types.html#rawtp
What comes after the first / differs depending on program type, and we'd need to properly parse this.


aya-obj/src/obj.rs line 280 at r1 (raw file):

Previously, tamird (Tamir Duberstein) wrote…

here and everywhere: I think this can be name rather than &*name.

Done.


aya-obj/src/obj.rs line 341 at r1 (raw file):

Previously, tamird (Tamir Duberstein) wrote…

s/_/name/?

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.

Reviewed 1 of 1 files at r2, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @dave-tucker)


aya-obj/src/obj.rs line 198 at r1 (raw file):

Previously, dave-tucker (Dave Tucker) wrote…

We might want to do some more parsing in future, but it will never be `name, which Aya used as workaround for the lack of support for multiple programs in the same ELF section - something that was added to libbpf 0.2 onwards.

The format is described here:
https://www.kernel.org/doc/html/latest/bpf/libbpf/program_types.html#rawtp
What comes after the first / differs depending on program type, and we'd need to properly parse this.

Thanks 👍

Clarifying further: aya still doesn't support multiple programs in the same section, right? I think you asked me to try that on a recent PR.

@dave-tucker
Copy link
Member Author

Reviewed 1 of 1 files at r2, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @dave-tucker)

aya-obj/src/obj.rs line 198 at r1 (raw file):

Previously, dave-tucker (Dave Tucker) wrote…
Thanks 👍

Clarifying further: aya still doesn't support multiple programs in the same section, right? I think you asked me to try that on a recent PR.

It does, so long as they don't have BTF - which is why the aya integration test with 2 progs in the same section passes but your uprobe C test does not. We're incorrectly building the func_info for programs and I'm working on a fix for that now.

@mergify
Copy link

mergify bot commented Aug 2, 2023

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

@mergify mergify bot requested a review from alessandrod August 2, 2023 14:25
@mergify mergify bot added the api/needs-review Makes an API change that needs review label Aug 2, 2023
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.

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


aya-obj/src/obj.rs line 198 at r1 (raw file):

Previously, dave-tucker (Dave Tucker) wrote…

It does, so long as they don't have BTF - which is why the aya integration test with 2 progs in the same section passes but your uprobe C test does not. We're incorrectly building the func_info for programs and I'm working on a fix for that now.

Ah, very helpful. Thanks for explaining (again!).


xtask/public-api/aya.txt line 999 at r3 (raw file):

pub aya::maps::MapError::PinError::name: core::option::Option<alloc::string::String>
pub aya::maps::MapError::ProgramNotLoaded
pub aya::maps::MapError::SyscallError(crate::sys::SyscallError)

I think this is incorrect; looks like you generated the API using a stable toolchain (we use nightly in CI)

@mergify
Copy link

mergify bot commented Aug 2, 2023

@dave-tucker, this pull request is now in conflict and requires a rebase.

@mergify mergify bot added the needs-rebase label Aug 2, 2023
The name here is never used as we get the program name from the symbol
table instead.

Signed-off-by: Dave Tucker <dave@dtucker.co.uk>
@mergify mergify bot added aya This is about aya (userspace) and removed needs-rebase labels Aug 2, 2023
@dave-tucker
Copy link
Member Author

@tamird would you mind taking a look at this rebased version?
I cleaned up the use of { .. } for ProgramSection while I was at it.

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 4 of 4 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 e915379 into aya-rs:main Aug 2, 2023
22 checks passed
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) aya-obj Relating to the aya-obj crate test A PR that improves test cases or CI
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants