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

Wrap getaddrinfo to try more than once on Windows. #1346

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Wrap getaddrinfo to try more than once on Windows. #1346

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
a262ff1
Wrap getaddrinfo to try more than once on Windows.
madbrain76 May 30, 2022
94274ee
Merge branch 'esnet:master' into patch_branch
madbrain76 Jun 4, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
28 changes: 25 additions & 3 deletions src/net.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -115,6 +115,28 @@ timeout_connect(int s, const struct sockaddr *name, socklen_t namelen,
return (ret);
}

#ifdef __CYGWIN__

const unsigned int MAX_GETADDRINFO_RETRIES = 10;
/* getaddrinfo may not be completely reliable on Windows hosts. Try more than once. */
int iperf_getaddrinfo(const char *restrict node,
const char *restrict service,
const struct addrinfo *restrict hints,
struct addrinfo **restrict res)
{
int retvalue = -1;
int i = 0;
while (i++ < MAX_GETADDRINFO_RETRIES) {
retvalue = getaddrinfo(node, service, hints, res);
if (0 == retvalue) break;
sleep(1); /* yield thread */
fprintf(stderr, "getadddrinfo failed. If iperf still ran successfully, this may be a Windows bug. Please see https://github.com/esnet/iperf/issues/1314 . \n");
Copy link
Contributor

Choose a reason for hiding this comment

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

Error should be either printer only if debug option was set or using iperf_err() (to properly handle the JSON output case). Suggest to leave the decision to the iperf3 maintainers (before merging the change).

In both cases, test should be passed as a parameter to the function and iperf.h should be included. In case iperf_err is used, also iperf_api.h should be included

If printed as debug message, suggest to use the newly added debug level option:

if (test->debug_level >= DEBUG_LEVEL_ERROR))
    fprintf(...);

Copy link
Author

Choose a reason for hiding this comment

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

David, thanks for the feedback. Obviously, I wrote my PR before the option was added. I would prefer for this message to be printed unconditionally if getaddrinfo fails intermittently, regardless of the new debug option. The rationale is that with the loop in place, a fatal error would not be seen anymore. And without being aware of the intermittent failure behind the scene, nobody would think to add -d. The intermittent failure seems rare enough that I wanted anyone with a affected system to see the message. Requiring -d would significantly decrease the odds anyone would ever know about the problem. The wording of the message could be changed, though.

I checked the source to see where debug_level was being set. The iperf_test structure that contains debug_level isn't available in the scope of most of the calls to getaddrinfo . A pointer to it would need to be propagated across the stack. getaddrinfo itself of course, being a low-level call, doesn't take this object as an argument. I am reluctant to change the prototype for getaddrinfo . I could make it a wrapper on all platforms, instead of having the platform #ifdef, though.

iperf_test appears to contain a global configuration state. And there is only ever a single instance, created in main() with a call to iperf_new_test. As long as that remains the case, it strikes me as a bit of busy work to pass this object around the stack down everywhere, not just to iperf_getaddrinfo. I don't want to address that in this PR.

Copy link
Contributor

Choose a reason for hiding this comment

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

I would prefer for this message to be printed unconditionally if getaddrinfo fails intermittently, regardless of the new debug option.

The new debug option is just the ability to set a debug level. Using if (test->debug) always existed, but it still needs the use of iperf_test.

The rationale is that with the loop in place, a fatal error would not be seen anymore. And without being aware of the intermittent failure behind the scene, nobody would think to add -d.

I agree. In this case still iperf_err() should be used to log the error, to properly handle JSON output (in case -J was set), which also needs the use of iperf_test.

A pointer to it would need to be propagated across the stack. getaddrinfo itself of course, being a low-level call, doesn't take this object as an argument. I am reluctant to change the prototype for getaddrinfo . .....

I agree that adding iperf_test to getaddrinfo and to net.c in general is a problem. I therefore suggest to leave the decision to the iperf3 maintainers whether to propagate iperf_test down to net.c, not log the error in getaddrinfo, add a wrapper or any of the other approaches you suggested.

}
return retvalue;
}

#endif

/* netdial and netannouce code comes from libtask: http://swtch.com/libtask/
* Copyright: http://swtch.com/libtask/COPYRIGHT
*/
Expand All @@ -131,15 +153,15 @@ create_socket(int domain, int proto, const char *local, const char *bind_dev, in
memset(&hints, 0, sizeof(hints));
hints.ai_family = domain;
hints.ai_socktype = proto;
if ((gerror = getaddrinfo(local, NULL, &hints, &local_res)) != 0)
if ((gerror = iperf_getaddrinfo(local, NULL, &hints, &local_res)) != 0)
return -1;
}

memset(&hints, 0, sizeof(hints));
hints.ai_family = domain;
hints.ai_socktype = proto;
snprintf(portstr, sizeof(portstr), "%d", port);
if ((gerror = getaddrinfo(server, portstr, &hints, &server_res)) != 0) {
if ((gerror = iperf_getaddrinfo(server, portstr, &hints, &server_res)) != 0) {
if (local)
freeaddrinfo(local_res);
return -1;
Expand Down Expand Up @@ -283,7 +305,7 @@ netannounce(int domain, int proto, const char *local, const char *bind_dev, int
}
hints.ai_socktype = proto;
hints.ai_flags = AI_PASSIVE;
if ((gerror = getaddrinfo(local, portstr, &hints, &res)) != 0)
if ((gerror = iperf_getaddrinfo(local, portstr, &hints, &res)) != 0)
return -1;

s = socket(res->ai_family, proto, 0);
Expand Down
8 changes: 8 additions & 0 deletions src/net.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,14 @@
#ifndef __NET_H
#define __NET_H

#ifdef __CYGWIN__
int iperf_getaddrinfo(const char *restrict node,
const char *restrict service,
const struct addrinfo *restrict hints,
struct addrinfo **restrict res);
#else
#define iperf_getaddrinfo getaddrinfo
#endif
int timeout_connect(int s, const struct sockaddr *name, socklen_t namelen, int timeout);
int create_socket(int domain, int proto, const char *local, const char *bind_dev, int local_port, const char *server, int port, struct addrinfo **server_res_out);
int netdial(int domain, int proto, const char *local, const char *bind_dev, int local_port, const char *server, int port, int timeout);
Expand Down