-
Couldn't load subscription status.
- Fork 2.3k
Fix Can not resolve "localhost" if not Network adapter exists #4988 #5019
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
base: main
Are you sure you want to change the base?
Conversation
| return result; | ||
| } | ||
| #if defined(_WIN32) | ||
| else if(rc == WSANO_DATA && address.isLoopback()) // no data record found and is local host |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does it make sense putting this condition first, before checking the "localhost" hostname?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i would say checking the rc value is much cheaper than checking for localhost or loopback and is less common.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I apologise for not being clear enough.
In line 86 (above) it is first checked if the hostname is localhost and if that is not true then in line 145 is is checked if the device is a loopback.
From the logic point of view, what shall be the order? localhost first of loopback first? Or it does not matter?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@matejk Those are different methods:
First is 'DNS::hostByName()' line 63-114, second is 'DNS::hostByAddress' from line 117-176
| #if defined(POCO_OS_FAMILY_WINDOWS) | ||
| else if(rc == WSANO_DATA && hostname == "localhost") // no data record found and is local host | ||
| { | ||
| #if defined(POCO_HAVE_IPv6) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think it's a good idea to default to IPv6 solely based on it being present on the system where poco was compiled. Not even the presence of IPv6 on the runtime system would warrant it being default, given the prevalence of IPv4.
Bottom line, I would prefer this to be decided at runtime, based on an additional function argument, defaulting to IPv4.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@aleks-f Since this is an edge case i am not sure if a addition argument wouldn't confuse users.
Does an function exists, that determines if ipv6 is available?
Otherwise i would use 127.0.0.1
|
I don't think Poco should do any name resolution at all outside of that provided by the operating system's network stack. |
|
@obiltschnig i can not respond to you comment. |
Fill HostEntry with localhost and a default loopback address if 'getaddrinfo' returns 'WSANO_DATA'