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

Revert "Implement fallback for sendmmsg and recvmmsg" #1966

Merged
merged 2 commits into from
Sep 3, 2024

Conversation

mxinden
Copy link
Contributor

@mxinden mxinden commented Aug 14, 2024

This reverts commit e6f1844.

e6f1844 called sendmmsg and recvmmsg through libc::syscall instead of libc::sendmmsg and libc::recvmmsg, thus preventing linking issues on old Android systems where libc::sendmmsg and libc::recvmmsg isn't available.

In #1503 (comment) the suggestion was made to no longer support these old Android systems (API level 16).

This commit reverts e6f1844. Given that sendmmsg support was previously dropped in ee08826, only the recvmmsg calls are reverted.


Alternative to #1964. Given the additional simplicity, my preference is for this change.

quinn-udp/src/unix.rs Outdated Show resolved Hide resolved
@mxinden
Copy link
Contributor Author

mxinden commented Aug 21, 2024

@djc @Ralith do you have some time to give this pull request a review?

Copy link
Member

@djc djc left a comment

Choose a reason for hiding this comment

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

This makes sense to me, but I'll want to wait for @Ralith's opinion.

@mxinden
Copy link
Contributor Author

mxinden commented Aug 27, 2024

Friendly ping @Ralith. Do you have some time to take a look?

Copy link
Collaborator

@Ralith Ralith left a comment

Choose a reason for hiding this comment

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

Do we still need the ENOSYS path and the recvmmsg_fallback definition? Prior to e6f1844 we just called libc::recvmmgs directly rather than wrapping it at all -- can we return to that?

This reverts commit e6f1844.

e6f1844 called `sendmmsg` and `recvmmsg` through `libc::syscall` instead of
`libc::sendmmsg` and `libc::recvmmsg`, thus preventing linking issues on old
Android systems where `libc::sendmmsg` and `libc::recvmmsg` isn't available.

In quinn-rs#1503 (comment) the
decision was made to no longer support these old Android systems (API level 16).

This commit reverts e6f1844. Given that `sendmmsg` support was previously
dropped in ee08826, only the `recvmmsg` calls are reverted.
@mxinden
Copy link
Contributor Author

mxinden commented Sep 2, 2024

Thank you for the review!

I am fine with no longer supporting both:

  1. Android API Level 16 (i.e. Android 4)
  2. Linux Kernel < 2.6.33

(1) allows us to drop indirection libc::syscall(libc::SYS_sendmmsg in favor of a direct call to libc::recvmmsg. (2) allows us to drop recvmsg (one m) fallback.

I updated this pull request accordingly.

@Ralith would you mind taking another look?

Copy link
Collaborator

@Ralith Ralith left a comment

Choose a reason for hiding this comment

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

LGTM! I don't think Rust officially supports targeting linux 2.x anyway.

@djc djc added this pull request to the merge queue Sep 3, 2024
Merged via the queue into quinn-rs:main with commit 8bdbf42 Sep 3, 2024
13 checks passed
@djc
Copy link
Member

djc commented Sep 3, 2024

  • Published quinn-udp v0.5.5 at registry crates-io
  • [new tag] quinn-udp-0.5.5 -> quinn-udp-0.5.5
  • Release notes

@mxinden
Copy link
Contributor Author

mxinden commented Sep 3, 2024

Thank you @djc for the prompt release. Very much appreciated 🙏

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