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: rework links #249

Merged
merged 1 commit into from May 4, 2022
Merged

aya: rework links #249

merged 1 commit into from May 4, 2022

Conversation

alessandrod
Copy link
Collaborator

Remove LinkRef and remove the Rc<RefCell<_>> that was used to store type-erased link values in ProgramData. Among other things, this allows Bpf to be Send, which makes it easier to use it with async runtimes.

Change the link API to:

let link_id = prog.attach(...)?;
...
prog.detach(link_id)?;

Link ids are strongly typed, so it's impossible to eg:

let link_id = uprobe.attach(...)?;
xdp.detach(link_id);

As it would result in a compile time error.

Links are still stored inside ProgramData, and unless detached explicitly, they are automatically detached when the parent program gets dropped.

@alessandrod
Copy link
Collaborator Author

This is another take on similar work that happened in #123

@alessandrod
Copy link
Collaborator Author

I guess to explain the gist of the PR:

  • links are stored in ProgramData::links
  • ProgramData::links is a hash map, where the keys are the link ids, and the values are the links
  • attach(...) inserts into ProgramData::links and returns the corresponding link_id (map key)
  • detach(link_id) deletes from ProgramData::links using link_id as the key

As a follow up to this PR, I'm going to add:

let link_id = program.attach(...)?;

// this leaves the link attached, transferring ownership to the caller (the name forget_link is provisional)
let owned_link = program.forget_link(link_id);

// this will make it so that the link stays attached when owned_link is dropped
owned_link.forget(); 

@alessandrod alessandrod force-pushed the new-links branch 3 times, most recently from f5e539c to d250fb6 Compare May 2, 2022 05:12
Remove LinkRef and remove the Rc<RefCell<_>> that was used to store
type-erased link values in ProgramData. Among other things, this allows
`Bpf` to be `Send`, which makes it easier to use it with async runtimes.

Change the link API to:

    let link_id = prog.attach(...)?;
    ...
    prog.detach(link_id)?;

Link ids are strongly typed, so it's impossible to eg:

    let link_id = uprobe.attach(...)?;
    xdp.detach(link_id);

As it would result in a compile time error.

Links are still stored inside ProgramData, and unless detached
explicitly, they are automatically detached when the parent program gets
dropped.
Copy link
Member

@vadorovsky vadorovsky left a comment

Choose a reason for hiding this comment

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

I tested it with 2 different projects and it works perfectly. No remarks to the code, I really like the new API!

Copy link
Member

@vadorovsky vadorovsky left a comment

Choose a reason for hiding this comment

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

I actually have one remark now nah, it was a brainfart

/// Detaches the program.
///
/// See [CgroupSkb::attach].
pub fn detach(&mut self, link_id: CgroupSkbLinkId) -> Result<(), ProgramError> {
Copy link
Member

Choose a reason for hiding this comment

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

Or hmm... actually using remove on HashMap always solves the problem. But anyway, there might be people wanting to use a reference for some other reason.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Nope taking self by value is very much intended: after detach a link becomes unusable (can't detach twice), and moving self ensures that trying to detach twice would fail at compile time.

Copy link
Member

Choose a reason for hiding this comment

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

OK, good point, then seems like remove is the only way to go with HashMaps on the userspace side.

/// Detaches the program.
///
/// See [CgroupSkb::attach].
pub fn detach(&mut self, link_id: CgroupSkbLinkId) -> Result<(), ProgramError> {
Copy link
Member

Choose a reason for hiding this comment

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

Or hmm... actually using remove on HashMap always solves the problem. But anyway, there might be people wanting to use a reference for some other reason.

@dave-tucker dave-tucker added feature A PR that implements a new feature or enhancement aya This is about aya (userspace) labels May 3, 2022
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.
@alessandrod just to be 100% clear. Per the comment in #123, we'll need #51 implemented... and to add to the release notes for 0.11.0 that there is a change for LircMode2, that links will need to be link_id.forget() - or whatever the API will be - to restore the pre 0.11.0 behaviour where Lirc programs would remain attached even after drop. Correct?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
aya This is about aya (userspace) feature A PR that implements a new feature or enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants