Fixes incorrect handling of inet_gethostbyname() return values in Cellular.resolve() and WiFi.resolve()

[avtolstoy]

Problem

See issue #1304

Cellular.resolve() and WiFi.resolve() functions that internally use inet_gethostbyname() incorrectly handle inet_gethostbyname() return values.

Solution

• Treat any inet_gethostbyname() return value other than 0 as an error.
• Fix tests to check all octets of the IP address, instead of just the first one.

Steps to Test

• wiring/no_fixture
• wiring/wifi

References

• Closes issue #1304
• CH7267

---

Completeness

• User is totes amazing for contributing!
• Contributor has signed CLA (Info here)
• Problem and Solution clearly stated
• Run unit/integration/application tests on device
• Added documentation
• Added to CHANGELOG.md after merging (add links to docs and issues)

[m-mcgowan]

can you please explain this? I thought the default comparison is not equal to 0?

[avtolstoy]

Electron's inet_gethostbyname returns a positive value in case of an error, which fails the <0 check. It sounds also reasonable to make sure that the return value is negative in all the HALs, but the check for != 0 sounds good as well, as that mimics the behavior of gethostbyname_r.

[m-mcgowan]

oh, I didn't see the <0 when I originally looked (text wrapping.) If we change it to != 0 then we should check no other HALs are using >0 codes for success. (I have a feeling the CC3000 does this...) so it might be better to make the Electron HAL conform to the established <0 convention.

[avtolstoy]

I don't think that's the case on the Core either: see https://github.com/spark/firmware/blob/develop/hal/src/core/inet_hal.c#L39

[technobly]

• Perhaps we should fix this in inet_gethostbyname directly. If you see the user's logs when resolve returns an obvious failure there is still a non-zero IP address.

• If it fails, perhaps we should also ensure the IP address is 0.0.0.0 as well as returning a proper error code (for users that don't bother to add error checking).

• Unclear why we (and by we I mean me) went with a positive return value, looks like an error. If we made cellular return -1 when an error occurs we'd have to check the places inet_gethostbyname is called in system firmware for cellular and possibly flip the logic.

• We should double check that either way.

cellular

int inet_gethostbyname(const char* hostname, uint16_t hostnameLen, HAL_IPAddress* out_ip_addr,
		network_interface_t nif, void* reserved)
{
	uint32_t result = electronMDM.gethostbyname(hostname);
	if (result > 0) {
		out_ip_addr->ipv4 = result;
		return 0;
	}
    return 1;
}

wifi

int inet_gethostbyname(const char* hostname, uint16_t hostnameLen, HAL_IPAddress* out_ip_addr, network_interface_t nif, void* reserved)
{
    wiced_ip_address_t address;
    wiced_result_t result = wiced_hostname_lookup (hostname, &address, 5000);
    if (result == WICED_SUCCESS) {
    		HAL_IPV4_SET(out_ip_addr, GET_IPV4_ADDRESS(address));
    }
    return -result;
}

[avtolstoy]

How about we do both then? :) Core also returns positive error:

int inet_gethostbyname(const char* hostname, uint16_t hostnameLen, HAL_IPAddress* out_ip_addr,
    network_interface_t nif, void* reserved)
{
    int attempts = 5;
    out_ip_addr->ipv4 = 0;
    int result = 0;
    while (!out_ip_addr->ipv4 && attempts --> 0) {
        HAL_Delay_Milliseconds(1);
        result = gethostbyname(hostname, hostnameLen, &out_ip_addr->ipv4)<0;
    }
    return result;
}

!= 0 check will also ensure that we return 0.0.0.0 IP address in case of any non-zero return value from inet_gethostbyname

[technobly]

where are you referring to for the !=0 check? I'm not seeing how that would force the ip to be 0.0.0.0.

electronMDM.gethostbyname() will return 0 for an error, so in that case we can set the ip object to 0.0.0.0 (or 0), but perhaps it's better to initialize the HAL_IPAddress ip; object to zero in Cellular/Wifi.resolve().

For cellular, we should be setting the ip with HAL_IPV4_SET(out_ip_addr, result); as well.

[m-mcgowan]

Cherry-picked to 0.7.0-rc.7 branch, and that merged to develop for 0.8.0-rc.2 release. (I probably should have rebased instead so Github maintained the tracking, but that's hindsight.)