Fix Error in hostByName with low timeout by ruggi99 · Pull Request #7585 · esp8266/Arduino (original) (raw)
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 andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Conversation7 Commits1 Checks0 Files changed
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters
[ Show hidden characters]({{ revealButtonHref }})
Before, aResult was set to INADDR_NONE, so 255.255.255.255 (broadcast). When timeout was too low and callback didn't fire, the aResult.isSet() line returned always true and that's not the correct behaviour.
Even thought I'm not a native speaker, INADDR_ANY and INADDR_NONE are confusing me.
Whenever I think about it, I always associate INADDR_NONE with 0.0.0.0 and INADDR_ANY with 255.255.255.255, but it's the other way.
Also lwIP has the same scheme.
I would like to ask if we can add a clear method to IPAddress like did to String class (maybe in another PR) to resolve any doubts.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That would be too much a breaking change.
What would be acceptable is to extend IPAddress::isSet() to return false for both INADDR_ANY and INADDR_NONE.
Also, nothing against a new IPAddress::clear() method that would set to INADDR_ANY.
Note that INADDR_NONE is considered to be an obsolete use of this 255.255.255.255 address that is however a valid address.
That would be too much a breaking change.
Don't understand why it would be a breaking change
Don't understand why it would be a breaking change
Consider this: if (result == INADDR_NONE) { Serial.printf("bad address"); } else { /* do something */ }
This is not the right way to do it, but if that code is used somewhere, it would still compile but wouldn't be valid anymore, silently.
The right way is to check the return value of the function.
Yes understood.
So in case the clear method is implemented in IPAddress, it could not be used here in hostByName for the same reason.
So the only solution for now is to modify the isSet function?
The best solution is to read hostByBame()'s result.
if you can only check on the resulted IP address, then using the proposed modification to isSet() could do it.
Agree.
In any case, if you rely on the result of the function, it could return success, when the IP was not really found. In the latter case, one might use isSet to check the IP (also parameter for the function) and still have an incorrect result.
Now we have to found the best method to implement that correction.
Searching quickly in Lwip definitions there's not a macro that checks if the IP is IPADDR_NONE, but only for IPADDR_ANY (already used).
So we might use the equal operator defined as bool operator==(const IPAddress& addr) const, but I don't know if that's the best solution in terms of performance and memory.
So we might use the equal operator defined as bool operator==(const IPAddress& addr) const, but I don't know if that's the best solution in terms of performance and memory.
I think it is safe to
return !ip_addr_isany(&_ip) && ((*this) != IPADDR_NONE);
Jason2866 added a commit to Jason2866/Arduino that referenced this pull request
Jason2866 added a commit to Jason2866/Tasmota that referenced this pull request
Jason2866 added a commit to tasmota/Arduino that referenced this pull request
2 participants
