Skip to content

Conversation

vi
Copy link
Contributor

@vi vi commented Feb 12, 2019

No description provided.

@rust-highfive
Copy link

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @gnzlbg (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

@asomers
Copy link
Contributor

asomers commented Feb 12, 2019

These constants are defined on FreeBSD 12, too. You should add them to all platforms that define them, not just Linux.

@vi
Copy link
Contributor Author

vi commented Feb 12, 2019

Is only FreeBSD missing? How do I know exhaustive list of platforms I need to add? Is just googling sufficient or one supposed to actually run a fresh release in a VM and check if it's there or not?

@asomers
Copy link
Contributor

asomers commented Feb 12, 2019

You aren't expected to run your own VM for every single platform. That's CI's job. But figuring out which platforms support a certain symbol is something that you can do. The easiest way is to check online man pages. That works well when binding functions. For constants you may need to look at the sources. Here are some of the resources I use:
https://www.freebsd.org/cgi/man.cgi
https://svnweb.freebsd.org/base/head/
http://netbsd.gw.com/cgi-bin/man-cgi?++NetBSD-current
http://cvsweb.netbsd.org/bsdweb.cgi/src/?only_with_tag=MAIN
https://man.openbsd.org/
https://cvsweb.openbsd.org/cgi-bin/cvsweb/src/
https://leaf.dragonflybsd.org/cgi/web-man
https://gitweb.dragonflybsd.org/?p=dragonfly.git;a=summary
https://github.com/illumos/illumos-gate

It's hard to find definitive answers for Linux because it's so fragmented. But a VM can give pretty good answers, even if they aren't perfect. The hardest platform to research is OSX. I pretty much use Google for OSX.

@gnzlbg
Copy link
Contributor

gnzlbg commented Feb 12, 2019

The hardest platform to research is OSX. I pretty much use Google for OSX.

For MacOS X you can search for the MacOSXSDK headers, e.g., this repo contains most of them: https://github.com/phracker/MacOSX-SDKs The only thing to keep in mind is that for i686-apple-darwin macosx headers types sometimes use #pragma pack but that's hidden behind macros (CI will tell you about this though).

Maybe we should compile some of this information to the contribution docs.

@gnzlbg
Copy link
Contributor

gnzlbg commented Feb 13, 2019

@bors: r+

@bors
Copy link
Contributor

bors commented Feb 13, 2019

📌 Commit 4d0694c has been approved by gnzlbg

@bors
Copy link
Contributor

bors commented Feb 13, 2019

⌛ Testing commit 4d0694c with merge a8a817a...

bors added a commit that referenced this pull request Feb 13, 2019
Add IP[V6]_[RECV]ORIGDSTADDR for Linux and Android
@bors
Copy link
Contributor

bors commented Feb 13, 2019

☀️ Test successful - checks-cirrus, checks-travis, status-appveyor
Approved by: gnzlbg
Pushing a8a817a to master...

@bors bors merged commit 4d0694c into rust-lang:master Feb 13, 2019
@asomers
Copy link
Contributor

asomers commented Feb 13, 2019

Why did you merge the PR even though it was incomplete, @gnzlbg ?

@gnzlbg
Copy link
Contributor

gnzlbg commented Feb 13, 2019

I thought we could add the FreeBSD 12 constants in a subsequent PR (as in, no need to block this part from landing on that).

@vi
Copy link
Contributor Author

vi commented Feb 13, 2019

I tried searching superficially for IP_ORIGDSTADDR using provided links and found nothing relevant for any of those apart from FreeBSD.

But looking at https://reviews.freebsd.org/D9235 and git grep-ping other constants in libc source code releals lack of not just ORIGDSTADDR-related things, but also previously numbered constants like IP_BINDMULTI, IP_RSS_LISTEN_BUCKET. Shall those be added along the way?

For IPV6_* it is even more pronounced: by selectively searching mentioned constnats I fail to find things higher than IPV6_V6ONLY (27). Shall all of them also be shoveled into src/unix/bsd/freebsdlike/freebsd/mod.rs? Or it needs more elaboration to distribute between unix/bsd/freebsdlike/freebsd, unix/bsd/freebsdlike, unix/bsd and unix?


Are those things supposed to be auto-generated from headers (or closely track platorm's respective header files) or each item should be judged on case-by-case basis?

@asomers
Copy link
Contributor

asomers commented Feb 13, 2019

Yeah, libc has plenty of missing definitions. You don't need to worry about fixing all of them, but it's good not to make things worse. You can CC me as a reviewer if you want.

vi added a commit to vi/libc that referenced this pull request Feb 13, 2019
bors added a commit that referenced this pull request Feb 18, 2019
Add IP_ORIGDSTADDR and others on FreeBSD.

A follow-up from #1252.

CC @asomers.

r? @gnzlbg
bors added a commit that referenced this pull request Feb 18, 2019
Add IP_ORIGDSTADDR and others on FreeBSD.

A follow-up from #1252.

CC @asomers.

r? @gnzlbg
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.

5 participants