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

Fix Error in hostByName with low timeout #7585

Merged
merged 1 commit into from
Sep 9, 2020
Merged

Fix Error in hostByName with low timeout #7585

merged 1 commit into from
Sep 9, 2020

Conversation

ruggi99
Copy link
Contributor

@ruggi99 ruggi99 commented Sep 8, 2020

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.

Copy link
Collaborator

@d-a-v d-a-v left a comment

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.

@ruggi99
Copy link
Contributor Author

ruggi99 commented Sep 8, 2020

That would be too much a breaking change.

Don't understand why it would be a breaking change

@d-a-v
Copy link
Collaborator

d-a-v commented Sep 8, 2020

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.

@ruggi99
Copy link
Contributor Author

ruggi99 commented Sep 8, 2020

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?

@d-a-v
Copy link
Collaborator

d-a-v commented Sep 8, 2020

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.

@ruggi99
Copy link
Contributor Author

ruggi99 commented Sep 8, 2020

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.

@d-a-v
Copy link
Collaborator

d-a-v commented Sep 8, 2020

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);

@d-a-v d-a-v merged commit 08f1705 into esp8266:master Sep 9, 2020
@ruggi99 ruggi99 deleted the fix-gethostbyname branch September 9, 2020 10:05
Jason2866 added a commit to Jason2866/Arduino that referenced this pull request Sep 17, 2020
Jason2866 added a commit to Jason2866/Tasmota that referenced this pull request Sep 17, 2020
Jason2866 added a commit to tasmota/Arduino that referenced this pull request Mar 22, 2021
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.

None yet

2 participants