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

Add missing networking support for Illumos/Solaris as used in quinn #3717

Merged
merged 1 commit into from
Aug 13, 2024

Conversation

psumbera
Copy link
Contributor

No description provided.

@rustbot
Copy link
Collaborator

rustbot commented May 22, 2024

r? @JohnTitor

rustbot has assigned @JohnTitor.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@rustbot
Copy link
Collaborator

rustbot commented May 22, 2024

Some changes occurred in solarish module

cc @jclulow, @pfmooney

@pfmooney
Copy link
Contributor

libc-test still passes on illumos with this change in place

@psumbera
Copy link
Contributor Author

@JohnTitor can you please have look at this? Thank you!

@psumbera
Copy link
Contributor Author

psumbera commented Jul 4, 2024

@JohnTitor is there anything wrong with this pull request?

@psumbera
Copy link
Contributor Author

r? @tgross35

Can you please help with merging of this?

@rustbot rustbot assigned tgross35 and unassigned JohnTitor Aug 12, 2024
src/unix/mod.rs Outdated
Comment on lines 593 to 594
#[cfg_attr(target_os = "illumos", link_name = "__xnet_socket")]
#[cfg_attr(target_os = "solaris", link_name = "__xnet_socket")]
Copy link
Contributor

@tgross35 tgross35 Aug 12, 2024

Choose a reason for hiding this comment

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

Dedupe the same link names (applies a couple other places in these files)

Suggested change
#[cfg_attr(target_os = "illumos", link_name = "__xnet_socket")]
#[cfg_attr(target_os = "solaris", link_name = "__xnet_socket")]
#[cfg_attr(any(target_os = "illumos", target_os = "solaris"), link_name = "__xnet_socket")]

@tgross35
Copy link
Contributor

tgross35 commented Aug 12, 2024

I am not sure what to check against, are there docs/headers online for the constants? Changes themselves look fine to me with one nit above.

Please rebase and squash so the merge commits go away.

(Also, I assume the PR title and commit is meant to say "add missing stuff". Please make this more descriptive, e.g. "Add missing networking support for solaris".)

Comment on lines 2876 to 2887
#[cfg_attr(target_os = "illumos", link_name = "__xnet_bind")]
#[cfg_attr(target_os = "solaris", link_name = "__xnet_bind")]
pub fn bind(socket: ::c_int, address: *const ::sockaddr, address_len: ::socklen_t) -> ::c_int;

pub fn writev(fd: ::c_int, iov: *const ::iovec, iovcnt: ::c_int) -> ::ssize_t;
pub fn readv(fd: ::c_int, iov: *const ::iovec, iovcnt: ::c_int) -> ::ssize_t;

#[cfg_attr(target_os = "illumos", link_name = "__xnet_sendmsg")]
#[cfg_attr(target_os = "solaris", link_name = "__xnet_sendmsg")]
pub fn sendmsg(fd: ::c_int, msg: *const ::msghdr, flags: ::c_int) -> ::ssize_t;
#[cfg_attr(target_os = "illumos", link_name = "__xnet_recvmsg")]
#[cfg_attr(target_os = "solaris", link_name = "__xnet_recvmsg")]
Copy link
Contributor

Choose a reason for hiding this comment

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

For cases where the link_name now applies to both illumos and Solaris, there should be no need for the cfg_attr test while in unix/solarish/mod.rs

@psumbera
Copy link
Contributor Author

I am not sure what to check against, are there docs/headers online for the constants?

The only way how to review header files I can think of is:

  1. Login to Solaris 11.4 machines as they are available at https://portal.cfarm.net/machines/list/
  2. Install Solaris CBE release per https://blogs.oracle.com/solaris/post/announcing-the-first-oracle-solaris-114-cbe

@tgross35
Copy link
Contributor

I don't have a cfarm account so that would take a couple days. Since @pfmooney has already verified illumos, I have no objection to merging.

This should have an update libc-test/semver, otherwise LGTM.

@tgross35 tgross35 changed the title Add missing staff for Illumos/Solaris as used in quinn Add missing networking support for Illumos/Solaris as used in quinn Aug 12, 2024
@tgross35 tgross35 added the stable-nominated This PR should be considered for cherry-pick to libc's stable release branch label Aug 12, 2024
@tgross35
Copy link
Contributor

@rustbot author

@tgross35 tgross35 added this pull request to the merge queue Aug 13, 2024
Merged via the queue into rust-lang:main with commit b496aac Aug 13, 2024
39 checks passed
tgross35 pushed a commit to tgross35/rust-libc that referenced this pull request Aug 17, 2024
(backport <rust-lang#3717>)
(cherry picked from commit 7d2eb94)
@tgross35 tgross35 added stable-applied This PR has been cherry-picked to libc's stable release branch and removed stable-nominated This PR should be considered for cherry-pick to libc's stable release branch labels Aug 17, 2024
@psumbera psumbera deleted the quinn branch November 13, 2024 12:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-author stable-applied This PR has been cherry-picked to libc's stable release branch
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants