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

feat(client): http_connector supports set_interface #2484

Closed
wants to merge 2 commits into from
Closed

feat(client): http_connector supports set_interface #2484

wants to merge 2 commits into from

Conversation

kolapapa
Copy link

@kolapapa kolapapa commented Mar 30, 2021

In my scenario, I need to detect the network stability of multiple interfaces at the same time, including ICMP, HTTP, etc.

The bind_device() of socket2's crate meets the requirement of specific interface in ICMP(ping). But HTTP related, I use the reqwest crate, but I can't use reqwest to bind a specific interface to achieve the flow in and out through a specific exit.

And now, I need to do upload and download, HTTP quality detection and other functions at present. If I want to achieve my goal by supporting bind_device(), please help review it. I can't help you.

@kolapapa kolapapa changed the title feat(client): http_connector supports SO_BINDTODEVICE feat(client): http_connector supports bind_device() Mar 31, 2021
Copy link
Member

@seanmonstar seanmonstar left a comment

Choose a reason for hiding this comment

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

Thanks for the PR! Some suggestions inline.

/// [VRF]: https://www.kernel.org/doc/Documentation/networking/vrf.txt
#[cfg(any(target_os = "android", target_os = "fuchsia", target_os = "linux"))]
#[inline]
pub fn bind_device<S: ToString>(&mut self, interface: S) -> &mut Self {
Copy link
Member

Choose a reason for hiding this comment

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

How about the name set_interface?

Copy link
Member

Choose a reason for hiding this comment

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

Also, I'd probably say the generic should be S: Into<String>, since otherwise set_interface(format("eth{}", num)) will make a string with format!, and then copy thanks to ToString.

Copy link
Author

Choose a reason for hiding this comment

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

I agree with you

@@ -612,6 +633,14 @@ fn connect(
)
.map_err(ConnectError::m("tcp bind local error"))?;

#[cfg(any(target_os = "android", target_os = "fuchsia", target_os = "linux"))]
// That this only works for some socket types, particularly AF_INET sockets.
if config.local_address_ipv6.is_none() && config.interface.is_some() {
Copy link
Member

Choose a reason for hiding this comment

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

Why only if the ipv6 address is none? What if the ipv4 address is some? What if ipv6 is some?

Copy link
Author

Choose a reason for hiding this comment

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

Sorry, I mistakenly thought that IPv6 does not support SO_BINDTODEVICE operation. Later, I tried, is feasible.

@kolapapa kolapapa changed the title feat(client): http_connector supports bind_device() feat(client): http_connector supports set_interface Apr 8, 2021
@kolapapa kolapapa closed this Nov 11, 2021
@hottea773
Copy link

@kolapapa is there a reason this was closed?

I'd like to make use of this change, so would be keen to get this merged.

@hottea773
Copy link

(I've opened #3076 which duplicates this, but would be happy for either to be merged.)

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.

4 participants