posix: multiple resolv.conf entries, avoid ipv6 failure on dns functions#405
posix: multiple resolv.conf entries, avoid ipv6 failure on dns functions#405AtieP wants to merge 2 commits intomanagarm:masterfrom
Conversation
AtieP
commented
Apr 9, 2022
AtieP
commented
- Allow any resolv.conf entry to be retrieved.
- Allow only IPV4 resolv.conf entries on lookup_name_dns() and lookup_addr_dns().
- If one resolv.conf entry doesn't work on lookup_nane_dns() and lookup_addr_dns(), use the next one.
* Allow any resolv.conf entry to be retrieved. * Allow only IPV4 resolv.conf entries on lookup_name_dns() and lookup_addr_dns().
Why is this needed? |
Because having an IPV6 address breaks the code, since there isn't support for them yet in inet_pton(), and the DNS code at all. |
|
What special work is even needed for them to work in the DNS code? This is not AAAA records, this is socket(AF_INET6) instead of AF_INET. |
|
I meant that there isn't IPV6 inet_pton() support yet, so passing a IPV6 address to it just breaks, and what lookup_name_dns()/lookup_addr_dns() do is use that exact function. |
Geertiebear
left a comment
Geertiebear
left a comment
There was a problem hiding this comment.
Some comments, nothing major. Another improvement that could be made is caching resolv.conf entries so we don't have to parse the file on every request.
| struct nameserver_data { | ||
| nameserver_data() | ||
| : name(getAllocator()) {} | ||
| nameserver_data(const char *name_, int af_) |
There was a problem hiding this comment.
This is a really minor nitpick, but we don't do this in the managarm code style, nor is it necessary.
| while (true) { | ||
| auto nameserver = get_nameserver(nameserver_idx++); | ||
| if (!nameserver) { | ||
| // no more nameservers in resolf.conf, |
There was a problem hiding this comment.
Another style note: comments should be formally written. So with proper punctuation and capitalization.
| if (!inet_aton(nameserver ? nameserver->name.data() : "127.0.0.1", &sin.sin_addr)) { | ||
| mlibc::infoLogger() << "lookup_name_dns(): inet_aton() failed!" << frg::endlog; | ||
| return -EAI_SYSTEM; | ||
| int nameserver_idx = 0; |
There was a problem hiding this comment.
I feel like this routine can be split up into a function.
| mlibc::infoLogger() << "lookup_name_dns(): inet_aton() failed!" << frg::endlog; | ||
| return -EAI_SYSTEM; | ||
| int nameserver_idx = 0; | ||
| while (true) { |
There was a problem hiding this comment.
Also this while loop doesn't need to be a while(true) loop, you can also write while(nameserver = get_nameserver()).
| for (end = pos; *end && !isspace(*end); end++); | ||
| *end = '\0'; | ||
| ret.name = frg::string<MemoryAllocator>( | ||
| auto str = frg::string<MemoryAllocator>( |
|
Sure @Geertiebear , I will improve everything you mentioned. The rationale behind the last snippet is to make error handling easier, since outside of the while loop the code checks if an error happened by seeing if |
|
|
Can we approve this PR for ci runs? |
4 participants
