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

Support TIMESTAMPNS #1402

Merged
merged 1 commit into from
Apr 8, 2021
Merged

Support TIMESTAMPNS #1402

merged 1 commit into from
Apr 8, 2021

Conversation

WiSaGaN
Copy link
Contributor

@WiSaGaN WiSaGaN commented Mar 20, 2021

This adds support of linux TIMESTAMPNS.
The code is mostly copied paste from #663

@WiSaGaN WiSaGaN force-pushed the support-timestampns branch 3 times, most recently from d2f691a to 736d9a3 Compare March 20, 2021 14:36
asomers
asomers requested changes Mar 20, 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.

It looks pretty good so far. But why did you decide to write the test without the usual test harness? It's preferable to use the regular test harness if possible. In fact, right now your test won't even be compiled, because you didn't add it to Cargo.toml.

@WiSaGaN
Copy link
Contributor Author

WiSaGaN commented Mar 21, 2021

It looks pretty good so far. But why did you decide to write the test without the usual test harness? It's preferable to use the regular test harness if possible. In fact, right now your test won't even be compiled, because you didn't add it to Cargo.toml.

Thanks for the review! Is there a way to use the macro cmsg_space in unit tests? It contains use unix::..., which I can't make it work within a unit test.

asomers
asomers reviewed Mar 21, 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.

I assume you mean cmsg_space!? It's currently used by the functional tests in test/sys/test_socket.rs. There aren't any unit tests that currently use it, but I don't see why there couldn't be. What problem are you having?

CHANGELOG.md Outdated Show resolved Hide resolved
@WiSaGaN
Copy link
Contributor Author

WiSaGaN commented Mar 21, 2021 •
edited
Loading

I assume you mean cmsg_space!? It's currently used by the functional tests in test/sys/test_socket.rs. There aren't any unit tests that currently use it, but I don't see why there couldn't be. What problem are you having?

It contains self-referential "use unix::*" statement. Apparently, when in "nix" crate, you can only reference it as "use crate::..." but not "use nix:...".

@asomers
Copy link
Member

asomers commented Mar 22, 2021

I assume you mean cmsg_space!? It's currently used by the functional tests in test/sys/test_socket.rs. There aren't any unit tests that currently use it, but I don't see why there couldn't be. What problem are you having?

It contains self-referential "use unix::*" statement. Apparently, when in "nix" crate, you can only reference it as "use crate::..." but not "use nix:...".

Well, that sounds like something you can fix.

@WiSaGaN
Copy link
Contributor Author

WiSaGaN commented Mar 22, 2021 •
edited
Loading

I assume you mean cmsg_space!? It's currently used by the functional tests in test/sys/test_socket.rs. There aren't any unit tests that currently use it, but I don't see why there couldn't be. What problem are you having?

It contains self-referential "use unix::*" statement. Apparently, when in "nix" crate, you can only reference it as "use crate::..." but not "use nix:...".

Well, that sounds like something you can fix.

I think it would be better to do that in a separate pull request. In the mean time, I can add a todo of moving the integration test to unit test after that is done.

Edit: created #1405 to track the cmsg_space! fix.

asomers
asomers reviewed Mar 23, 2021
View reviewed changes
Cargo.toml Outdated Show resolved Hide resolved
@WiSaGaN WiSaGaN force-pushed the support-timestampns branch 2 times, most recently from 15b1987 to a855796 Compare March 23, 2021 04:11
asomers
asomers requested changes Mar 24, 2021
View reviewed changes
src/sys/socket/mod.rs Outdated Show resolved Hide resolved
@WiSaGaN
Copy link
Contributor Author

WiSaGaN commented Apr 6, 2021

libc recently updated its socketopt consts: rust-lang/libc@682eba6

We will wait for the release of this to have a more updated lists of arch here.

@WiSaGaN
Copy link
Contributor Author

WiSaGaN commented Apr 7, 2021 •
edited
Loading

After upgrading libc to 0.2.93, now the support has been added to all linux platforms. Tests however are restricted to x86-64, non musl due to suspected QEMU issue.

@WiSaGaN
Copy link
Contributor Author

WiSaGaN commented Apr 7, 2021

Please feel free to squash all the commits into one if it is ready to merge.

asomers
asomers reviewed Apr 7, 2021
View reviewed changes
test/sys/test_socket.rs Outdated Show resolved Hide resolved
@asomers
Copy link
Member

asomers commented Apr 8, 2021

Ok, this looks good now. But you'll have to squash yourself. Nix uses the bors merge bot, which is incompatible with Github's squash-and-merge button.

@WiSaGaN
Copy link
Contributor Author

WiSaGaN commented Apr 8, 2021

Sqaushed. Now it is ready to merge.

asomers
asomers approved these changes Apr 8, 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 Apr 8, 2021

Build succeeded:

@bors bors bot merged commit 68d01f0 into nix-rust:master Apr 8, 2021
@WiSaGaN WiSaGaN mentioned this pull request Apr 8, 2021
Closed
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
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@WiSaGaN @asomers