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

ancillary data size and doc improvement #88425

Closed
wants to merge 2 commits into from
Closed

ancillary data size and doc improvement #88425

wants to merge 2 commits into from

Conversation

beroal
Copy link

@beroal beroal commented Aug 28, 2021

Functions calculating the size of a control message. Improvement of the doc for SocketAncillary and the main doc. Clarification that add_fds and add_creds add one control message.
r? @LinkTed

@beroal beroal mentioned this pull request Aug 28, 2021
Open
the8472
the8472 reviewed Aug 28, 2021
View reviewed changes
library/std/src/os/unix/net/ancillary.rs Outdated Show resolved Hide resolved
library/std/src/os/unix/net/ancillary.rs Show resolved Hide resolved
library/std/src/os/unix/net/ancillary.rs Outdated Show resolved Hide resolved
@inquisitivecrystal
Copy link
Contributor

It appears that LinkTed isn't a team member, and thus highfive wouldn't assign this to them. In the interest of having it assigned to someone...

r? @the8472

@inquisitivecrystal inquisitivecrystal added A-io Area: std::io, std::fs, std::net and std::path O-unix Operating system: Unix-like T-libs Relevant to the library team, which will review and decide on the PR/issue. S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Aug 31, 2021
the8472
the8472 reviewed Sep 5, 2021
View reviewed changes
library/std/src/os/unix/net/ancillary.rs
@@ -407,7 +434,13 @@ pub struct SocketAncillary<'a> {
}

impl<'a> SocketAncillary<'a> {
/// Create an ancillary data with the given buffer.
/// Create a struct for accessing an ancillary data packet in `buffer`.
Copy link
Member

Choose a reason for hiding this comment

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

accessing [...] in buffer

While this isn't wrong I think it could be misunderstood that the purpose of this constructor is to pass in a buffer that already contains messages and then deserializing it.

The next sentence clarifies it but I think we can still do better. Maybe something like

Create a struct holding ancillary data. buffer is used to store the serialized data without dynamic allocations.

Exposing the buffer is mostly an optimization to play nice with low-level network code that uses preallocated buffers.

Copy link
Author

Choose a reason for hiding this comment

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

I'm trying to avoid saying that the struct holds ancillary data because it doesn't.

Copy link
Member

Choose a reason for hiding this comment

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

Create a struct to process ancillary data. buffer is used to store the serialized data without dynamic allocations.

the8472
the8472 reviewed Sep 5, 2021
View reviewed changes
library/std/src/os/unix/net/ancillary.rs
@@ -369,7 +384,19 @@ impl<'a> Iterator for Messages<'a> {
}
}

/// A Unix socket Ancillary data struct.
/// A struct for accessing a socket ancillary data packet.
Copy link
Member

Choose a reason for hiding this comment

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

Same as the comment on new

the8472
the8472 reviewed Sep 5, 2021
View reviewed changes
library/std/src/os/unix/net/ancillary.rs
@@ -418,6 +451,7 @@ impl<'a> SocketAncillary<'a> {
/// let mut ancillary_buffer = [0; 128];
/// let mut ancillary = SocketAncillary::new(&mut ancillary_buffer[..]);
/// ```
// FIXME: add size example
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 meant for a future PR or is this PR incomplete?

Copy link
Author

Choose a reason for hiding this comment

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

It's incomplete.

@the8472 the8472 added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Sep 8, 2021
@JohnCSimon
Copy link
Member

Ping from triage:
@beroal can you please post your status on this PR?

@beroal
Copy link
Author

beroal commented Sep 27, 2021

Ping from triage:
@beroal can you please post your status on this PR?

I can't finish this PR now. ancillary_data_size needs examples. The form of examples depends on whether the const-extern-fn feature of libc is stable. If I include examples now, I'll need to send another PR with new examples when const-extern-fn is stabilized. Too much hassle for such a small change.

@JohnCSimon JohnCSimon added the S-inactive Status: Inactive and waiting on the author. This is often applied to closed PRs. label Oct 19, 2021
@JohnCSimon
Copy link
Member

Triage:
@beroal I'm closing this as inactive, feel free to reopen when you're ready to continue. Thanks.

@JohnCSimon JohnCSimon closed this Nov 8, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@the8472 the8472 the8472 left review comments

@LinkTed LinkTed LinkTed left review comments

Assignees

@the8472 the8472

Labels
A-io Area: std::io, std::fs, std::net and std::path O-unix Operating system: Unix-like S-inactive Status: Inactive and waiting on the author. This is often applied to closed PRs. S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. T-libs Relevant to the library team, which will review and decide on the PR/issue.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@beroal @inquisitivecrystal @JohnCSimon @the8472 @LinkTed