Multihoming initial release #9387

tymoteuszblochmobica · 2019-01-15T16:57:25Z

Description

Added Multihoming feature to LWIP (ability to use more than one network interfaces) for increasing networking reliability.
This involves:

LWIP interface
LWIP IP routing
DNS storage
Sockets (bind to interface name possibility)
possibility to add non default network interface
cellular middleware modifications if cellular connection is used

Release notes

Network Interface and Network Stack API is extended with new members and new parameter to specify name of network interface
LWIP IP layer IP_route function is extended for selecting interface by its name.
NSAPI DNS now has storage for each interface's DNS servers.

New members :
NetworkInterface::set_as_default
NetworkInterface::get_interface name
NetworkStack::get_ip_address_if

LWIP::setsockopt has new option NSAPI_BIND_TO_DEVICE for binding socket to interface name.

New parameter "interface_name" addded to:
NetworkInterface::add_dns_server
NetworkInterface::gethostbyname

If selection based on interface name is not needed an empty tring can be passed.
Then original functionality is preserved.

Pull request type

[ ] Fix
[ ] Refactor
[ ] Target update
[] Functionality change
[ ] Docs update
[ ] Test update
[x ] Breaking change

Reviewers

@SeppoTakalo
@mikaleppanen
@kjbracey-arm

tymoteuszblochmobica · 2019-01-15T16:58:33Z

@SeppoTakalo please review
@mikaleppanen please review
@kjbracey-arm please review

cmonr · 2019-01-15T17:58:12Z

@tymoteuszblochmobica Please add a description to this PR, as it's completely unclear what this is.

Also, please take a look at the travis-ci/astyle job.

ciarmcom · 2019-01-15T18:00:34Z

@tymoteuszblochmobica, thank you for your changes.
@ARMmbed/mbed-os-test @ARMmbed/mbed-os-ipcore @ARMmbed/mbed-os-wan @ARMmbed/mbed-os-maintainers please review.

0xc0170 · 2019-01-16T09:01:18Z

Functionality change

This needs description here in the first commit but more importantly in the commit message. Introducing a new functionality change .
Multihoming initial release commit msg is good for simple fix that is easy to understand from the change.

SeppoTakalo

Thanks @tymoteuszblochmobica
Left couple of questions and suggestions...

SeppoTakalo · 2019-01-21T11:57:28Z

features/netsocket/nsapi_types.h

@@ -127,10 +127,14 @@ typedef enum nsapi_security {
    NSAPI_SECURITY_UNKNOWN      = 0xFF,     /*!< unknown/unsupported security in scan results */
 } nsapi_security_t;

-/** Maximum size of network interface name
+/** Size of  2 char network interface name from driver
 */
 #define NSAPI_INTERFACE_NAME_SIZE 2


Is this actually a networkinterface name prefix instead of full name?

Yes this is a 2 char prefix from network driver.LWIP netif_add appends 8bit sequence number so range 0-255 takes 3 chars plus '\0' gives 6 char total string length

SeppoTakalo · 2019-01-21T12:20:08Z

features/netsocket/nsapi_dns.cpp

+        query->interface_name = NULL;
+    } else {
+        query->interface_name = interface_name;
+    }


This if-else is not needed.
Even if interface_name is NULL, you can just do query->interface_name = interface_name;

SeppoTakalo · 2019-01-21T12:23:08Z

features/netsocket/SocketAddress.h

    {
-        _SocketAddress(nsapi_create_stack(stack), host, port);
+        _SocketAddress(nsapi_create_stack(stack), host, port, interface_name);


This is deprecated function, can we just use NULL and not touch the API?

That way we would not need to touch the SocketAddress at all, just call gethostbyname() with NULL interface.

SeppoTakalo · 2019-01-21T12:31:25Z

features/netsocket/NetworkStack.cpp

+
+    call_in_callback_cb_t call_in_cb = get_call_in_callback();
+
+    return nsapi_dns_query_async(this, name, callback, call_in_cb, interface_name, version);


There is only one line difference to the function above, so can these two be combined?

SeppoTakalo · 2019-01-21T12:32:09Z

features/netsocket/NetworkStack.cpp

+        }
+    }
+
+    return nsapi_dns_query(this, name, address, interface_name, version);


There is only one line difference to the function above, so can these two be combined?

SeppoTakalo · 2019-01-21T12:45:01Z

features/lwipstack/lwipopts.h

@@ -340,6 +340,8 @@
 #endif // MBED_CONF_LWIP_ETHERNET_ENABLED

 #define LWIP_L3IP   				0
+//Maximum size of network interface name
+#define NETWORK_INTERFACE_NAME_MAX_SIZE 4


Why this is 4 and NSAPI_INTERFACE_NAME_MAX_SIZE is 6?

SeppoTakalo · 2019-01-21T12:46:07Z

features/lwipstack/lwip/src/include/lwip/netif.h

@@ -136,6 +137,8 @@ enum lwip_internal_netif_client_data_index
 #define NETIF_CHECKSUM_DISABLE_ALL  0x0000
 #endif /* LWIP_CHECKSUM_CTRL_PER_NETIF */

+#define INTERFACE_NAME_SIZE    6


Can we use NSAPI_INTERFACE_NAME_MAX_SIZE here? is it the same?

INTERFACE_NAME_SIZE renamed to INTERFACE_NAME_MAX_SIZE
Modified lwipopts.h
#define INTERFACE_NAME_MAX_SIZE NSAPI_INTERFACE_NAME_MAX_SIZE to avoid including nsapi_types.h in lwip dns and netif

SeppoTakalo · 2019-01-21T12:48:03Z

features/lwipstack/lwip/src/include/lwip/dns.h

@@ -49,6 +49,12 @@
 extern "C" {
 #endif

+struct dns_server_interface {
+    char interface_name [NETWORK_INTERFACE_NAME_MAX_SIZE];


Why the name is copied here instead of referring to const char *?

I'm assuming that interface names do not change in run time.

This is a local storage for DNS servers for each interface name

SeppoTakalo · 2019-01-21T12:54:09Z

features/lwipstack/lwip/src/core/lwip_dns.c

  if (numdns < DNS_MAX_SERVERS) {
-    return &dns_servers[numdns];
+    return;


Is the comparison wrong way?

Documentation says must be < DNS_MAX_SERVERS then you immediately return if that is true.

Mistake during whitespace correction. Fixed

SeppoTakalo · 2019-01-21T13:02:04Z

components/wifi/esp8266-driver/ESP8266Interface.h

@@ -47,6 +47,8 @@
 */
 class ESP8266Interface : public NetworkStack, public WiFiInterface {
 public:
+    using NetworkStack::get_ip_address;
+    using WiFiInterface::connect;


Are we truly causing all external drivers to adapt to this new API?

We are not controlling all of those, so I would still consider whether this can be avoided.

mikaleppanen · 2019-01-22T13:59:39Z

features/lwipstack/LWIPInterface.cpp

@@ -457,6 +474,30 @@ nsapi_error_t LWIP::add_l3ip_interface(L3IP &l3ip, bool default_if, OnboardNetwo
 nsapi_error_t LWIP::remove_l3ip_interface(OnboardNetworkStack::Interface **interface_out)
 {
 #if LWIP_L3IP
+    if ((interface_out != NULL) && (*interface_out != NULL)) {
+
+        Interface *lwip =  static_cast<Interface *>(*interface_out);


extra space

mikaleppanen · 2019-01-22T14:24:49Z

features/lwipstack/LWIPStack.h

+         *
+         * @return       netif name  eg "en0"
+        */
+        virtual char *get_interface_name(char *buf);


Should this have buffer length as a parameter, also is returning the buffer needed?

mikaleppanen · 2019-01-22T14:26:17Z

features/lwipstack/lwip/src/core/ipv6/lwip_ip6.c

 {
  struct netif *netif;
  s8_t i;

+  if(interface_name !=NULL) {


correct spaces

mikaleppanen · 2019-01-22T14:31:29Z

features/lwipstack/lwip/src/core/lwip_netif.c

+  if (netif_default == netif) {
+    return true;
+  } else {
+	return false;


indentation

mikaleppanen · 2019-01-22T14:31:47Z

features/lwipstack/lwip/src/core/lwip_tcp.c

@@ -1911,13 +1911,14 @@ tcp_eff_send_mss_impl(u16_t sendmss, const ip_addr_t *dest
 #if LWIP_IPV6 || LWIP_IPV4_SRC_ROUTING
                     , const ip_addr_t *src
 #endif /* LWIP_IPV6 || LWIP_IPV4_SRC_ROUTING */
+					 , const char *interface_name


indentation

mikaleppanen · 2019-01-22T14:32:29Z

features/lwipstack/lwip/src/include/lwip/dns.h

@@ -49,6 +49,12 @@
 extern "C" {
 #endif

+struct dns_server_interface {
+    char interface_name [INTERFACE_NAME_SIZE];
+    ip_addr_t              dns_servers[DNS_MAX_SERVERS];


indentation

mikaleppanen · 2019-01-22T14:33:55Z

features/lwipstack/lwipopts.h

-#define LWIP_L3IP   				0
+#define LWIP_L3IP                   0
+//Maximum size of network interface name
+#define INTERFACE_NAME_SIZE 6


should this be defined to be NSAPI_INTERFACE_NAME_MAX_SIZE

INTERFACE_NAME_SIZE renamed to INTERFACE_NAME_MAX_SIZE
Modified lwipopts.h
#define INTERFACE_NAME_MAX_SIZE NSAPI_INTERFACE_NAME_MAX_SIZE to avoid including nsapi_types.h in lwip dns and netif

mikaleppanen · 2019-01-22T14:36:15Z

features/netsocket/DNS.h

+     *  @param address  Pointer to a SocketAddress to store the result.
+     *  @param version  IP version of address to resolve, NSAPI_UNSPEC indicates
+     *  @param interface_name  Network interface name
+     *                  version is chosen by the stack (defaults to NSAPI_UNSPEC).


parameters are other way around in function prototype. Also move "version is chosen..."

mikaleppanen · 2019-01-22T14:39:30Z

features/netsocket/NetworkInterface.h

+     *  @param address  Pointer to a SocketAddress to store the result.
+     *  @param version  IP version of address to resolve, NSAPI_UNSPEC indicates
+     *  @param interface_name  Network interface name
+     *                  version is chosen by the stack (defaults to NSAPI_UNSPEC).


Parameters are other way around, correct "version is..."

mikaleppanen · 2019-01-22T14:39:57Z

features/netsocket/NetworkInterface.h

+     *  @param callback Callback that is called for result.
+     *  @param version  IP version of address to resolve, NSAPI_UNSPEC indicates
+     *  @param interface_name  Network interface_name
+     *                  version is chosen by the stack (defaults to NSAPI_UNSPEC).


same as above

lauri-piikivi · 2019-01-29T07:07:44Z

features/netsocket/InternetSocket.cpp

@@ -174,6 +174,9 @@ nsapi_error_t InternetSocket::setsockopt(int level, int optname, const void *opt
        ret = NSAPI_ERROR_NO_SOCKET;
    } else {
        ret = _stack->setsockopt(_socket, level, optname, optval, optlen);
+        if (optname == NSAPI_BIND_TO_DEVICE && level == NSAPI_SOCKET) {
+            strncpy(_interface_name, static_cast<const char *>(optval), optlen);
+        }


We should get cloud client input on this as well. We make the interface binding (of client socket) with options, where as linux uses the bind operation. Now they need to adapt to both options

@JanneKiiskila Mind having someone chime in?

@yogpan01 @TeroJaasko @teetak01 @anttiylitokola to check it up, please.

This is now ready for integration, is there anything blocking ?

If it helps in any way, the mbed-client-testapp CI is passing fine with the most recent commit in this PR

AriParkkila · 2019-01-29T10:18:59Z

The current DNS implementation my return IPv4 address over IPv6 network interface, if version is not explicitly given for gethostbyname. This PR could also improve DNS query to prefer to the active IP version?

SeppoTakalo · 2019-01-29T10:53:42Z

@yogpan01 Who is the correct person from Client team to review this work?

Basically we need opinion what do you think about Socket to use setsockopt() to bind it into one particular interface. This differs from Linux where bind() can already do that.

0xc0170 · 2019-02-20T11:31:42Z

Too fast? I'll cancel the job (another force push)

mbed-ci · 2019-02-20T11:32:02Z

Test run: FAILED

Summary: 1 of 1 test jobs failed
Build number : 8
Build artifacts

Failed test jobs:

jenkins-ci/mbed-os-ci_unittests

michalpasztamobica · 2019-02-20T11:37:57Z

@0xc0170 , let's wait for @SeppoTakalo to double check if wiced binaries were built correctly. We've seen some failures in our local CI...

tymoteuszblochmobica · 2019-02-20T11:41:22Z

Unittests now build and pass after API change

SeppoTakalo · 2019-02-20T12:40:55Z

Yesterday I made a mistake and pushed Wiced binaries build against master.
Now this branch should have correct ones. I just rebuilt those.

I'll verify in CI, so wait a minute.

SeppoTakalo · 2019-02-20T12:48:50Z

Builds now.

@0xc0170 This can now be tested. Thanks.

0xc0170 · 2019-02-20T12:56:41Z

CI restarted

0xc0170 · 2019-02-20T14:39:42Z

While in CI

@mikaleppanen
@kjbracey-arm

Can you review please?

mbed-ci · 2019-02-20T16:21:59Z

Test run: SUCCESS

Summary: 12 of 12 test jobs passed
Build number : 9
Build artifacts

kjbracey

Could of things to follow up on, but seems fine.

kjbracey · 2019-02-21T09:48:52Z

features/lwipstack/LWIPStack.cpp

+            }
+
+            if (NETCONNTYPE_GROUP(s->conn->type) == NETCONN_TCP) {
+                s->conn->pcb.tcp->interface_name = (const char *)optval;


This bends the setsockopt API because it's not actually copying the option data. Can get away with it though.

This will be refactored in LWIP 2.1.2.
New LWIP has new socket to name bind implementation so code must be modified.

kjbracey · 2019-02-21T09:57:29Z

features/lwipstack/lwip/src/core/ipv4/lwip_ip4.c

@@ -160,6 +160,20 @@ ip4_route(const ip4_addr_t *dest)
  }
 #endif /* LWIP_MULTICAST_TX_OPTIONS */

+  if(interface_name !=NULL) {


Seems inefficient to be doing name matching inside the stack per-packet - I would have thought the setsockopt would do lookup at that time, and then you're just netif pointer matching per-packet. Still, just an efficiency nit.

Would mean adding a clean-up to strip stale pointers from sockets if you ever delete an interface.

On the other hand, would solve the "not copying/processing option data" issue on the setsockopt.

This will be refactored in LWIP 2.1.2.
New LWIP has modified ipxx_route function so name/ip matching must be implemented again.

0xc0170 · 2019-02-21T11:51:31Z

Still waiting for #9387 (comment) , if all resolved, let us know

cmonr added the needs: work label Jan 15, 2019

ciarmcom requested review from a team January 15, 2019 18:00

ciarmcom added the needs: review label Jan 15, 2019

cmonr removed the needs: review label Jan 15, 2019

tymoteuszblochmobica force-pushed the Sockets branch 2 times, most recently from c5fb7f8 to 5782d1b Compare January 16, 2019 10:22

cmonr added release-version: 5.12.0-rc1 needs: review and removed needs: work labels Jan 17, 2019

tymoteuszblochmobica force-pushed the Sockets branch 2 times, most recently from ea0fdb7 to d62a3f0 Compare January 21, 2019 12:14

SeppoTakalo reviewed Jan 21, 2019

View reviewed changes

tymoteuszblochmobica force-pushed the Sockets branch from d62a3f0 to d699def Compare January 21, 2019 16:06

mikaleppanen suggested changes Jan 22, 2019

View reviewed changes

0xc0170 added needs: work and removed needs: review labels Jan 22, 2019

tymoteuszblochmobica force-pushed the Sockets branch 4 times, most recently from 48a5cf4 to a3e6d5c Compare January 25, 2019 13:24

lauri-piikivi reviewed Jan 29, 2019

View reviewed changes

tymoteuszblochmobica force-pushed the Sockets branch from bf3b3f4 to 84e0b9c Compare February 20, 2019 11:35

0xc0170 added needs: review and removed needs: CI labels Feb 20, 2019

tymoteuszblochmobica and others added 2 commits February 20, 2019 14:37

Multihoming change parameter orded in gethostbyname

641189c

Rebuild new Wiced binaries

e45da3f

SeppoTakalo force-pushed the Sockets branch from 84e0b9c to e45da3f Compare February 20, 2019 12:37

0xc0170 added needs: CI and removed needs: review labels Feb 20, 2019

SeppoTakalo mentioned this pull request Feb 20, 2019

Various clarifications for NetworkInterface state and connection ARMmbed/mbed-os-5-docs#957

Merged

cmonr added needs: review and removed needs: CI labels Feb 20, 2019

kjbracey approved these changes Feb 21, 2019

View reviewed changes

0xc0170 added ready for merge and removed needs: review labels Feb 21, 2019

0xc0170 approved these changes Feb 21, 2019

View reviewed changes

0xc0170 merged commit dbd92c7 into ARMmbed:master Feb 21, 2019

0xc0170 removed the ready for merge label Feb 21, 2019

jeromecoutant mentioned this pull request Mar 26, 2019

Corrupt address returned in an mbed-os DNS greentea test suite ARMmbed/wifi-ism43362#41

Closed

tymoteuszblochmobica deleted the Sockets branch April 25, 2019 12:26

michalpasztamobica mentioned this pull request Oct 31, 2019

Add API to get ipv6 link local address #11279

Merged


		call_in_callback_cb_t call_in_cb = get_call_in_callback();

		return nsapi_dns_query_async(this, name, callback, call_in_cb, interface_name, version);

Multihoming initial release #9387

Multihoming initial release #9387

Uh oh!

Conversation

tymoteuszblochmobica commented Jan 15, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Release notes

Pull request type

Reviewers

Uh oh!

tymoteuszblochmobica commented Jan 15, 2019

Uh oh!

cmonr commented Jan 15, 2019

Uh oh!

ciarmcom commented Jan 15, 2019

Uh oh!

0xc0170 commented Jan 16, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

SeppoTakalo left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

tymoteuszblochmobica commented Jan 15, 2019 •

edited

Loading

0xc0170 commented Jan 16, 2019 •

edited

Loading