nsapi - Standardize support of NSAPI_UNSPEC #2897

geky · 2016-10-03T22:07:06Z

Standardize support of NSAPI_UNSPEC (renamed to NSAPI_IP_NONE)

Introduced in #2767

Renamed to NSAPI_IP_NONE for consistency with other enums
Reordered nsapi_version_t to make defaule nsapi_addr_t NSAPI_IP_NONE
Added support to NSAPI_IP_NONE in SocketAddress

cc @sg-, @c1728p9, @kjbracey-arm, @mikaleppanen

kjbracey · 2016-10-04T06:48:46Z

Part of the problem is the naming has not existing prefix in the enum, to say what enum it is.

NSAPI_AF_UNSPEC, NSAPI_AF_IPV4, NSAPI_AF_IPV6 would make sense.

"nsapi_version_t" is also horrible, as it's not the version of nsapi_t. It's really nsapi_address_family_t.

"Unspec" came from POSIX, where that is the specific code for an unspecified address family. Asking for "None" from a resolution function doesn't make a huge amount of sense. "IP None" makes even less sense - conceptually it isn't necessarily IP.

Always seems like a good idea to use standard names for standard concepts.

kjbracey

If you're going to attempt to add support for UNSPEC in SocketAddress, you really have to extend that to all the actual socket implementations.

This does really not seem like a one-day-after-freeze change.

kjbracey · 2016-10-04T06:53:22Z

features/netsocket/SocketAddress.cpp

        } else if (_addr.version == NSAPI_IPv6) {
-            ipv6_to_address(ip_address, _addr.bytes);
+            ipv6_to_address(_ip_address, _addr.bytes);
+        } else {


This doesn't make sense. "None/Unspec" in a socket address is not "0.0.0.0" - that could be inferred as a statement that it's IPv4. Suggest "*", which is what you see in inetstat for unbound sockets.

I was wondering what to return here. I don't think "*" is something users would expect.

"0.0.0.0" does have the benifit that it's compatible with current code and also parsable as an address.

An alternative is to simply return a null pointer, since the representation doesn't really exist.

Not sure that compatibility with current code or being parsable as such is actually desirable. If you pass "unspec" to someone, do you really want them to silently treat it as if it was IPv4?

It kind of extends the somewhat undesirable behaviour of treating IPv4 0.0.0.0 as NULL and vice versa. (Nanostack had to have a special check to permit IPv4 iff it was 0.0.0.0...) I'd prefer it if binary->text->binary round-tripping preserved value+type. (The stack doesn't do that as much as it used to, but it still can happen).

An unspec SocketAddress is only appropriate to a few quite limited cases - error returns, and bind are all I can currently think of.

It sounds like a null-pointer from get_ip_address is reasonable if set_ip_address accepts this as NSAPI_UNSPEC.

This strategy would satisfy your concerns with ipv4 as null, round-trip preservation, and forces users to consider NSAPI_UNSPEC as a special case.

geky · 2016-10-04T13:30:46Z

You make a good point. I'll switch back to NSAPI_UNSPEC until we get a chance to update the names to contain the AF prefix.

I agree. This isn't the time socket API changes, but unfortunately NSAPI_UNSPEC was introduced with inconsistent support in the socket API. I understand the motivation for adding the option, but currently most of the socket API has undefined behavior when NSAPI_UNSPEC is passed. This patch attempts to remedy that, and if it doesn't make it in to this next release, at least it is available for the next minor release.

kjbracey · 2016-10-04T14:34:06Z

We did consider working this through - it would be generally useful to have "unspec" in the socket, but we decided to limit the scope to just the gethostbyname for now, and left the comment that it was not applicable to the SocketAddress. We didn't feel we had time to work out the design or consequences for that.

I would suggest just blocking the attempt in the few SocketAddress APIs that accept naspi_version_t, if you want to make sure people don't do it.

The moment you explicitly allow it in SocketAddress, I'm fairly confident quite a few lwip_stack APIs will go a bit weird. There is an assumption throughout that the SocketAddress being received is IPv4 or IPv6.

geky · 2016-10-04T16:10:51Z

Inconsistent support of NSAPI_UNSPEC does not seem like a good user experience. For implementations we can't prevent NSAPI_UNSPEC in the socket_addr_t struct, which becomes problematic when passed to SocketAddress.

The moment you explicitly allow it in SocketAddress, I'm fairly confident quite a few lwip_stack APIs will go a bit weird. There is an assumption throughout that the SocketAddress being received is IPv4 or IPv6.

Do you think we should make the SocketAddress treat NSAPI_UNSPEC identical to ipv4 0.0.0.0 for now?

Alternatively we can update lwip (the convert_lwip_addr_to_mbed function makes this trivial). The nsapi changes from lwip and the feature_wifi_ublox branch will already require updates to all of the other implementations.

EDIT: I've updated lwip with support for NSAPI_UNSPEC. Let me know if I made a mistake.

sg- · 2016-10-04T19:07:54Z

/morph test

bridadan · 2016-10-04T19:13:52Z

/morph test

mbed-bot · 2016-10-04T21:51:18Z

Result: FAILURE

Your command has finished executing! Here's what you wrote!

/morph test

Output

mbed Build Number: 1049

Test failed!

bridadan · 2016-10-04T21:55:09Z

The test failure was caused by an issue with the latest version of mbed-ls. mbed-ls was recently updated, however the latest release has an issue detecting NUCLEO boards. This is a regression and is logged here: ARMmbed/mbed-ls#137. I have since pinned mbed-ls in CI to use version 1.2.4, however past PRs may already be affected.

All other boards passed, and since this wasn't a nightly, networking tests weren't even run. So should be fine

geky · 2016-10-04T22:11:17Z

Tested locally with K64F, NUCLEO_F429ZI, and NUMAKER_PFM_NUC472

kjbracey · 2016-10-05T06:25:13Z

Inconsistent support of NSAPI_UNSPEC does not seem like a good user experience.

Agreed, but this is very much a first pass of non-IPv4 support. I'd argue there's no huge rush to define behaviour here, particularly if it ends up locking in API. If you effectively specify what UNSPEC does in a SocketAddress by implementation now, can we change it later?

Do you think we should make the SocketAddress treat NSAPI_UNSPEC identical to ipv4 0.0.0.0 for now?

My feeling is that lack of unspec was always a mistake, as was treating IPv4 0.0.0.0 as "unset". Ideally unspec takes over from IPv4 0.0.0.0 as the NULL respresentation. So, yes, UNSPEC should be treated as IPv4 0.0.0.0 currently is, but ultimately we should withdraw IPv4 0.0.0.0 from being the default and null value.

kjbracey · 2016-10-05T06:32:14Z

features/FEATURE_LWIP/lwip-interface/lwip_stack.c

        ip_addr_set_zero_ip4(out);
        return true;
    }
 #endif
 #endif

+#if LWIP_IPV4 && LWIP_IPV6


Not sure about this flopping towards IPV4. Could it at least follow the IPv4/IPv6 preference? Ideally we'd convert to lwip's own IPADDR_TYPE_ANY, but that isn't fully supported in their ip_addr_t either - it's used internally, but I don't believe it's valid for external users.

Jesus, this ifdeffing is confusing. Not sure we can do better though.

kjbracey · 2016-10-05T06:35:31Z

features/netsocket/NetworkStack.cpp

@@ -100,25 +90,13 @@ class NetworkStackWrapper : public NetworkStack
        return address->get_ip_address();
    }

-    virtual int gethostbyname(const char *name, SocketAddress *address)


This removal breaks gethostbyname(literal, UNSPEC) - gethostbyname(version = AF_UNSPEC) doesn't actually work. Need to extend that too.

Actually, it's worse than that. The original code called the dns library with version omitted, which meant it defaulted to IPv4. (The DNS library does not accept UNSPEC). This now means that you call the DNS library with UNSPEC, which it interprets as "not IPv4, so IPv6". So gethostbyname is now doing AAAA lookups by default.

Does work with LWIP though, because LWIP is providing its own gethostbyname, which does work with AF_UNSPEC.

Whoops good catch, should be updated

kjbracey · 2016-10-05T06:41:16Z

features/netsocket/SocketAddress.cpp

@@ -215,17 +215,19 @@ void SocketAddress::set_port(uint16_t port)

 const char *SocketAddress::get_ip_address() const
 {
-    char *ip_address = (char *)_ip_address;
+    if (_addr.version == NSAPI_UNSPEC) {


So UNSPEC SocketAddress round-trips via NULL string pointer, if I read correctly. Seems fine. But would still vaguely prefer a real string, so users don't have to take care when doing printf("%s", foo->get_ip_address()).

This does have the benefit of matching the behaviour of iface->get_ip_address() when not connected.

kjbracey · 2016-10-05T06:56:06Z

features/netsocket/SocketAddress.cpp

-    int count = 0;
-    if (a._addr.version == NSAPI_IPv4 && b._addr.version == NSAPI_IPv4) {
-        count = NSAPI_IPv4_BYTES;
+    if (a._addr.version == NSAPI_UNSPEC && b._addr.version == NSAPI_UNSPEC) {


Could check different versions first and return false.

But should UNSPEC be equal to IPv4 0.0.0.0?

Good point, should be updated to compare equal for "null" ip addresses

- Reordered nsapi_version_t to make defaule nsapi_addr_t NSAPI_UNSPEC - Added support to NSAPI_UNSPEC in SocketAddress

0xc0170 · 2016-10-25T09:56:21Z

@kjbracey-arm Happy with this patch?

kjbracey · 2016-10-25T10:17:41Z

I think it looks okay, but I'd want @mikaleppanen to cast an eye over it.

Does it achieve the wanted results when, say, IP preference is IPv4, but we only have an IPv6 address? Or at least, does it work as well as it currently does?

mikaleppanen · 2016-10-25T12:54:16Z

Tested this with dual stack configuration and works ok. Looks good to me.

geky added needs: review needs: CI labels Oct 3, 2016

kjbracey reviewed Oct 4, 2016

View reviewed changes

geky force-pushed the nsapi-consistent-unspec branch from ac36124 to d7f44f4 Compare October 4, 2016 15:37

geky force-pushed the nsapi-consistent-unspec branch from 0314f2f to 9bbb793 Compare October 4, 2016 16:16

sg- removed the needs: review label Oct 4, 2016

geky force-pushed the nsapi-consistent-unspec branch 2 times, most recently from 1d6174a to 32e0e81 Compare October 4, 2016 21:49

kjbracey suggested changes Oct 5, 2016

View reviewed changes

geky force-pushed the nsapi-consistent-unspec branch from 32e0e81 to d6b3cb5 Compare October 5, 2016 16:55

geky added 2 commits October 5, 2016 12:48

nsapi - Standardized support of NSAPI_UNSPEC

1f4eb0a

- Reordered nsapi_version_t to make defaule nsapi_addr_t NSAPI_UNSPEC - Added support to NSAPI_UNSPEC in SocketAddress

lwip - Added support for NSAPI_UNSPEC to lwip

6d9ac9f

geky force-pushed the nsapi-consistent-unspec branch from d6b3cb5 to 6d9ac9f Compare October 5, 2016 18:40

geky added do not merge release-version: 5.2.1 and removed needs: CI labels Oct 6, 2016

geky added ready for merge and removed do not merge labels Oct 19, 2016

sg- added release-version: 5.2.2 and removed release-version: 5.2.1 labels Oct 19, 2016

0xc0170 added release-version: 5.2.1 and removed release-version: 5.2.2 labels Oct 25, 2016

0xc0170 added needs: review and removed ready for merge labels Oct 25, 2016

0xc0170 removed release-version: 5.2.1 needs: review labels Oct 27, 2016

0xc0170 merged commit ba47aa5 into ARMmbed:master Oct 27, 2016

0xc0170 added the release-version: 5.2.1 label Oct 27, 2016

adbridge mentioned this pull request Nov 23, 2016

TLS handshake fails using WiFi in mbed OS 5.2.3 #3314

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

nsapi - Standardize support of NSAPI_UNSPEC #2897

nsapi - Standardize support of NSAPI_UNSPEC #2897

geky commented Oct 3, 2016 •

edited

Loading

kjbracey commented Oct 4, 2016

kjbracey left a comment

kjbracey Oct 4, 2016

geky Oct 4, 2016

kjbracey Oct 4, 2016

geky Oct 4, 2016

geky commented Oct 4, 2016

kjbracey commented Oct 4, 2016

geky commented Oct 4, 2016 •

edited

Loading

sg- commented Oct 4, 2016

bridadan commented Oct 4, 2016

mbed-bot commented Oct 4, 2016

bridadan commented Oct 4, 2016 •

edited

Loading

geky commented Oct 4, 2016

kjbracey commented Oct 5, 2016

kjbracey Oct 5, 2016 •

edited

Loading

kjbracey Oct 5, 2016 •

edited

Loading

kjbracey Oct 5, 2016

kjbracey Oct 5, 2016

geky Oct 5, 2016

kjbracey Oct 5, 2016 •

edited

Loading

geky Oct 5, 2016

kjbracey Oct 5, 2016

geky Oct 5, 2016

0xc0170 commented Oct 25, 2016

kjbracey commented Oct 25, 2016

mikaleppanen commented Oct 25, 2016

nsapi - Standardize support of NSAPI_UNSPEC #2897

nsapi - Standardize support of NSAPI_UNSPEC #2897

Conversation

geky commented Oct 3, 2016 • edited Loading

kjbracey commented Oct 4, 2016

kjbracey left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

geky commented Oct 4, 2016

kjbracey commented Oct 4, 2016

geky commented Oct 4, 2016 • edited Loading

sg- commented Oct 4, 2016

bridadan commented Oct 4, 2016

mbed-bot commented Oct 4, 2016

Output

bridadan commented Oct 4, 2016 • edited Loading

geky commented Oct 4, 2016

kjbracey commented Oct 5, 2016

kjbracey Oct 5, 2016 • edited Loading

Choose a reason for hiding this comment

kjbracey Oct 5, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kjbracey Oct 5, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

0xc0170 commented Oct 25, 2016

kjbracey commented Oct 25, 2016

mikaleppanen commented Oct 25, 2016

geky commented Oct 3, 2016 •

edited

Loading

geky commented Oct 4, 2016 •

edited

Loading

bridadan commented Oct 4, 2016 •

edited

Loading

kjbracey Oct 5, 2016 •

edited

Loading

kjbracey Oct 5, 2016 •

edited

Loading

kjbracey Oct 5, 2016 •

edited

Loading