Skip to content

Conversation

@swananan
Copy link
Contributor

@swananan swananan commented Dec 17, 2025

This follows the #1417 review discussion:

By bundling location + cookie into a UProbeAttachPoint we get a more idiomatic Into<_> entry point, keep the one-to-one relationship enforced by the type system, and make it easier to extend attach with multi-location support without introducing parallel arrays or a brand new API.


This change is Reviewable

@netlify
Copy link

netlify bot commented Dec 17, 2025

Deploy Preview for aya-rs-docs ready!

Built without sensitive environment variables

Name Link
🔨 Latest commit 2204f75
🔍 Latest deploy log https://app.netlify.com/projects/aya-rs-docs/deploys/694ac7eda9602b0008f737bf
😎 Deploy Preview https://deploy-preview-1421--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 project configuration.

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.

@tamird reviewed 15 files and all commit messages, and made 4 comments.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @swananan).


test/integration-test/src/tests/uprobe_cookie.rs line 29 at r1 (raw file):

    const PROG_B: &str = "uprobe_cookie_trigger_ebpf_program_b";
    let attach = |prog: &mut UProbe, fn_name, cookie| {
        prog.attach((fn_name, cookie), "/proc/self/exe", None)

would be nice to use the explicit form here (UProbeAttachPoint { .. }).

In fact, I think we should probably drop the conversion from a tuple, it doesn't buy as anything (the existing conversion traits were here to avoid source breakage relative to the previous API that took just a string)


aya/src/programs/uprobe.rs line 53 at r1 (raw file):

/// The location in the target object file to which the uprobe is to be
/// attached.
#[derive(Debug, Clone)]

do we need these? ditto below on UProbeAttachPoint


aya/src/programs/uprobe.rs line 98 at r1 (raw file):

        self.cookie = Some(cookie);
        self
    }

is there precedent for this builder pattern? i'd prefer to avoid it

actually, why do we need this whole impl block at all? all fields are pub

Code quote:

    /// Sets the cookie for this attach point.
    pub fn with_cookie(mut self, cookie: u64) -> Self {
        self.cookie = Some(cookie);
        self
    }

aya/src/programs/uprobe.rs line 148 at r1 (raw file):

    /// argument may be just a location (no cookie), a [`UProbeAttachPoint`], or
    /// a `(location, cookie)` tuple; the latter two set the cookie explicitly.
    pub fn attach<'loc, T: AsRef<Path>, Point: Into<UProbeAttachPoint<'loc>>>(

plz rename 'loc

@swananan swananan force-pushed the feature/uprobe-attach branch from ee9d35a to 9f56633 Compare December 23, 2025 16:40
This follows the aya-rs#1417 review discussion: by bundling location
+ cookie into a UProbeAttachPoint we get a more idiomatic Into<_>
entry point, keep the one-to-one relationship enforced by the type
system, and make it easier to extend attach with multi-location
support without introducing parallel arrays or a brand new API.
@swananan swananan force-pushed the feature/uprobe-attach branch from 9f56633 to 2204f75 Compare December 23, 2025 16:48
@swananan
Copy link
Contributor Author

@tamird thanks for the review, I have addressed all the comments. I misused build pattern here, since pub fields are enough.

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:

@tamird reviewed 3 files and all commit messages, made 1 comment, and resolved 4 discussions.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @swananan).

@tamird tamird merged commit eb99da3 into aya-rs:main Dec 23, 2025
21 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants