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

socket ancillary data implementation for FreeBSD (from 13 and above). #91793

Merged
merged 1 commit into from
Mar 28, 2023
Merged

socket ancillary data implementation for FreeBSD (from 13 and above). #91793

merged 1 commit into from
Mar 28, 2023
+209 −21

Conversation

devnexen
Copy link
Contributor

introducing new build config as well.

@rust-highfive
Copy link
Collaborator

r? @joshtriplett

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Dec 11, 2021
@apiraino apiraino added the T-libs Relevant to the library team, which will review and decide on the PR/issue. label Dec 14, 2021
@bors
Copy link
Contributor

bors commented Dec 18, 2021

☔ The latest upstream changes (presumably #92062) made this pull request unmergeable. Please resolve the merge conflicts.

@sanxiyn sanxiyn added the O-freebsd Operating system: FreeBSD label Jan 5, 2022
@camelid camelid added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jan 25, 2022
@JohnCSimon JohnCSimon added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Feb 27, 2022
@devnexen
Copy link
Contributor Author

ping :)

@rust-log-analyzer

This comment has been minimized.

Sign in to view
@devnexen devnexen force-pushed the anc_data_fbsd branch 2 times, most recently from 24f96d0 to 296c1f9 Compare March 19, 2022 08:45
@bors
Copy link
Contributor

bors commented Apr 6, 2022

☔ The latest upstream changes (presumably #95702) made this pull request unmergeable. Please resolve the merge conflicts.

@devnexen
Copy link
Contributor Author

cc @Amanieu :)

Amanieu
Amanieu reviewed May 3, 2022
View reviewed changes
Copy link
Member

@Amanieu Amanieu left a comment

Choose a reason for hiding this comment

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

LGTM overall but I'd like someone more familiar with FreeBSD to have a second look.

cc @asomers

library/std/src/os/freebsd/fs.rs Outdated
panic!("as_raw_stat not supported with FreeBSD 12 ABI");
#[cfg(not(freebsd12))]
#[cfg(not(any(freebsd12, freebsd13)))]
Copy link
Member

Choose a reason for hiding this comment

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

If build.rs defines both freebsd12 and freebsd13 then this shouldn't be needed.

library/std/src/sys/unix/net.rs Outdated
@@ -430,6 +430,17 @@ impl Socket {
Ok(passcred != 0)
}

#[cfg(all(target_os = "freebsd", freebsd13))]
Copy link
Member

Choose a reason for hiding this comment

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

#[cfg(freebsd13)] should be enough here.

library/std/src/os/unix/net/stream.rs Outdated
target_os = "android",
target_os = "linux",
target_os = "dragonfly",
all(target_os = "freebsd", freebsd13)
Copy link
Member

Choose a reason for hiding this comment

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

And here too.

asomers
asomers suggested changes May 3, 2022
View reviewed changes
Copy link
Contributor

@asomers asomers left a comment

Choose a reason for hiding this comment

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

Looks mostly ok overall. However, gating this feature on freebsd13 means that it won't be available on builds from rustup, probably not for years. However, all relevant symbols are defined in libc, which means the code could be successfully built when using the freebsd12 (or 11, or 10) ABI. Then, if you try to use it on freebsd12, you'll just get an ENOPROTOOPT error. IMHO, that's not so bad. So why not enable it on all freebsd builds? That way it will be available to rustup users.

Also, you should adjust the target_os gate on some of the functions in library/std/src/os/unix/net/tests.rs .

library/std/src/sys/unix/net.rs

#[cfg(all(target_os = "freebsd", freebsd13))]
pub fn passcred(&self) -> io::Result<bool> {
let passcred: libc::c_int = getsockopt(self, libc::AF_LOCAL, libc::LOCAL_CREDS_PERSISTENT)?;
Copy link
Contributor

Choose a reason for hiding this comment

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

Why use LOCAL_CREDS_PERSISTENT for FreeBSD, but LOCAL_CREDS for NetBSD?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because LOCAL_CREDS_PERSISTENT is used in conjunction with sockcred2 as opposed to LOCAL_CREDS and sockcred which does not get the process id on FreeBSD. NetBSD does not have the same history regarding this feature.

library/std/src/os/unix/net/stream.rs Outdated
any(
target_os = "android",
target_os = "linux",
target_os = "dragonfly",
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
target_os = "dragonfly",
target_os = "netbsd",

library/std/src/os/unix/net/stream.rs Outdated
not(any(
target_os = "android",
target_os = "linux",
target_os = "dragonfly",
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
target_os = "dragonfly",
target_os = "netbsd",

library/std/src/os/unix/net/datagram.rs Outdated
#[cfg_attr(any(target_os = "android", target_os = "linux"), doc = "```no_run")]
#[cfg_attr(not(any(target_os = "android", target_os = "linux")), doc = "```ignore")]
#[cfg_attr(
any(target_os = "android", target_os = "linux", all(target_os = "freebsd", freebsd13),),
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
any(target_os = "android", target_os = "linux", all(target_os = "freebsd", freebsd13),),
any(target_os = "android", target_os = "linux", target_os = "netbsd", all(target_os = "freebsd", freebsd13),),

library/std/src/os/unix/net/datagram.rs Outdated
not(any(
target_os = "android",
target_os = "linux",
all(target_os = "freebsd", freebsd13)
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
all(target_os = "freebsd", freebsd13)
target_os = "netbsd",
all(target_os = "freebsd", freebsd13)

Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
all(target_os = "freebsd", freebsd13)
target_os = "freebsd"

And do the same on lines 884 and 896 as well.

library/std/src/os/unix/net/ancillary.rs
@@ -270,6 +360,7 @@ impl SocketCred {
}

/// Get the current PID.
#[must_use]
Copy link
Contributor

Choose a reason for hiding this comment

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

Similarly, why put #[must_use] here, but not on get_pid or get_uid?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

good point here

Copy link
Member

Choose a reason for hiding this comment

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

This change is in the netbsd implementation. Also it's missing from the other get_* in the netbsd implementation.

library/std/src/os/unix/net/ancillary.rs Outdated
@@ -326,11 +417,15 @@ pub struct ScmCredentials<'a>(AncillaryDataIter<'a, ()>);
#[unstable(feature = "unix_socket_ancillary_data", issue = "76915")]
pub struct ScmCredentials<'a>(AncillaryDataIter<'a, libc::ucred>);

#[cfg(freebsd13)]
Copy link
Contributor

Choose a reason for hiding this comment

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

You need to change the #[cfg( statement on line 409 or else this will be defined twice.

library/std/src/os/unix/net/ancillary.rs Outdated
@@ -605,7 +702,7 @@ impl<'a> SocketAncillary<'a> {
/// The function returns `true` if there was enough space in the buffer.
/// If there was not enough space then no credentials was appended.
/// Technically, that means this operation adds a control message with the level `SOL_SOCKET`
/// and type `SCM_CREDENTIALS` or `SCM_CREDS`.
/// and type `SCM_CREDENTIALS` or `SCM_CREDS` or `SCM_CREDS2`.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
/// and type `SCM_CREDENTIALS` or `SCM_CREDS` or `SCM_CREDS2`.
/// and type `SCM_CREDENTIALS`, `SCM_CREDS`, or `SCM_CREDS2`.

library/std/src/os/unix/net/ancillary.rs Outdated
Comment on lines 702 to 703
/// The function returns `true` if there was enough space in the buffer.
/// If there was not enough space then no credentials was appended.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
/// The function returns `true` if there was enough space in the buffer.
/// If there was not enough space then no credentials was appended.
/// The function returns `true` if there is enough space in the buffer.
/// If there is not enough space then no credentials will be appended.

library/std/src/os/unix/net/ancillary.rs Outdated
@@ -623,6 +720,19 @@ impl<'a> SocketAncillary<'a> {
)
}

#[cfg(freebsd13)]
Copy link
Contributor

Choose a reason for hiding this comment

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

It's inconsistent to redefine the function for FreeBSD, but share a definition for Linux and NetBSD. There's only one line of difference, so why not share a single definition for all OSes?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

good point I forgot but I started this change before the netbsd changes was ever committed most likely this reason is gone :)

@rust-log-analyzer

This comment has been minimized.

Sign in to view
asomers
asomers suggested changes May 3, 2022
View reviewed changes
library/std/src/os/unix/net/datagram.rs Outdated
target_os = "android",
target_os = "linux",
target_os = "netbsd",
all(target_os = "freebsd", freebsd13),
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
all(target_os = "freebsd", freebsd13),
target_os = "freebsd",

library/std/src/os/unix/net/datagram.rs Outdated
not(any(
target_os = "android",
target_os = "linux",
all(target_os = "freebsd", freebsd13)
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
all(target_os = "freebsd", freebsd13)
target_os = "freebsd"

And do the same on lines 884 and 896 as well.

@rust-log-analyzer

This comment has been minimized.

Sign in to view
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@devnexen devnexen force-pushed the anc_data_fbsd branch 2 times, most recently from e2046e1 to 91a6898 Compare May 26, 2022 12:06
@devnexen
Copy link
Contributor Author

ping :)

@JohnCSimon JohnCSimon added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jul 3, 2022
@JohnCSimon
Copy link
Member

Ping:
@joshtriplett - what is your status on this review?

@JohnCSimon JohnCSimon added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Oct 8, 2022
@ChrisDenton
Copy link
Contributor

I think this is ready to be merged if freebsd folks are still happy with it?

@asomers
Copy link
Contributor

asomers commented Mar 26, 2023

Nothing's changed on the OS side. I still approve, assuming none of the force-pushes broke something (I didn't closely review the force-pushes after 8-May-2022)

@ChrisDenton
Copy link
Contributor

Ok I'm happy to merge this, CI permitting. The only thing I'd ask, @devnexen, is that you squish the two commits into one.

@rustbot rustbot added the T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) label Mar 27, 2023
@rustbot
Copy link
Collaborator

rustbot commented Mar 27, 2023

Hey! It looks like you've submitted a new PR for the library teams!

If this PR contains changes to any rust-lang/rust public library APIs then please comment with @rustbot label +T-libs-api -T-libs to tag it appropriately. If this PR contains changes to any unstable APIs please edit the PR description to add a link to the relevant API Change Proposal or create one if you haven't already. If you're unsure where your change falls no worries, just leave it as is and the reviewer will take a look and make a decision to forward on if necessary.

Examples of T-libs-api changes:

  • Stabilizing library features
  • Introducing insta-stable changes such as new implementations of existing stable traits on existing stable types
  • Introducing new or changing existing unstable library APIs (excluding permanently unstable features / features without a tracking issue)
  • Changing public documentation in ways that create new stability guarantees
  • Changing observable runtime behavior of library APIs

@ChrisDenton
Copy link
Contributor

ChrisDenton commented Mar 27, 2023 •
edited
Loading

Edit: bors not responding

@ChrisDenton ChrisDenton reopened this Mar 27, 2023
@ChrisDenton
Copy link
Contributor

@bors r+

@bors
Copy link
Contributor

bors commented Mar 27, 2023

📌 Commit ed5c0f6 has been approved by ChrisDenton

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Mar 27, 2023
@Nilstrieb Nilstrieb mentioned this pull request Mar 28, 2023
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Mar 28, 2023
@bors
Auto merge of rust-lang#109692 - Nilstrieb:rollup-hq65rps, r=Nilstrieb
478cbb4 
Rollup of 8 pull requests

Successful merges:

 - rust-lang#91793 (socket ancillary data implementation for FreeBSD (from 13 and above).)
 - rust-lang#92284 (Change advance(_back)_by to return the remainder instead of the number of processed elements)
 - rust-lang#102472 (stop special-casing `'static` in evaluation)
 - rust-lang#108480 (Use Rayon's TLV directly)
 - rust-lang#109321 (Erase impl regions when checking for impossible to eagerly monomorphize items)
 - rust-lang#109470 (Correctly substitute GAT's type used in `normalize_param_env` in `check_type_bounds`)
 - rust-lang#109562 (Update ar_archive_writer to 0.1.3)
 - rust-lang#109629 (remove obsolete `givens` from regionck)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 857b631 into rust-lang:master Mar 28, 2023
@rustbot rustbot added this to the 1.70.0 milestone Mar 28, 2023
@dankm dankm mentioned this pull request Jun 6, 2023
Merged
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Jun 17, 2023
@matthiaskrgr
Rollup merge of rust-lang#112352 - dankm:fbsd_doc_fix, r=thomcc
1051e8f 
Fix documentation build on FreeBSD

After the socket ancillary data implementation was introduced, the documentation build was broken on FreeBSD hosts, add the same workaround as for the existing implementations.

Fixes the doc build after rust-lang#91793
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Jun 17, 2023
@matthiaskrgr
Rollup merge of rust-lang#112352 - dankm:fbsd_doc_fix, r=thomcc
d2120b7 
Fix documentation build on FreeBSD

After the socket ancillary data implementation was introduced, the documentation build was broken on FreeBSD hosts, add the same workaround as for the existing implementations.

Fixes the doc build after rust-lang#91793
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Amanieu Amanieu Amanieu left review comments

@asomers asomers asomers approved these changes

Assignees

@joshtriplett joshtriplett

Labels
O-freebsd Operating system: FreeBSD S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) T-libs Relevant to the library team, which will review and decide on the PR/issue.
Projects
None yet
Milestone
1.70.0
Development

Successfully merging this pull request may close these issues.

None yet