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

aya: MapData::fd is non-optional #758

Merged
merged 2 commits into from
Aug 17, 2023
Merged

aya: MapData::fd is non-optional #758

merged 2 commits into from
Aug 17, 2023

Conversation

tamird
Copy link
Member

@tamird tamird commented Aug 16, 2023

The primary driver of change here is that MapData::create is now a
factory function that returns Result<Self, _> rather than mutating
&mut self. The remaining changes are consequences of that change, the
most notable of which is the removal of several errors which are no
longer possible.

@netlify
Copy link

netlify bot commented Aug 16, 2023 •
edited
Loading

✅ Deploy Preview for aya-rs-docs ready!

Name Link
🔨 Latest commit a31544b
🔍 Latest deploy log https://app.netlify.com/sites/aya-rs-docs/deploys/64de314e78443900086e74df
😎 Deploy Preview https://deploy-preview-758--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
Copy link

mergify bot commented Aug 16, 2023

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

@mergify mergify bot requested a review from alessandrod August 16, 2023 15:57
@mergify mergify bot added 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 labels Aug 16, 2023
ajwerner
ajwerner approved these changes Aug 16, 2023
View reviewed changes
Copy link
Member

@ajwerner ajwerner left a comment

Choose a reason for hiding this comment

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

This seems reasonable, but is this change motivated as part of some other effort? I get the ways in which this is better. I suppose before it was possible to defer the actual creation of the map from the setup and it's not now, but I'm unsure as to why somebody might have wanted to do that.

@tamird
Copy link
Member Author

tamird commented Aug 16, 2023 •
edited
Loading

This seems reasonable, but is this change motivated as part of some other effort?

I noticed the oddity during review of another PR and decided to fix it. In particular this makes life better when we later replace the RawFd with an OwnedFd (notice we are no longer passing a "42" file descriptor in tests)

I get the ways in which this is better. I suppose before it was possible to defer the actual creation of the map from the setup and it's not now, but I'm unsure as to why somebody might have wanted to do that.

Indeed. Unclear why anyone would want such deferral.

alessandrod
alessandrod reviewed Aug 17, 2023
View reviewed changes
aya/src/maps/array/array.rs Outdated
@@ -39,7 +39,7 @@ impl<T: Borrow<MapData>, V: Pod> Array<T, V> {
let data = map.borrow();
check_kv_size::<u32, V>(data)?;

let _fd = data.fd_or_err()?;
let _fd = data.fd;
Copy link
Collaborator

Choose a reason for hiding this comment

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

This can go now? We used to get the fd to ensure that the map was created, but
now it's always created

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. Thanks for catching that.

alessandrod
alessandrod reviewed Aug 17, 2023
View reviewed changes
aya/src/maps/array/per_cpu_array.rs Outdated
@@ -58,7 +58,7 @@ impl<T: Borrow<MapData>, V: Pod> PerCpuArray<T, V> {
let data = map.borrow();
check_kv_size::<u32, V>(data)?;

let _fd = data.fd_or_err()?;
let _fd = data.fd;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same here (and everywhere else)

alessandrod
alessandrod reviewed Aug 17, 2023
View reviewed changes
aya/src/maps/mod.rs Outdated
pinned: false,
}),
Err(_) => {
let mut map = Self::create(obj, name, None)?;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why is btf_fd hardcoded to None here?

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.

@tamird tamird force-pushed the map-fd-not-option branch 2 times, most recently from a352537 to d749634 Compare August 17, 2023 14:36
alessandrod
alessandrod reviewed Aug 17, 2023
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.

Thanks again for doing this, see comments

@tamird
aya: MapData::fd is non-optional
89bc255 
The primary driver of change here is that `MapData::create` is now a
factory function that returns `Result<Self, _>` rather than mutating
`&mut self`. The remaining changes are consequences of that change, the
most notable of which is the removal of several errors which are no
longer possible.
@tamird
maps: BloomFilter::insert takes &mut self
a31544b 
This is consistent with all the other maps.
@tamird
Copy link
Member Author

tamird commented Aug 17, 2023

I've addressed all the comments, PTAL.

@alessandrod
Copy link
Collaborator

I get the ways in which this is better. I suppose before it was possible to defer the actual creation of the map from the setup and it's not now, but I'm unsure as to why somebody might have wanted to do that.

Indeed. Unclear why anyone would want such deferral.

context for posterity https://discord.com/channels/855676609003651072/855676609003651075/1141557678955577445

@tamird tamird merged commit 1d5f764 into main Aug 17, 2023
22 checks passed
@tamird tamird deleted the map-fd-not-option branch August 17, 2023 14:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@alessandrod alessandrod alessandrod approved these changes

@ajwerner ajwerner ajwerner approved these changes

Assignees
No one assigned
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
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@tamird @alessandrod @ajwerner