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

lib: make register_buffers unsafe #219

Merged
merged 2 commits into from
Mar 20, 2023

Conversation

fathyb
Copy link
Contributor

@fathyb fathyb commented Mar 18, 2023

This breaking change is required as calling register_buffers with incorrect inputs may introduce undefined behaviour.

This PR also changes the method documentation to use fixed buffers instead of user buffers to match the io_uring man pages terminology, related to #217.

BREAKING CHAMGE: this is required as incorrect inputs
may introduce undefined behaviour.

Please see the "Safety" section of the
`Submitter::register_buffers` documentation for more
information about safety requirements.
@FrankReh
Copy link
Contributor

I don't mind the breaking change and you raise a good point for a crate that wants to be sound. Could you provide a Safety comment that was more in the affirmative of what the caller has to do to maintain the safety contract? Saying it should not be used improperly is more vague than other tokio-maintained unsafe comments.

@fathyb
Copy link
Contributor Author

fathyb commented Mar 18, 2023

I've tweaked the message to match the one on Submission::push, let me know what you think!

@FrankReh
Copy link
Contributor

Yes, looks better to me. @quininer is in a different timezone. Have to wait for their review.

@FrankReh FrankReh requested a review from quininer March 18, 2023 15:13
@quininer
Copy link
Member

This makes sense. I will release a 0.6-pre first so that I can merge other break changes in the meantime.

@quininer quininer merged commit 4a72c92 into tokio-rs:master Mar 20, 2023
@quininer
Copy link
Member

I noticed that register_buf_ring is still not marked unsafe and ring_addr is not an address type. maybe we should change it too. cc @FrankReh

@FrankReh
Copy link
Contributor

I noticed that register_buf_ring is still not marked unsafe and ring_addr is not an address type. maybe we should change it too. cc @FrankReh

Yes. I can see the value in making those changes now too.

You think the ring_addr type should be made *const BufRingEntry? You give me a day or two to make a PR?

@quininer
Copy link
Member

*const BufRingEntry is fine, please feel free to open PR. :)

@FrankReh
Copy link
Contributor

*const BufRingEntry is fine, please feel free to open PR. :)

Checkout PR 221. I didn't include a type change because as the test code shows, a user of the library is not always really starting with a BufRingEntry array or vector and casting to a u64 is more directly what the io_uring API calls for.

But I'm on the fence. If you want it, I will do that too. I'm just not sure so wanted you to take a second look first.

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.

3 participants