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

feat: Add glibc::SOF_TIMESTAMPING_* support #1547

Merged
merged 1 commit into from
Dec 24, 2021
Merged

feat: Add glibc::SOF_TIMESTAMPING_* support #1547

merged 1 commit into from
Dec 24, 2021

Conversation

pacak
Copy link
Contributor

@pacak pacak commented Sep 29, 2021

Support for kernel and hardware receive timestamps

@pacak
Copy link
Contributor Author

pacak commented Sep 30, 2021

Displays strange results on a machine that supposed to support them, I'll reopen when I figure out what's going on.

@pacak pacak closed this Sep 30, 2021
@pacak pacak reopened this Sep 30, 2021
@pacak
Copy link
Contributor Author

pacak commented Sep 30, 2021

False alarm, I wasn't allocating enough memory with cmsg_space!.

asomers
asomers requested changes Oct 1, 2021
View reviewed changes
Copy link
Member

@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.

Could you add some tests?

src/sys/socket/sockopt.rs Outdated Show resolved Hide resolved
src/sys/socket/mod.rs Outdated Show resolved Hide resolved
@pacak
Copy link
Contributor Author

pacak commented Oct 1, 2021

Could you add some tests?

Are there any existing tests that look similar to what you want to get for this code?

docs please

Sure

Instead of taking a u32 argument, you should use libc_bitflags! to define a custom type.

Will take a look

@asomers
Copy link
Member

asomers commented Oct 1, 2021

Could you add some tests?

Are there any existing tests that look similar to what you want to get for this code?

A good test should set the sockopt, and receive a cmsg with the timestamping structure in it. Maybe similar to what test_local_peercred_stream does.

@pacak pacak force-pushed the master branch 3 times, most recently from 72009e2 to ba01693 Compare October 1, 2021 08:00
@pacak
Copy link
Contributor Author

pacak commented Oct 1, 2021

I'm not quite sure what is it complaining about - no changes in src/dir.rs in my PR. I can make a variant of proposed changes or ask clippy to shut up, up to you.

@pacak pacak requested a review from asomers October 3, 2021 22:38
asomers
asomers requested changes Oct 6, 2021
View reviewed changes
Copy link
Member

@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.

Nice. You're almost there. Don't forget to add a CHANGELOG entry too

src/sys/socket/mod.rs Show resolved Hide resolved
src/sys/socket/mod.rs Outdated Show resolved Hide resolved
test/sys/test_socket.rs Outdated Show resolved Hide resolved
@pacak pacak requested a review from asomers October 6, 2021 05:26
asomers
asomers requested changes Oct 8, 2021
View reviewed changes
CHANGELOG.md Outdated Show resolved Hide resolved
@pacak pacak requested a review from asomers October 9, 2021 04:46
asomers
asomers approved these changes Oct 16, 2021
View reviewed changes
Copy link
Member

@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.

LGTM. But I'm going to hold off merging for now, because there are a few bug fix PRs that should merge before any new features.

@asomers asomers added this to the 0.24.0 milestone Oct 16, 2021
@asomers
Copy link
Member

asomers commented Dec 21, 2021

@pacak needs a rebase

@pacak
Copy link
Contributor Author

pacak commented Dec 21, 2021

Done. And (yay) tests are passing.

@asomers
Copy link
Member

asomers commented Dec 22, 2021

Sorry, @pacak but it's conflicting again.

@pacak
Copy link
Contributor Author

pacak commented Dec 24, 2021 via email

asomers
asomers approved these changes Dec 24, 2021
View reviewed changes
Copy link
Member

@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.

bors r+

@bors
Copy link
Contributor

bors bot commented Dec 24, 2021

Build succeeded:

@bors bors bot merged commit a392647 into nix-rust:master Dec 24, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@asomers asomers asomers approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
0.24.0
Development

Successfully merging this pull request may close these issues.
2 participants
@pacak @asomers