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

Proper error reporting for connect failures #578

Merged
merged 4 commits into from
Oct 3, 2018
Merged

Conversation

mnunberg
Copy link
Contributor

@mnunberg mnunberg commented Mar 4, 2018

No description provided.

@mnunberg mnunberg changed the title Connfix Proper error reporting for connect failures Mar 4, 2018
@mnunberg
Copy link
Contributor Author

mnunberg commented Mar 4, 2018

Note that another PR (#461) also exists. Unlike the linked PR, this PR doesn't add a new API, and it doesn't break the abstraction layer between socket and context.

@mnunberg
Copy link
Contributor Author

mnunberg commented Mar 4, 2018

I'd also like to abstract getaddrinfo to be non-blocking; right now it's blocking and in async mode we never really rotate. I'd also like to abstract the sockaddr member as well.

@mnunberg
Copy link
Contributor Author

mnunberg commented Mar 4, 2018

@dvirsky

@dvirsky
Copy link

dvirsky commented Mar 4, 2018

Cc: @yossigo can you review this?

net.c Outdated
if (c->saddr) {
free(c->saddr);
}
c->saddr = malloc(sizeof(*p->ai_addr));
Copy link
Member

Choose a reason for hiding this comment

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

The allocation should be done based on ai_addrlen, as ai_addr is a vanilla sockaddr which may be smaller than what you really need (e.g. for AF_UNIX or AF_INET6).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oops - done. There's also struct sockaddr_storage, but ideally I'd like to preserve the original addrinfo structure anyway.

@mnunberg
Copy link
Contributor Author

mnunberg commented Mar 5, 2018

Still working on getting the tests to pass..

This retrieves the actual error which occurred, as getsockopt is not
always reliable in this regard.
Some ISPs like to inject their own "Suggestions" page whenever you hit
NXDOMAIN. This confuses Redis as well as addrinfo (black-holing the
route).
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.

3 participants