-
Notifications
You must be signed in to change notification settings - Fork 89
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
improvements for ipv6 #703
Conversation
Have confirmation from ZD Ticket that this change is working as expected. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Over to @ejohnstown
apps/wolfsshd/wolfsshd.c
Outdated
#ifdef TEST_IPV6 | ||
&clientAddr.sin6_addr, | ||
inet_ntop(AF_INET, &clientAddr.sin6_addr, conn->ip, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This doesn't actually work. inet_ntop() is going to take the first 32-bits of sin6_addr and turn it into a dotted-quad string.
wolfssh/test.h
Outdated
@@ -546,7 +546,7 @@ static INLINE void build_addr(SOCKADDR_IN_T* addr, const char* peer, | |||
#endif | |||
|
|||
|
|||
static INLINE void tcp_socket(WS_SOCKET_T* sockFd) | |||
static INLINE void tcp_socket(WS_SOCKET_T* sockFd, int specifyProtocol, int targetProtocol) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we make this a little smarter? Maybe have tcp_socket() and tcp6_socket() that call a utility function?
apps/wolfssh/wolfssh.c
Outdated
@@ -991,7 +991,13 @@ static THREAD_RETURN WOLFSSH_THREAD wolfSSH_Client(void* args) | |||
err_sys("Couldn't set the username."); | |||
|
|||
build_addr(&clientAddr, config.hostname, config.port); | |||
tcp_socket(&sockFd); | |||
tcp_socket(&sockFd, 1, | |||
#ifdef TEST_IPV6 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These checks are really awkward. TEST_IPV6 is meant to force the example tools to use IPV6 for testing. It was a quick-and-dirty way to support IPv6. In general, we shouldn't need checks like this. Information about the IP address type are kept with the IP address. (clientAddr.sin_family)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure how to make it less awkward. Since we have the typedef for SOCKADDR_IN_T
, the check of the member will need this macro wrap. (clientAddr.sin_family vs. clientAddr.sin6_family).
@@ -2374,8 +2374,8 @@ static int StartSSHD(int argc, char** argv) | |||
#ifdef WOLFSSL_NUCLEUS | |||
struct addr_struct clientAddr; | |||
#else | |||
SOCKADDR_IN_T clientAddr; | |||
socklen_t clientAddrSz = sizeof(clientAddr); | |||
struct sockaddr_in6 clientAddr; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a reason we don't have all this wrapped with TEST_IPV6
and use SOCKADDR_IN_T
in the non IPv6 case? I noticed a bit of a mess between the examples for the IPv4 and IPv6 support.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Already discussed internally. No need for changes. Back to @dgarske.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The sshd will always support IPv6
improvements for ipv6
improvements for ipv6
improvements for ipv6
improvements for ipv6
Fixes ZD18053