Skip to content

Reimplement ares_gethostbyname() by wrapping ares_getaddrinfo()#428

Merged
bradh352 merged 15 commits intoc-ares:mainfrom
bradh352:gethostbyname_wrap_getaddrinfo
Oct 16, 2021
Merged

Reimplement ares_gethostbyname() by wrapping ares_getaddrinfo()#428
bradh352 merged 15 commits intoc-ares:mainfrom
bradh352:gethostbyname_wrap_getaddrinfo

Conversation

@bradh352
Copy link
Member

ares_gethostbyname() and ares_getaddrinfo() do a lot of similar things, however ares_getaddrinfo() has some desirable behaviors that should be imported into ares_gethostbyname(). For one, it sorts the address lists for the most likely to succeed based on the current system routes. Next, when AF_UNSPEC is specified, it properly handles search lists instead of first searching all of AF_INET6 then AF_INET, it searches in parallel. Therefore, this PR should also resolve the issues attempted in #94.

A few things this PR does:

  1. ares_parse_a_reply() and ares_parse_aaaa_reply() had very similar code to translate struct ares_addrinfo into a struct hostent as well as into struct ares_addrttl/ares_addr6ttl this has been split out into helper functions of ares__addrinfo2hostent() and ares__addrinfo2addrttl() to prevent this duplicative code.
  2. ares_getaddrinfo() was apparently never honoring HOSTALIASES, and this was discovered once ares_gethostbyname() was turned into a wrapper, the affected test cases started failing.
  3. A slight API modification to save the query hostname into struct ares_addrinfo as the last element of name. Since this is the last element, and all user-level instances of struct ares_addrinfo are allocated internally by c-ares, this is not an ABI-breaking change nor would it impact any API compatibility. This was needed since struct hostent has an h_name element.
  4. Test Framework: MockServer tests via TCP would fail if more than 1 request was received at a time which is common when ares_getaddrinfo() queries for both A and AAAA records simultaneously. Infact, this was a long standing issue in which the ares_getaddrinfo() test were bypassing TCP alltogether. This has been corrected, the message is now processed in a loop.
  5. Some tests had to be updated for overall correctness as they were invalid but somehow passing prior to this change.

@bradh352
Copy link
Member Author

@ChristianAmmer, please review and see if this accomplishes your goal from PR #94

@bradh352
Copy link
Member Author

If there's no comments, I'll probably just merge it and deal with the fallout. This blocks me from working on #429 & #399

@bagder
Copy link
Member

bagder commented Oct 15, 2021

I'll probably just merge it and deal with the fallout.

That's fine with me. I've started a review and it looks fine but I haven't managed to get through it all...

@ChristianAmmer-zz
Copy link

@bradh352 sorry for the late reply. Your PR accomplishes the goal from PR #94, thanks for taking care. I'm going to do a review, but I'm not that familiar with the code anymore.

@bradh352 bradh352 merged commit c642b9f into c-ares:main Oct 16, 2021
@bradh352 bradh352 mentioned this pull request Oct 16, 2021
sergepetrenko pushed a commit to tarantool/c-ares that referenced this pull request Jul 29, 2022
…es#428)

ares_gethostbyname() and ares_getaddrinfo() do a lot of similar things, however ares_getaddrinfo() has some desirable behaviors that should be imported into ares_gethostbyname(). For one, it sorts the address lists for the most likely to succeed based on the current system routes. Next, when AF_UNSPEC is specified, it properly handles search lists instead of first searching all of AF_INET6 then AF_INET, since ares_gethostbyname() searches in parallel. Therefore, this PR should also resolve the issues attempted in c-ares#94.

A few things this PR does:

1. ares_parse_a_reply() and ares_parse_aaaa_reply() had very similar code to translate struct ares_addrinfo into a struct hostent as well as into struct ares_addrttl/ares_addr6ttl this has been split out into helper functions of ares__addrinfo2hostent() and ares__addrinfo2addrttl() to prevent this duplicative code.

2. ares_getaddrinfo() was apparently never honoring HOSTALIASES, and this was discovered once ares_gethostbyname() was turned into a wrapper, the affected test cases started failing.

3. A slight API modification to save the query hostname into struct ares_addrinfo as the last element of name. Since this is the last element, and all user-level instances of struct ares_addrinfo are allocated internally by c-ares, this is not an ABI-breaking change nor would it impact any API compatibility. This was needed since struct hostent has an h_name element.

4. Test Framework: MockServer tests via TCP would fail if more than 1 request was received at a time which is common when ares_getaddrinfo() queries for both A and AAAA records simultaneously. Infact, this was a long standing issue in which the ares_getaddrinfo() test were bypassing TCP alltogether. This has been corrected, the message is now processed in a loop.

5. Some tests had to be updated for overall correctness as they were invalid but somehow passing prior to this change.

Change By: Brad House (@bradh352)
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