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

LwIP: add ICMPv4 Socket support #10978

Merged
merged 12 commits into from
Oct 31, 2019
Merged

LwIP: add ICMPv4 Socket support #10978

merged 12 commits into from
Oct 31, 2019

Conversation

vmedcy
Copy link
Contributor

@vmedcy vmedcy commented Jul 5, 2019

Description

Implement ICMPSocket class, make LWIP_RAW option configurable with lwip.raw_socket_enabled, and update socket_open to support nsapi_protocol_t::NSAPI_ICMP.

This allows the application layer to hook into the IP layer itself.

This feature supports only ICMP socket (ICMPv4) and not RAW Socket

Pull request type

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

Reviewers

@kjbracey-arm @tymoteuszblochmobica

Release Notes

netsocket feature is extended with ICMP socket implementation. This change is only for ICMPv4 and ICMPv6 is not currently supported

@ciarmcom ciarmcom requested review from kjbracey, tymoteuszblochmobica and a team July 5, 2019 15:00
@ciarmcom
Copy link
Member

ciarmcom commented Jul 5, 2019

@vmedcy, thank you for your changes.
@tymoteuszblochmobica @kjbracey-arm @ARMmbed/mbed-os-ipcore @ARMmbed/mbed-os-maintainers please review.

@@ -84,7 +84,7 @@
#define SYS_LIGHTWEIGHT_PROT 1

#ifndef LWIP_RAW
#define LWIP_RAW 0
#define LWIP_RAW 1
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be configurable via JSON, like TCP. Enabling raw socket support will increase build size of lwIP, and most people will not need it.

I think it should be off by default, but maybe that could be debated.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes I agree.

@@ -184,6 +184,7 @@ class NetworkStack: public DNS {
protected:
friend class InternetSocket;
friend class UDPSocket;
friend class RAWSocket;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

RawIPSocket. RAW is not an acronym, and I think we need to distinguish from non-IP cases.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If there some way to create an ICMP Socket by re-using UDPSocket class ( such as InternetDatagram Socket with ICMP protocol it would be nice)

@@ -127,6 +127,7 @@ typedef enum nsapi_security {
NSAPI_SECURITY_CHAP = 0x6, /*!< phrase conforms to PPP authentication context */
NSAPI_SECURITY_EAP_TLS = 0x7, /*!< phrase conforms to EAP-TLS */
NSAPI_SECURITY_PEAP = 0x8, /*!< phrase conforms to PEAP */
NSAPI_SECURITY_WPA2_ENT = 0x9, /*!< phrase conforms to WPA2-AES and WPA-TKIP with enterprise security */
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems separate to this PR. Can you make it separate - group it with any other change it applies too.

@@ -266,6 +268,7 @@ typedef enum nsapi_socket_option {
NSAPI_ADD_MEMBERSHIP, /*!< Add membership to multicast address */
NSAPI_DROP_MEMBERSHIP, /*!< Drop membership to multicast address */
NSAPI_BIND_TO_DEVICE, /*!< Bind socket network interface name*/
NSAPI_IP_TOS, /*!< IP TOS */
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Separate out from this PR - if you're adding it, I would want to see at least 1 of lwIP or Nanostack support to illustrate the parameter type.

*
* To be used within accept() function. Close() will clean this up.
*/
RAWSocket(RAWSocket *parent, nsapi_socket_t socket, SocketAddress address);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was copied from TCPSocket? Not needed for raw.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I made this change by copying from UDPSocket.cpp, yes it is not needed for RAWSocket

features/lwipstack/LWIPStack.cpp Show resolved Hide resolved
#include "Timer.h"
#include "mbed_assert.h"

RAWSocket::RAWSocket()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a big duplication of UDPSocket (another one after CellularNonIPSocket). In this case it's so similar, because it's NSAPI, that I'm thinking there should be an InternetDatagramSocket base class to subsume the commonality, that both this and UDPSocket inherit from.

What do others think?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes InternetDatagramSocket can be a base class, if Application needs to create an ICMP socket they can use this class.

@maclobdell
Copy link
Contributor

@balajicyp @vmedcy - Can you confirm the status of this PR from your side? It is showing changes requested. Is there a clear way forward?

@0xc0170
Copy link
Contributor

0xc0170 commented Aug 20, 2019

@balajicyp @vmedcy - Can you confirm the status of this PR from your side? It is showing changes requested. Is there a clear way forward?

If this comes to 5.14, it should be updated soon.

@vmedcy
Copy link
Contributor Author

vmedcy commented Aug 20, 2019

@maclobdell @0xc0170 :

If this comes to 5.14, it should be updated soon.

I expect @balajicyp will respond next week after return from vacation.

@0xc0170
Copy link
Contributor

0xc0170 commented Aug 26, 2019

If this wants to be in 5.14, it needs to be updated soon as 5.14 code freeze is end of this week.

@balajicyp
Copy link
Contributor

I will take up this work this week.
I will create a new InternetDatagram socket class , which will have parameter as NSAPI_ICMP. The UDP socket class can inherit from this InternetDatagramSocket Class and have methods which can over-ride the base class.

The Enabling of the option LWIP_RAW will be done by *overrides in Application json file, so that by default this option will be off.

@balajicyp
Copy link
Contributor

I have my changes here at 23b0891 , I try to push to same PR but there is some refs which are mismatched

@0xc0170 @vmedcy @@maclobdell

@balajicyp
Copy link
Contributor

I have the new changes pushed here, please review and merge

@bulislaw
Copy link
Member

@kjbracey-arm @AnttiKauppila please re-review.

} else if (proto == NSAPI_UDP) {
netconntype = NETCONN_UDP;
} else {
netconntype = NETCONN_RAW;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For future-proofing, can you make this error for unknown proto? I know it didn't before, but it should have,

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do you mean the known proto be NASPI_ERROR_PROTO_UNKNOWN and return from function as below.
...
} else if (proto == NSAPI_UDP) {
netconntype = NETCONN_UDP;
} else if { proto == NSAPI_ICMP) {
netconntype = NETCONN_RAW;
} else {
return NSAPI_ERROR_PROTO_UNKNOWN;
}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

removed NSAPI_ERROR_PROTO_UNKNOWN

#include "Timer.h"
#include "mbed_assert.h"

RAWIPSocket::RAWIPSocket()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In earlier review we agreed this should become ICMPSocket.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes I can change it to ICMPSocket.h and ICMPSocket.cpp and change class names accordingly.

"The RAWSocket(S *stack) constructor is deprecated"
"It discards the open() call return value."
"Use another constructor and call open() explicitly, instead.")
RAWIPSocket(S *stack)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No need to include a deprecated constructor in a new class.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok I will remove RAWIPSocket(S *stack)



/** UDP socket implementation.
*/
class UDPSocket : public InternetSocket {
class UDPSocket : public RAWIPSocket {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The code sharing is good, but the class heirarchy is off. This lets you pass a UDPSocket * to someone expecting a RAWIPSocket *. Previous discussion suggested inserting an underlying datagram socket base class.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kjbracey-arm, Please specify more details what functions Datagram Socket Base class should have? is it all the methods except for the constructor and get_proto()

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kjbracey-arm, I have added the InternetDatagram.h/InternetDatagram.cpp as Base Class, Please review.

@0xc0170
Copy link
Contributor

0xc0170 commented Aug 29, 2019

PR was updated, @kjbracey-arm @tymoteuszblochmobica Please review

Note, we are close to the 5.14 code freeze. This needs update asap

Copy link
Contributor

@kjbracey kjbracey left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nearly there. Just minor tidies and elimination of unneeded bits.

features/netsocket/ICMPSocket.h Outdated Show resolved Hide resolved
features/netsocket/InternetDatagram.cpp Outdated Show resolved Hide resolved
features/netsocket/InternetDatagram.h Outdated Show resolved Hide resolved
features/netsocket/InternetDatagram.cpp Outdated Show resolved Hide resolved
features/netsocket/InternetDatagram.h Outdated Show resolved Hide resolved
features/netsocket/NetworkStack.h Outdated Show resolved Hide resolved
features/netsocket/UDPSocket.h Outdated Show resolved Hide resolved
features/netsocket/nsapi_types.h Outdated Show resolved Hide resolved
features/lwipstack/LWIPStack.cpp Outdated Show resolved Hide resolved
features/lwipstack/mbed_lib.json Outdated Show resolved Hide resolved
@0xc0170
Copy link
Contributor

0xc0170 commented Aug 29, 2019

@vmedcy Let us know once answered reviewers comments above. IF this is targeting 5.14, needs to be updated asap to get its approvals otherwise will be moved to the next minor version.

Copy link
Contributor

@balajicyp balajicyp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kjbracey-arm, I have uploaded the latest changes based on your comments, please review the same.

@mbed-ci
Copy link

mbed-ci commented Oct 28, 2019

Test run: FAILED

Summary: 1 of 4 test jobs failed
Build number : 2
Build artifacts

Failed test jobs:

  • jenkins-ci/mbed-os-ci_unittests

@0xc0170
Copy link
Contributor

0xc0170 commented Oct 28, 2019

One more commit to be added

It was quicker than I thought :)
Here is a commit which fixes the unittests.
This is the branch.

@vmedcy Please see this response above - it should fix unittests, cherry-pick it to your branch

@balajicyp
Copy link
Contributor

@0xc0170
I have re-based my code to the commit done by @michalpasztamobica . Please check.

@0xc0170
Copy link
Contributor

0xc0170 commented Oct 29, 2019

CI restarted

@mbed-ci
Copy link

mbed-ci commented Oct 29, 2019

Test run: SUCCESS

Summary: 11 of 11 test jobs passed
Build number : 3
Build artifacts

@0xc0170 0xc0170 added ready for merge release-version: 6.0.0-alpha-1 First pre-release version of 6.0.0 and removed needs: CI labels Oct 30, 2019
@0xc0170
Copy link
Contributor

0xc0170 commented Oct 30, 2019

Release Notes

@vmedcy One missing thing for this one, please add release notes for this addition.

@kjbracey
Copy link
Contributor

kjbracey commented Oct 30, 2019

The basic description and summary needs to be updated too - it's now adding ICMPSocket specifically, not a general raw IP socket.

@kjbracey
Copy link
Contributor

On which note, I've very belatedly realised that this implementation is specifically making an ICMPv4 socket (IP_PROTO_ICMP = 1). For IPv6 you want to be speaking ICMPv6 (IP_NEXTH_ICMP6 = 58).

ICMPv6 is a different protocol, so I guess it logically would be a separate ICMP6Socket class, and a separate NSAPI_ICMP6. There's nothing fundamentally stopping you speaking ICMPv6 on IPv4 or ICMPv4 on IPv6. It would just be dumb. An application has to know which protocol it wants to speak, you can't quietly make ICMPSocket choose different protocol numbers based on address.

So this is fine. But it should be mentioned in the release notes/docs that this is ICMPv4, and ICMPv6 is not currently supported.

@balajicyp
Copy link
Contributor

@kjbracey-arm @0xc0170 @vmedcy
I am unable to update the release notes section or description. The Author for this is @vmedcy

@vmedcy, Please update the release notes section and the description to say this feature supports only ICMP socket (ICMPv4) and not RAW Socket

@vmedcy
Copy link
Contributor Author

vmedcy commented Oct 30, 2019

@balajicyp I updated the release notes and description appropriately.

@balajicyp
Copy link
Contributor

Thanks @vmedcy

@vmedcy vmedcy changed the title LwIP: add RAW socket support LwIP: add ICMPv4 Socket support Oct 30, 2019
@0xc0170 0xc0170 merged commit da6b336 into ARMmbed:master Oct 31, 2019
@0xc0170 0xc0170 added release-version: 5.15.0-rc1 and removed release-version: 6.0.0-alpha-1 First pre-release version of 6.0.0 labels Nov 19, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.