Refactor: Modify udp_recvfrom. Raplace recvmsg with WSARecvMsg in windows #1324
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
KangLin
changed the title
KangLin
force-pushed
the
noblock
branch
4 times, most recently
from
da57486
to
525eb41
Compare
…dows - Because of getsockopt is not support IPV6_TCLASS in windows. it need WSARecvMsg get IPV6_TCLASS. and it must called after bind(). - Because of no recvmsg in windows. Raplace recvmsg with WSARecvMsg see: - https://learn.microsoft.com/windows/win32/winsock/ipproto-ipv6-socket-options - https://learn.microsoft.com/windows/win32/api/mswsock/nc-mswsock-lpfn_wsarecvmsg
Test environment:
Test resultsConsistent with the projected results Logturnutils_peer #1314turnutils_uclientturnserverMSVCmingw: |
Test environment:
Test resultsConsistent with the projected results Logturnutils_peer #1314turnutils_uclientturnserver |
jonesmz
reviewed
Jan 19, 2024
| @@ -387,8 +374,23 @@ int get_raw_socket_tos(evutil_socket_t fd, int family) { | |||
| #else | |||
| socklen_t slen = (socklen_t)sizeof(tos); | |||
| if (getsockopt(fd, IPPROTO_IPV6, IPV6_TCLASS, &tos, &slen) < 0) { | |||
| perror("get TCLASS on socket"); | |||
| return -1; | |||
| #if defined(_MSC_VER) | |||
There was a problem hiding this comment.
shouldn't this be checking defined(WINDOWS) like is done in other places?
jonesmz
reviewed
Jan 19, 2024
| @@ -93,7 +93,8 @@ static int udp_create_server_socket(server_type *server, const char *ifname, con | |||
| if (addr_bind(udp_fd, server_addr, 1, 1, UDP_SOCKET) < 0) | |||
| return -1; | |||
|
|
|||
| socket_set_nonblocking(udp_fd); | |||
| if (evutil_make_socket_nonblocking(udp_fd)) | |||
jonesmz
reviewed
Jan 19, 2024
| @@ -467,28 +467,22 @@ static int create_new_connected_udp_socket(dtls_listener_relay_server_type *serv | |||
|
|
|||
| evutil_socket_t udp_fd = socket(s->local_addr.ss.sa_family, CLIENT_DGRAM_SOCKET_TYPE, CLIENT_DGRAM_SOCKET_PROTOCOL); | |||
There was a problem hiding this comment.
Check that s is valid to dereference before using operator->
jonesmz
reviewed
Jan 19, 2024
| if (!ret) { | ||
| TURN_LOG_FUNC(TURN_LOG_LEVEL_ERROR, "%s: Cannot allocate new socket structure\n", __FUNCTION__); | ||
| TURN_LOG_FUNC(TURN_LOG_LEVEL_ERROR, "Cannot allocate new ioa_socket\n"); |
There was a problem hiding this comment.
it would be nice to continue log the function name, as well as the file and line number.
Additionally, can the error code explaining the allocation failure be logged?
jonesmz
reviewed
Jan 19, 2024
| @@ -789,6 +783,9 @@ static int create_server_socket(dtls_listener_relay_server_type *server, int rep | |||
| } | |||
| } | |||
|
|
|||
| if (evutil_make_socket_nonblocking(udp_listen_fd)) | |||
jonesmz
reviewed
Jan 19, 2024
| ret = (ioa_socket *)calloc(sizeof(ioa_socket), 1); | ||
|
|
||
| ret->magic = SOCKET_MAGIC; | ||
| ret = create_ioa_socket_from_fd(e, fd, NULL, st, sat, NULL, NULL); |
There was a problem hiding this comment.
declare and define on same line
jonesmz
reviewed
Jan 19, 2024
| @@ -1294,10 +1296,28 @@ ioa_socket_handle create_ioa_socket_from_fd(ioa_engine_handle e, ioa_socket_raw | |||
| } | |||
|
|
|||
| ret = (ioa_socket *)calloc(sizeof(ioa_socket), 1); | |||
There was a problem hiding this comment.
declare and define on same line
jonesmz
reviewed
Jan 19, 2024
| recv_tos = *((recv_tos_t *)WSA_CMSG_DATA(cmsg)); | ||
| break; | ||
| #endif | ||
| #if defined(IP_RECVERR) |
There was a problem hiding this comment.
what would cause these various defines to be defined or not defined?
Having some documentation in the code explaining that would be helpful.
jonesmz
reviewed
Jan 19, 2024
| TURN_LOG_FUNC(TURN_LOG_LEVEL_DEBUG, "WSARecvMsg len: %d; ttl: %08X; tos: %08X\n", bytes_received, recv_ttl, | ||
| recv_tos); | ||
|
|
||
| #elif !defined(CMSG_SPACE) | ||
| do { | ||
| len = recvfrom(fd, buffer, buf_size, flags, (struct sockaddr *)orig_addr, (socklen_t *)&slen); |
There was a problem hiding this comment.
what prevents this from looping forever?
|
No. MingW is not need.
在 2024-01-20 05:05:02，"Michael Jones" ***@***.***> 写道：
@jonesmz commented on this pull request.
In src/apps/common/apputils.c:
@@ -387,8 +374,23 @@ int get_raw_socket_tos(evutil_socket_t fd, int family) {
#else
socklen_t slen = (socklen_t)sizeof(tos);
if (getsockopt(fd, IPPROTO_IPV6, IPV6_TCLASS, &tos, &slen) < 0) {
- perror("get TCLASS on socket");
- return -1;
+#if defined(_MSC_VER)
shouldn't this be checking defined(WINDOWS) like is done in other places?
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you authored the thread.Message ID: ***@***.***>
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Replace socket_set_nonblocking() with evutil_make_socket_nonblocking()
Windows: Modify udp_recvfrom. Raplace recvmsg with WSARecvMsg
it need WSARecvMsg get IPV6_TCLASS. and it must called after bind().
see: https://learn.microsoft.com/windows/win32/winsock/ipproto-ipv6-socket-options
see: https://learn.microsoft.com/windows/win32/api/mswsock/nc-mswsock-lpfn_wsarecvmsg