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.

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

Add routing socket type on macOS #1867

Merged
merged 1 commit into from
Nov 29, 2022
Merged

Conversation

pinkisemils
Copy link
Contributor

This is a small change to add the routing socket type to the list of socket types one can open with nix. I've added a smoke test to see that a socket of such type can actually be opened, but I'm not sure if such a test belongs in the codebase here.

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 think this is reasonable. But how does one use a routing socket? Do you need to bind and/or connect with it? If so, then Nix support won't be complete without those. Also, it needs a CHANGELOG entry.

@pinkisemils
Copy link
Contributor Author

pinkisemils commented Nov 21, 2022

There's no reason to connect or bind the socket as it can only ever be connected to the kernel to talk to/about the routing table, and it is ready to send and receive messages as soon as it's created via socket. It's used in the route CLI, e.g. route get 8.8.8.8 or route monitor.

Having used this for a bit, it seems that there are also some sockaddr_x types that are missing, I'll see about adding those too in a separate PR.

@pinkisemils
Copy link
Contributor Author

I now realize that these changes might also be relevant for FreeBSD and friends - but I don't have a FreeBSD VM handy to test my changes.

@asomers
Copy link
Member

asomers commented Nov 21, 2022

It looks to me like you should enable this for macos, ios, freebsd, dragonfly, openbsd, netbsd, linux, android, illumos, haiku, and fuchsia. In fact, it's everywhere but Redox. But on Linux and Android, it's an alias for AF_NETLINK. Why don't you try enabling it for all of those, and let CI test it for you? We have full testing in CI for FreeBSD, OSX, and Linux.

@pinkisemils pinkisemils force-pushed the add-pf-route-socket branch 2 times, most recently from 158da17 to 14e6451 Compare November 25, 2022 10:18
@pinkisemils
Copy link
Contributor Author

It seems that it does just work on most OS'es. I elected to not define it on Linux, since the same value is already used for netlink sockets (and they are kind of the same kind of a thing). Should I remove the test that tries to open the routing socket, or should that stay? I think it doesn't really test any logic, just that a socket of this type can be opened.

@asomers
Copy link
Member

asomers commented Nov 25, 2022

I think it's ok to leave the test. It helps us validate which platforms have routing sockets. I'll create another PR to fix the clippy failure.

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.

The CI failure on uclibc is fixed on master.

bors r+

@bors
Copy link
Contributor

bors bot commented Nov 29, 2022

Merge conflict.

@pinkisemils
Copy link
Contributor Author

I've rebased and it passes all the checks now.

CHANGELOG.md Outdated Show resolved Hide resolved
CHANGELOG.md Outdated
@@ -22,6 +22,9 @@ This project adheres to [Semantic Versioning](https://semver.org/).
- Added `MntFlags` and `unmount` on all of the BSDs.
- Added `any()` and `all()` to `poll::PollFd`.
([#1877](https://github.com/nix-rust/nix/pull/1877))
- Add `PF_ROUTE` to `SockType` on macOS.
- Add `PF_ROUTE` to `SockType` on macOS, all of the BSDs and Linux.
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 incorrect, because you didn't add it to Linux.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, you are right. I've updated it now and I do apologize for the sloppy push.

CHANGELOG.md Outdated
@@ -22,6 +22,8 @@ This project adheres to [Semantic Versioning](https://semver.org/).
- Added `MntFlags` and `unmount` on all of the BSDs.
- Added `any()` and `all()` to `poll::PollFd`.
([#1877](https://github.com/nix-rust/nix/pull/1877))
- Add `PF_ROUTE` to `SockType` on macOS, all of the BSDs.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
- Add `PF_ROUTE` to `SockType` on macOS, all of the BSDs.
- Add `PF_ROUTE` to `SockType` on macOS and the BSDs.

Copy link
Member

Choose a reason for hiding this comment

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

But isn't this still inaccurate? Because you also added it for iOS, illumos, Haiku, and Fuchsia, I believe.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You are correct, I also moved the entry up the changelog, since these changes have not been released anywhere yet.

Copy link
Member

Choose a reason for hiding this comment

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

I think you forgot to push?

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 bors bot merged commit ed8319c into nix-rust:master Nov 29, 2022
@dlon
Copy link

dlon commented May 17, 2023

@asomers Would you be able to cut a release that includes this patch? We're relying on it now but would like to get rid of the git dependency.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants