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

Multihoming initial release #9387

Merged
merged 4 commits into from Feb 21, 2019

Conversation

@tymoteuszblochmobica
Copy link
Contributor

commented Jan 15, 2019

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

This comment has been minimized.

Copy link
Contributor Author

commented Jan 15, 2019

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

@cmonr cmonr added the needs: work label Jan 15, 2019

@cmonr

This comment has been minimized.

Copy link
Contributor

commented Jan 15, 2019

@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 ciarmcom requested review from ARMmbed/mbed-os-maintainers Jan 15, 2019

@ciarmcom

This comment has been minimized.

Copy link
Member

commented Jan 15, 2019

@0xc0170

This comment has been minimized.

Copy link
Member

commented Jan 16, 2019

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.

@tymoteuszblochmobica tymoteuszblochmobica force-pushed the tymoteuszblochmobica:Sockets branch 2 times, most recently Jan 16, 2019

@tymoteuszblochmobica tymoteuszblochmobica force-pushed the tymoteuszblochmobica:Sockets branch 2 times, most recently Jan 17, 2019

@SeppoTakalo
Copy link
Contributor

left a comment

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

features/netsocket/nsapi_types.h Outdated
@@ -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

This comment has been minimized.

Copy link
@SeppoTakalo

SeppoTakalo Jan 21, 2019

Contributor

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

This comment has been minimized.

Copy link
@tymoteuszblochmobica

tymoteuszblochmobica Jan 24, 2019

Author Contributor

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

features/netsocket/nsapi_dns.cpp Outdated
query->interface_name = NULL;
} else {
query->interface_name = interface_name;
}

This comment has been minimized.

Copy link
@SeppoTakalo

SeppoTakalo Jan 21, 2019

Contributor

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

This comment has been minimized.

Copy link
@tymoteuszblochmobica

tymoteuszblochmobica Jan 24, 2019

Author Contributor

done

features/netsocket/SocketAddress.h Outdated
{
_SocketAddress(nsapi_create_stack(stack), host, port);
_SocketAddress(nsapi_create_stack(stack), host, port, interface_name);

This comment has been minimized.

Copy link
@SeppoTakalo

SeppoTakalo Jan 21, 2019

Contributor

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.

This comment has been minimized.

Copy link
@tymoteuszblochmobica

tymoteuszblochmobica Jan 24, 2019

Author Contributor

done


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

This comment has been minimized.

Copy link
@SeppoTakalo

SeppoTakalo Jan 21, 2019

Contributor

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

This comment has been minimized.

Copy link
@tymoteuszblochmobica

tymoteuszblochmobica Jan 24, 2019

Author Contributor

done

}
}

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

This comment has been minimized.

Copy link
@SeppoTakalo

SeppoTakalo Jan 21, 2019

Contributor

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

This comment has been minimized.

Copy link
@tymoteuszblochmobica

tymoteuszblochmobica Jan 24, 2019

Author Contributor

done

features/lwipstack/lwipopts.h Outdated
@@ -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

This comment has been minimized.

Copy link
@SeppoTakalo

SeppoTakalo Jan 21, 2019

Contributor

Why this is 4 and NSAPI_INTERFACE_NAME_MAX_SIZE is 6?

This comment has been minimized.

Copy link
@tymoteuszblochmobica

tymoteuszblochmobica Jan 24, 2019

Author Contributor

Fixed

features/lwipstack/lwip/src/include/lwip/netif.h Outdated
@@ -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

This comment has been minimized.

Copy link
@SeppoTakalo

SeppoTakalo Jan 21, 2019

Contributor

Can we use NSAPI_INTERFACE_NAME_MAX_SIZE here? is it the same?

This comment has been minimized.

Copy link
@tymoteuszblochmobica

tymoteuszblochmobica Jan 25, 2019

Author Contributor

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

features/lwipstack/lwip/src/include/lwip/dns.h Outdated
@@ -49,6 +49,12 @@
extern "C" {
#endif

struct dns_server_interface {
char interface_name [NETWORK_INTERFACE_NAME_MAX_SIZE];

This comment has been minimized.

Copy link
@SeppoTakalo

SeppoTakalo Jan 21, 2019

Contributor

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 comment has been minimized.

Copy link
@tymoteuszblochmobica

tymoteuszblochmobica Jan 24, 2019

Author Contributor

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

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

This comment has been minimized.

Copy link
@SeppoTakalo

SeppoTakalo Jan 21, 2019

Contributor

Is the comparison wrong way?

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

This comment has been minimized.

Copy link
@tymoteuszblochmobica

tymoteuszblochmobica Jan 24, 2019

Author Contributor

Mistake during whitespace correction. Fixed

components/wifi/esp8266-driver/ESP8266Interface.h Outdated
@@ -47,6 +47,8 @@
*/
class ESP8266Interface : public NetworkStack, public WiFiInterface {
public:
using NetworkStack::get_ip_address;
using WiFiInterface::connect;

This comment has been minimized.

Copy link
@SeppoTakalo

SeppoTakalo Jan 21, 2019

Contributor

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.

This comment has been minimized.

Copy link
@tymoteuszblochmobica

tymoteuszblochmobica Jan 24, 2019

Author Contributor

Removed

@tymoteuszblochmobica tymoteuszblochmobica force-pushed the tymoteuszblochmobica:Sockets branch Jan 21, 2019

features/lwipstack/LWIPInterface.cpp Outdated
@@ -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);

This comment has been minimized.

Copy link
@mikaleppanen

mikaleppanen Jan 22, 2019

Contributor

extra space

This comment has been minimized.

Copy link
@tymoteuszblochmobica

tymoteuszblochmobica Jan 25, 2019

Author Contributor

done

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

This comment has been minimized.

Copy link
@mikaleppanen

mikaleppanen Jan 22, 2019

Contributor

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

features/lwipstack/lwip/src/core/ipv6/lwip_ip6.c Outdated
{
struct netif *netif;
s8_t i;

if(interface_name !=NULL) {

This comment has been minimized.

Copy link
@mikaleppanen

mikaleppanen Jan 22, 2019

Contributor

correct spaces

This comment has been minimized.

Copy link
@tymoteuszblochmobica

tymoteuszblochmobica Jan 25, 2019

Author Contributor

done

features/lwipstack/lwip/src/core/lwip_netif.c Outdated
if (netif_default == netif) {
return true;
} else {
return false;

This comment has been minimized.

Copy link
@mikaleppanen

mikaleppanen Jan 22, 2019

Contributor

indentation

This comment has been minimized.

Copy link
@tymoteuszblochmobica

tymoteuszblochmobica Jan 25, 2019

Author Contributor

done

features/lwipstack/lwip/src/core/lwip_tcp.c Outdated
@@ -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

This comment has been minimized.

Copy link
@mikaleppanen

mikaleppanen Jan 22, 2019

Contributor

indentation

This comment has been minimized.

Copy link
@tymoteuszblochmobica

tymoteuszblochmobica Jan 25, 2019

Author Contributor

done

features/lwipstack/lwip/src/include/lwip/dns.h Outdated
@@ -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];

This comment has been minimized.

Copy link
@mikaleppanen

mikaleppanen Jan 22, 2019

Contributor

indentation

This comment has been minimized.

Copy link
@tymoteuszblochmobica

tymoteuszblochmobica Jan 25, 2019

Author Contributor

done

features/lwipstack/lwipopts.h Outdated
#define LWIP_L3IP 0
#define LWIP_L3IP 0
//Maximum size of network interface name
#define INTERFACE_NAME_SIZE 6

This comment has been minimized.

Copy link
@mikaleppanen

mikaleppanen Jan 22, 2019

Contributor

spaces

This comment has been minimized.

Copy link
@mikaleppanen

mikaleppanen Jan 22, 2019

Contributor

should this be defined to be NSAPI_INTERFACE_NAME_MAX_SIZE

This comment has been minimized.

Copy link
@tymoteuszblochmobica

tymoteuszblochmobica Jan 25, 2019

Author Contributor

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

* @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).

This comment has been minimized.

Copy link
@mikaleppanen

mikaleppanen Jan 22, 2019

Contributor

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

This comment has been minimized.

Copy link
@tymoteuszblochmobica

tymoteuszblochmobica Jan 25, 2019

Author Contributor

done

* @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).

This comment has been minimized.

Copy link
@mikaleppanen

mikaleppanen Jan 22, 2019

Contributor

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

* @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).

This comment has been minimized.

Copy link
@mikaleppanen

mikaleppanen Jan 22, 2019

Contributor

same as above

This comment has been minimized.

Copy link
@tymoteuszblochmobica

tymoteuszblochmobica Jan 25, 2019

Author Contributor

done

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

@tymoteuszblochmobica tymoteuszblochmobica force-pushed the tymoteuszblochmobica:Sockets branch 4 times, most recently to a3e6d5c Jan 24, 2019

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

This comment has been minimized.

Copy link
@lauri-piikivi

lauri-piikivi Jan 29, 2019

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

This comment has been minimized.

Copy link
@cmonr

cmonr Feb 12, 2019

Contributor

@JanneKiiskila Mind having someone chime in?

This comment has been minimized.

Copy link
@JanneKiiskila

This comment has been minimized.

Copy link
@0xc0170

0xc0170 Feb 21, 2019

Member

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

This comment has been minimized.

Copy link
@michalpasztamobica

michalpasztamobica Feb 21, 2019

Contributor

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

@AriParkkila

This comment has been minimized.

Copy link
Contributor

commented Jan 29, 2019

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

This comment has been minimized.

Copy link
Contributor

commented Jan 29, 2019

@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

This comment has been minimized.

Copy link
Member

commented Feb 20, 2019

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

@mbed-ci

This comment has been minimized.

Copy link

commented Feb 20, 2019

Test run: FAILED

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

Failed test jobs:

  • jenkins-ci/mbed-os-ci_unittests

@tymoteuszblochmobica tymoteuszblochmobica force-pushed the tymoteuszblochmobica:Sockets branch Feb 20, 2019

@michalpasztamobica

This comment has been minimized.

Copy link
Contributor

commented Feb 20, 2019

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

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

@tymoteuszblochmobica

This comment has been minimized.

Copy link
Contributor Author

commented Feb 20, 2019

Unittests now build and pass after API change

@SeppoTakalo SeppoTakalo force-pushed the tymoteuszblochmobica:Sockets branch to e45da3f Feb 20, 2019

@SeppoTakalo

This comment has been minimized.

Copy link
Contributor

commented Feb 20, 2019

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

This comment has been minimized.

Copy link
Contributor

commented Feb 20, 2019

image

Builds now.

@0xc0170 This can now be tested. Thanks.

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

@0xc0170

This comment has been minimized.

Copy link
Member

commented Feb 20, 2019

CI restarted

@0xc0170

This comment has been minimized.

Copy link
Member

commented Feb 20, 2019

While in CI

@mikaleppanen
@kjbracey-arm

Can you review please?

@mbed-ci

This comment has been minimized.

Copy link

commented Feb 20, 2019

Test run: SUCCESS

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

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

@kjbracey-arm
Copy link
Contributor

left a comment

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

}

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

This comment has been minimized.

Copy link
@kjbracey-arm

kjbracey-arm Feb 21, 2019

Contributor

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

This comment has been minimized.

Copy link
@tymoteuszblochmobica

tymoteuszblochmobica Feb 21, 2019

Author Contributor

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

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

if(interface_name !=NULL) {

This comment has been minimized.

Copy link
@kjbracey-arm

kjbracey-arm Feb 21, 2019

Contributor

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 comment has been minimized.

Copy link
@tymoteuszblochmobica

tymoteuszblochmobica Feb 21, 2019

Author Contributor

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

This comment has been minimized.

Copy link
Member

commented Feb 21, 2019

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

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

27 checks passed

continuous-integration/jenkins/pr-head This commit looks good
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
jenkins-ci/build-ARM Success
Details
jenkins-ci/build-ARMC6 Success
Details
jenkins-ci/build-GCC_ARM Success
Details
jenkins-ci/build-IAR Success
Details
jenkins-ci/cloud-client-test Success
Details
jenkins-ci/dynamic-memory-usage RTOS ROM(+0 bytes) RAM(+0 bytes)
Details
jenkins-ci/exporter Success
Details
jenkins-ci/greentea-test Success
Details
jenkins-ci/mbed2-build-ARM Success
Details
jenkins-ci/mbed2-build-GCC_ARM Success
Details
jenkins-ci/mbed2-build-IAR Success
Details
jenkins-ci/unittests Success
Details
travis-ci/astyle Local astyle testing has passed
Details
travis-ci/docs Local docs testing has passed
Details
travis-ci/doxy-spellcheck Local doxy-spellcheck testing has passed
Details
travis-ci/events Passed, runtime is 9012 cycles
Details
travis-ci/gitattributestest Local gitattributestest testing has passed
Details
travis-ci/include_check Local include_check testing has passed
Details
travis-ci/licence_check Local licence_check testing has passed
Details
travis-ci/littlefs Passed, code size is 8408B
Details
travis-ci/psa-autogen Local psa-autogen testing has passed
Details
travis-ci/tools-py2.7 Local tools-py2.7 testing has passed
Details
travis-ci/tools-py3.5 Local tools-py3.5 testing has passed
Details
travis-ci/tools-py3.6 Local tools-py3.6 testing has passed
Details
travis-ci/tools-py3.7 Local tools-py3.7 testing has passed
Details

@tymoteuszblochmobica tymoteuszblochmobica deleted the tymoteuszblochmobica:Sockets branch Apr 25, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.