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

ScmCredentials netbsd implementation. #88025

Merged
merged 1 commit into from
Apr 6, 2022
Merged

ScmCredentials netbsd implementation. #88025

merged 1 commit into from
Apr 6, 2022

Conversation

devnexen
Copy link
Contributor

No description provided.

@rust-highfive
Copy link
Collaborator

r? @Mark-Simulacrum

(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 Aug 14, 2021
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@Mark-Simulacrum
Copy link
Member

r? @joshtriplett perhaps?

@the8472 the8472 mentioned this pull request Aug 22, 2021
Closed
@inquisitivecrystal inquisitivecrystal added T-libs Relevant to the library team, which will review and decide on the PR/issue. O-netbsd Operating system: NetBSD labels Aug 24, 2021
@bjorn3
Copy link
Member

bjorn3 commented Aug 25, 2021

Does the same implementation work on other bsd's like openbsd and freebsd? If so can you add it to those platforms too?

@devnexen
Copy link
Contributor Author

I wish that was the case :-) but for example freebsd is a different beast in that regard.

@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 Sep 13, 2021
@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 3, 2021
@joshtriplett
Copy link
Member

I'm not the right person to review non-trivial BSD changes, in general. I think someone else should take a look at this.

Who are the BSD target maintainers?

@devnexen
Copy link
Contributor Author

devnexen commented Oct 4, 2021

do not know if there is a dedicated one for netbsd, being low tier platform.

@joshtriplett
Copy link
Member

@devnexen All new targets, even tier 3 targets, are supposed to have at least one maintainer. That isn't necessarily the case for older targets, but we should work towards identifying maintainers for existing targets, or considering the possibility of deprecation if there are zero people available to maintain them.

@devnexen
Copy link
Contributor Author

devnexen commented Oct 5, 2021

I see often contributions by unique individuals, no recurring patterns. I know who are the freebsd team and the most visible openbsd expert but netbsd, hard to know...

@JohnCSimon JohnCSimon added I-needs-decision Issue: In need of a decision. S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. O-netbsd Operating system: NetBSD and removed O-netbsd Operating system: NetBSD S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Oct 24, 2021
@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 Nov 16, 2021
@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 Dec 5, 2021
@Dylan-DPC
Copy link
Member

@Mark-Simulacrum any clue who should review BSD changes?

@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 Mar 20, 2022
@Amanieu
Copy link
Member

Amanieu commented Mar 30, 2022

r? @Amanieu

@Amanieu
Copy link
Member

Amanieu commented Mar 31, 2022

@devnexen Have you tried running the test_send_vectored_with_ancillary_to_unix_datagram test from the standard library on netbsd? You can run it with ./x.py test library/std --test-args test_send_vectored_with_ancillary_to_unix_datagram.

Other than that the code looks fine to me.

@rust-log-analyzer

This comment has been minimized.

Sign in to view
@devnexen
Copy link
Contributor Author

@Amanieu after rebasing relevant tests pass.

Amanieu
Amanieu reviewed Apr 3, 2022
View reviewed changes
library/std/src/os/unix/net/ancillary.rs
@@ -252,11 +313,15 @@ impl<'a> Iterator for ScmRights<'a> {
/// This control message contains unix credentials.
///
/// The level is equal to `SOL_SOCKET` and the type is equal to `SCM_CREDENTIALS` or `SCM_CREDS`.
#[cfg(any(doc, target_os = "android", target_os = "linux",))]
Copy link
Member

Choose a reason for hiding this comment

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

We want ScmCredentials to appear in the docs even on unsupported targets. That's what the fake libc module is for.

Copy link
Member

Choose a reason for hiding this comment

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

The best way to achieve that is probably to have a separate definition when building docs on an unsupported target:

#[cfg(all(doc, not(target_os = "linux", target_os = "android", target_os = "netbsd"))]
#[unstable(feature = "unix_socket_ancillary_data", issue = "76915")]
#[derive(Clone)]
pub struct SocketCred(());

Amanieu
Amanieu reviewed Apr 3, 2022
View reviewed changes
library/std/src/os/unix/net/ancillary.rs Outdated
@@ -15,6 +15,7 @@ use crate::sys::net::Socket;
mod libc {
pub use libc::c_int;
pub struct ucred;
pub struct sockcred;
Copy link
Member

Choose a reason for hiding this comment

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

This is no longer needed. Also in the cfg above you should exclude netbsd.

library/std/src/os/unix/net/ancillary.rs Outdated
#[derive(Clone)]
pub struct SocketCred(libc::sockcred);

#[cfg(any(target_os = "android", target_os = "linux",))]
Copy link
Member

Choose a reason for hiding this comment

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

Add doc here so the methods appear in the docs.

@devnexen devnexen force-pushed the netbsd_scm_creds branch 2 times, most recently from 1f6dbf1 to ed6e070 Compare April 3, 2022 18:13
@rust-log-analyzer

This comment has been minimized.

Sign in to view
Amanieu
Amanieu reviewed Apr 3, 2022
View reviewed changes
library/std/src/os/unix/net/ancillary.rs Outdated
@@ -234,6 +245,62 @@ impl SocketCred {
}
}

#[doc(cfg(target_os = "netbsd"))]
Copy link
Member

Choose a reason for hiding this comment

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

This line is not needed.

@Amanieu
Copy link
Member

Amanieu commented Apr 5, 2022

@bors r+ rollup=iffy

@bors
Copy link
Contributor

bors commented Apr 5, 2022

📌 Commit 23e6314 has been approved by Amanieu

@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 Apr 5, 2022
@Dylan-DPC Dylan-DPC mentioned this pull request Apr 5, 2022
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Apr 5, 2022
@bors
Auto merge of rust-lang#95702 - Dylan-DPC:rollup-793rz6v, r=Dylan-DPC
bbe9d27 
Rollup of 8 pull requests

Successful merges:

 - rust-lang#88025 (ScmCredentials netbsd implementation.)
 - rust-lang#95473 (track individual proc-macro expansions in the self-profiler)
 - rust-lang#95547 (caution against ptr-to-int transmutes)
 - rust-lang#95585 (Explain why `&T` is cloned when `T` is not `Clone`)
 - rust-lang#95591 (Use revisions to track NLL test output (part 1))
 - rust-lang#95663 (diagnostics: give a special note for unsafe fn / Fn/FnOnce/FnMut)
 - rust-lang#95673 (:arrow_up: rust-analyzer)
 - rust-lang#95681 (resolve: Fix resolution of empty paths passed from rustdoc)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors
Copy link
Contributor

bors commented Apr 6, 2022

⌛ Testing commit 23e6314 with merge bbe9d27...

@bors bors merged commit d2e1e6d into rust-lang:master Apr 6, 2022
@rustbot rustbot added this to the 1.62.0 milestone Apr 6, 2022
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

Assignees

@Amanieu Amanieu

Labels
I-needs-decision Issue: In need of a decision. O-netbsd Operating system: NetBSD S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-libs Relevant to the library team, which will review and decide on the PR/issue.
Projects
None yet
Milestone
1.62.0
Development

Successfully merging this pull request may close these issues.