Create abstract Socket interface #7192

SeppoTakalo · 2018-06-12T11:31:30Z

Description

Create abstract Socket interface with only virtual functions. This is protocol independent
Move IP Socket stuff to InternetSocket class which is inherited by TCP/UDP
Implement sendto() and recvfrom() on TCP socket
Implement connect() call on UDP
Implement send() and recv() calls on UDP socket
Define abstract Socket::accept() call
Deprecate TCPServer. TCPSocket now support listen() and accept() calls.

Pull request type

[ ] Fix
[X] Refactor
[ ] New target
[ ] Feature
[ ] Breaking change

SeppoTakalo · 2018-06-12T11:31:54Z

@kjbracey-arm For early review, please check.

SeppoTakalo · 2018-06-12T11:45:04Z

Testing on K64F+GCC_ARM

mbedgt: test suite report:
+--------------+---------------+-----------------------------+--------+--------------------+-------------+
| target       | platform_name | test suite                  | result | elapsed_time (sec) | copy_method |
+--------------+---------------+-----------------------------+--------+--------------------+-------------+
| K64F-GCC_ARM | K64F          | mbed-os-tests-netsocket-tcp | OK     | 51.44              | shell       |
| K64F-GCC_ARM | K64F          | mbed-os-tests-netsocket-udp | OK     | 25.27              | shell       |
+--------------+---------------+-----------------------------+--------+--------------------+-------------+
mbedgt: test suite results: 2 OK
mbedgt: test case report:
+--------------+---------------+-----------------------------+------------------------------------+--------+--------+--------+--------------------+
| target       | platform_name | test suite                  | test case                          | passed | failed | result | elapsed_time (sec) |
+--------------+---------------+-----------------------------+------------------------------------+--------+--------+--------+--------------------+
| K64F-GCC_ARM | K64F          | mbed-os-tests-netsocket-tcp | TCPSOCKET_ECHOTEST                 | 1      | 0      | OK     | 2.12               |
| K64F-GCC_ARM | K64F          | mbed-os-tests-netsocket-tcp | TCPSOCKET_ECHOTEST_NONBLOCK        | 1      | 0      | OK     | 23.05              |
| K64F-GCC_ARM | K64F          | mbed-os-tests-netsocket-tcp | TCPSOCKET_OPEN_CLOSE_REPEAT        | 1      | 0      | OK     | 0.06               |
| K64F-GCC_ARM | K64F          | mbed-os-tests-netsocket-tcp | TCPSOCKET_OPEN_LIMIT               | 1      | 0      | OK     | 0.22               |
| K64F-GCC_ARM | K64F          | mbed-os-tests-netsocket-tcp | TCPSOCKET_THREAD_PER_SOCKET_SAFETY | 1      | 0      | OK     | 1.34               |
| K64F-GCC_ARM | K64F          | mbed-os-tests-netsocket-udp | UDPSOCKET_ECHOTEST_NONBLOCK        | 1      | 0      | OK     | 2.69               |
| K64F-GCC_ARM | K64F          | mbed-os-tests-netsocket-udp | UDPSOCKET_OPEN_CLOSE_REPEAT        | 1      | 0      | OK     | 0.05               |
| K64F-GCC_ARM | K64F          | mbed-os-tests-netsocket-udp | UDPSOCKET_OPEN_LIMIT               | 1      | 0      | OK     | 0.22               |
| K64F-GCC_ARM | K64F          | mbed-os-tests-netsocket-udp | UDPSOCKET_SENDTO_TIMEOUT           | 1      | 0      | OK     | 0.08               |
+--------------+---------------+-----------------------------+------------------------------------+--------+--------+--------+--------------------+

So I'm assuming I did not break anything major.

kjbracey · 2018-06-12T12:25:03Z

features/netsocket/InternetSocket.cpp

    : _stack(0)
    , _socket(0)
    , _timeout(osWaitForever)
+    , _remote_peer()


No need to pop this in for something with a default constructor

kjbracey · 2018-06-12T12:31:16Z

features/netsocket/Socket.h

     *
     *  Closes socket if the socket is still open
     */
    virtual ~Socket() {}

-    /** Opens a socket
+    /** Close the socket.


I think listen() and accept() should go into the base level. They're as theoretically generic as all the other functions, and it would be harder to change later - particularly if you're trying to keep Socket totally abstract to the point of excluding default "unsupported" errors.

Accept is a bit painfull because it does not return any abstract type, it needs a pointer of already allocated object which it then fills in.
nsapi_error_t accept(TCPSocket *connection, SocketAddress *address = NULL);

One option would be that we create new accept() call that is able to work with abstract types. After all, accepting from TCPserver is fine to return Socket* and just use it and call close() after you are done.

Ah, right, I recall this discussion now. Any generic accept would need a different prototype, yes. So maybe define that now, and leave the other one deprecated in TCPServer?

Generic accept() call now implemented

kjbracey · 2018-06-12T12:31:59Z

features/netsocket/TCPServer.cpp

+
+nsapi_error_t TCPServer::connect(const SocketAddress &address)
+{
+    (void) address;


As it's C++, how's about just eliminating/commenting the parameter name?

kjbracey · 2018-06-12T12:33:44Z

features/netsocket/TCPServer.h

@@ -28,7 +28,7 @@
 /** TCP socket server
 *  @addtogroup netsocket
 */
-class TCPServer : public Socket {
+class TCPServer : public InternetSocket {


Not totally convinced that TCPServer needs to exist as a separate class, but I guess it does avoid the implementations of listen and accept being pulled in unless needed. Would it be more or less efficient to derive from TCPSocket, do you think?

TCPSocket now support listen() and accept(). TCPServer deprecated, but left the implementation in place to support old code.

kjbracey · 2018-06-12T12:36:04Z

features/netsocket/UDPSocket.cpp

+nsapi_size_or_error_t UDPSocket::recv(void *buffer, nsapi_size_t size)
+{
+    SocketAddress ignored; // Dangerous, I'm spending ~50 bytes from stack for address space that I'll ignore.
+    return recvfrom(&ignored, buffer, size);


Do we believe NSAPI stacks won't cope with being passed NULL? If so, this dummy actually needs to be in UDPSocket::recvfrom, as we want recv to be equivalent to recvfrom(NULL).

kjbracey · 2018-06-12T12:36:56Z

features/netsocket/InternetSocket.h

+    mbed::Callback<void()> _event;
+    mbed::Callback<void()> _callback;
+    rtos::Mutex _lock;
+    SocketAddress _remote_peer;


To minimise bloat, could we store the underlying ns_addr, or whatever it is, excluding the string rep?

I refactored the SocketAddress to allocate string only if its requested.

kjbracey · 2018-06-12T12:37:35Z

features/netsocket/TCPServer.h

+
+    /** Not supported on TCPServer.
+     * @param data unused
+     * @param sizez unused


Typo - showed up in CI

kjbracey · 2018-06-12T12:38:11Z

features/netsocket/TCPSocket.cpp

@@ -79,6 +79,10 @@ nsapi_error_t TCPSocket::connect(const SocketAddress &address)

    _write_in_progress = false;

+    if (ret == NSAPI_ERROR_OK) {


This needs to be below the following IS_CONNECTED->OK conversion.

But is it then creating a loophole where you first set socket to non blocking mode. Then call connect(A) and later connect(B) would actually change the connection address here, even that the actual connection is with peer A?

I'm unsure whether IP stacks actually check the connection addresses after a first call.

Okay, but in non-blocking there's possibly never an "OK" - it's "INPROGESS", "ALREADY", "ISCONN". If you don't want to note it on "ISCONN" then note it on "INPROGRESS". That should be the return code for "I've accepted these parameters and am acting on them, but I've not finished yet".

kjbracey · 2018-06-12T12:40:19Z

features/netsocket/Socket.h

+    /** Receive data from a socket.
+     *
+     *  Receive data from connected socket or in case of connectionless socket
+     *  this is equivalent of calling recvfrom(NULL, data, size).


I still can't decide whether we should actually have final implementations of send/recv here to enforce the equivalence.

Do you mean that send() would be just sendto(NULL, data, size);

That's what I'm pondering, but looking more closely at the POSIX specs I actually don't see the sendto(NULL) == send() equivalence - only send(0) == write and connection-mode sendto() == send().

SeppoTakalo · 2018-06-18T11:52:40Z

@kjbracey-arm Please re-review

SeppoTakalo · 2018-06-19T13:48:54Z

@mikaleppanen Please review

kjbracey · 2018-06-19T13:45:04Z

features/netsocket/SocketAddress.h

@@ -147,6 +153,12 @@ class SocketAddress {
     */
    operator bool() const;

+    /** Copy addres from another SocketAddress


kjbracey · 2018-06-19T13:48:10Z

features/netsocket/SocketAddress.cpp

@@ -180,7 +183,8 @@ const char *SocketAddress::get_ip_address() const
        return NULL;
    }

-    if (!_ip_address[0]) {
+    if (!_ip_address) {
+        _ip_address = new char[NSAPI_IP_SIZE];


I guess you need an "else" here now, as this will leave the address undefined, not "", if somehow version is not v4 or v6.

Maybe you could actually put the "new" in the ifs? Would allow you to have a smaller buffer for IPv4.

(Another option would be to convert into stack temporary, then alloc exactly the right size. But a bit wary of using more stack).

Couple of lines up, it is already checked that address type is not NSAPI_UNSPEC if so, it returns NULL before allocating the space.

Yes, using a stack space, then copying the string by strdup() would use less heap, but do we want to optimise for this? For me, it sounds like should be used only once per application.

Ah. And we assume it's not an illegal value as a precondition I guess.

Maybe make it just else /* if (_addr.version == NSAPI_IPv6) */ { then to save an instruction or two, as the compiler's going to feel the need to test.

Although I now wonder if there should maybe be a system-level IPv4/IPv6 support switch to leave out unneeded conversion stuff. We've only got the lwIP-level switches. Something for later...

kjbracey · 2018-06-19T13:48:39Z

features/netsocket/SocketAddress.h

-    
-    /** Get the IP address
+
+    /** Get the human readable IP address


human-readable with a hyphen. Same below.

kjbracey · 2018-06-19T14:02:56Z

features/netsocket/TCPSocket.cpp

+        _pending = 0;
+        void *socket;
+        SocketAddress address;
+        *error = _stack->socket_accept(_socket, &socket, &address);


API says error can be NULL.

kjbracey · 2018-06-19T14:06:10Z

features/netsocket/UDPSocket.cpp

@@ -37,6 +32,13 @@ nsapi_protocol_t UDPSocket::get_proto()
    return NSAPI_UDP;
 }

+nsapi_error_t UDPSocket::connect(const SocketAddress &address)
+{
+    if (!address)


POSIX permits a null address to undo the connection, so I think you can just take this check out.

kjbracey · 2018-06-19T14:09:47Z

features/netsocket/UDPSocket.cpp

 nsapi_size_or_error_t UDPSocket::recvfrom(SocketAddress *address, void *buffer, nsapi_size_t size)
 {
    _lock.lock();
    nsapi_size_or_error_t ret;
+    SocketAddress ignored;
+
+    if (!address) {


Possible refinement - connect() is supposed to act as a filter on received packets, as well as be the destination for send().

Maybe we should have a if (recv >= 0 && _remote_peer && address != _remote_peer) continue in the loop?

I'm OK with that idea, but are we allowed to trash the buffers of caller, if we don't provide any data for them?
For example:
Socket is set to non-blocking mode and connected
Packet comes in
sigio() happens
Application calls recvfrom()
packet is not from the _peer_address, so continue happens
timeout
return WOULD_BLOCK

Now we are telling that we did not receive anything, but we did write the incoming packet to callers buffer, and address is recorded in the provided storage.

Also, POSIX says For SOCK_DGRAM sockets, the peer address identifies where all datagrams are sent on subsequent send() functions, and limits the remote sender for subsequent recv() functions.

So should we filter only on recv() or both recv() and recvfrom()?

we did write the incoming packet to callers buffer

Good point. Can the caller assume buffer is untouched if error is returned? It certainly would be in a real POSIX system, cos of the way system calls work, but I'm not sure it's a useful guarantee for applications, whereas the ability to use input buffer as "scratch" seems like it's potentially generally useful for drivers/stacks.

I'd be inclined to maybe add a little API note that buffer and address may be modified even if the call ends up returning an error.

Also, POSIX says

I saw that, but concluded it was poor wording - they must mean recv*. The equivalence between recv and recvfrom forces it to affect both, I believe:

The recv() function is equivalent to recvfrom() with a zero address_len argument

I guess the connect specifically makes recv() useful, in that you can assume the remote address, but the filtering must apply generally. (In a real stack the filtering would happen on entry to the socket buffer, not exit, so call chosen to read the buffer can't be a factor).

SeppoTakalo · 2018-06-20T08:54:11Z

Added incoming UDP packet filtering based on connected address.

kjbracey · 2018-06-20T09:16:12Z

features/netsocket/UDPSocket.cpp

@@ -122,6 +122,13 @@ nsapi_size_or_error_t UDPSocket::recvfrom(SocketAddress *address, void *buffer,

        _pending = 0;
        nsapi_size_or_error_t recv = _stack->socket_recvfrom(_socket, address, buffer, size);
+
+        // Filter incomming packets using connected peer address
+        if (recv > 0 && _remote_peer && _remote_peer == *address) {


>= 0. Can have zero-size datagrams.

* Move IP Socket stuff to InternetSocket class which is inherited by TCP/UDP * Implement sendto() and recvfrom() on TCP socket * Implement connect() call on UDP * Implement send() and recv() calls on UDP socket

Deprecate TCPServer in favor of just TCPSocket::accept()

This saves RAM/stack on typical socket usage where human readable format is not exchanged so often.

SeppoTakalo · 2018-06-20T09:39:33Z

Fixed based on Kevin's comments and rebased.

SeppoTakalo · 2018-06-20T11:12:23Z

@0xc0170 Who can check the travis-ci/pr? it seems to be now working anymore:

The command "travis_retry $(! sudo apt-get update 2>&1 |grep Failed)" failed and exited with 127 during .

SeppoTakalo · 2018-06-20T12:40:54Z

Memory Impact:

mbed-os-example-socket	Static RAM	ROM	Heap	sizeof TCPSocket
current master	12352	64587	3958	112
Socket refactoring	12352	65867	3958	140
Difference :	0	1280	0	28

Build for K64F using GCC_ARM, Ethernet+LwIP

0xc0170 · 2018-06-20T12:42:12Z

@0xc0170 Who can check the travis-ci/pr? it seems to be now working anymore:

I got a fix opened as PR, should be resolved soon. I restarted it now (will be green)

kjbracey · 2018-06-25T08:33:27Z

/morph build

mbed-ci · 2018-06-25T10:22:43Z

Build : SUCCESS

Build number : 2442
Build artifacts/logs : http://mbed-os.s3-website-eu-west-1.amazonaws.com/?prefix=builds/7192/

Triggering tests

/morph test
/morph uvisor-test
/morph export-build
/morph mbed2-build

mbed-ci · 2018-06-25T11:43:05Z

Exporter Build : SUCCESS

Build number : 2073
Build artifacts/logs : http://mbed-os.s3-website-eu-west-1.amazonaws.com/?prefix=builds/exporter/7192/

mbed-ci · 2018-06-25T13:55:20Z

Test : SUCCESS

Build number : 2221
Test logs :http://mbed-os-logs.s3-website-us-west-1.amazonaws.com/?prefix=logs/7192/2221

juhoeskeli · 2018-06-26T09:29:17Z

Was open() removed intentionally from the Socket class?

SeppoTakalo · 2018-06-26T09:37:05Z

@juhoeskeli Yes.

open() is a function that opens the socket on the IP stack. The base class is no IP specific and therefore does not contain open() as there is nothing to open from.

TCPSocket and UDPSocket contains open() as they always have.

It is a bit weird that somebody have been using the baseclass pointer when the base class did not offer any way to send or receive data.

Basically everything that was on the Socket is now in InternetSocket so that is an easy refactor for the application, if they use the base class.

kjbracey · 2018-06-26T09:39:44Z

Plus if anything was using Socket they must have had a lot of manual casts to UDPSocket/TCPSocket to do transfer calls - those are no longer necessary.

juhoeskeli · 2018-06-26T09:45:56Z

This is clear, thank you! 👍

kjbracey reviewed Jun 12, 2018

View reviewed changes

0xc0170 added the needs: review label Jun 12, 2018

SeppoTakalo force-pushed the socket-refactor branch 2 times, most recently from 0ed3740 to 6507744 Compare June 18, 2018 11:10

SeppoTakalo changed the title ~~WIP: Create abstract Socket interface~~ Create abstract Socket interface Jun 18, 2018

SeppoTakalo force-pushed the socket-refactor branch from 6507744 to a6d85f5 Compare June 18, 2018 11:52

kjbracey reviewed Jun 19, 2018

View reviewed changes

kjbracey reviewed Jun 20, 2018

View reviewed changes

Seppo Takalo added 6 commits June 20, 2018 12:38

Refactor Sockets by moving functions up into common base.

cdbe43c

* Move IP Socket stuff to InternetSocket class which is inherited by TCP/UDP * Implement sendto() and recvfrom() on TCP socket * Implement connect() call on UDP * Implement send() and recv() calls on UDP socket

Move accept() to abstract socket class and implement in TCPSocket

407b2f2

Deprecate TCPServer in favor of just TCPSocket::accept()

Move string allocation to a point where get_ip_address() is called

96ef8e3

This saves RAM/stack on typical socket usage where human readable format is not exchanged so often.

Close Doxygen group at the end of file

2b60e42

Fix small issues from the review

e7cb475

Filter incoming UDP packets based on connected peer address.

1ca9ccd

SeppoTakalo force-pushed the socket-refactor branch from 3c2d6d9 to 1ca9ccd Compare June 20, 2018 09:38

mikaleppanen approved these changes Jun 20, 2018

View reviewed changes

kjbracey approved these changes Jun 25, 2018

View reviewed changes

kjbracey added the needs: CI label Jun 25, 2018

kjbracey removed the needs: review label Jun 25, 2018

cmonr added the release-version: 5.10.0-rc1 label Jun 25, 2018

cmonr merged commit 24efa4c into ARMmbed:master Jun 25, 2018

0xc0170 removed the needs: CI label Jun 25, 2018

SeppoTakalo deleted the socket-refactor branch June 26, 2018 09:37

adbridge mentioned this pull request Jun 29, 2018

Fix connected UDPSocket filtering #7336

Merged

adbridge mentioned this pull request Aug 10, 2018

netsocket: astyle fix #7692

Merged

cmonr mentioned this pull request Sep 27, 2018

TCPSocket class hierarchy in 5.9 is incorrect ARMmbed/mbed-os-5-docs#749

Closed

		@@ -79,6 +79,10 @@ nsapi_error_t TCPSocket::connect(const SocketAddress &address)

		_write_in_progress = false;

		if (ret == NSAPI_ERROR_OK) {

Create abstract Socket interface #7192

Create abstract Socket interface #7192

Uh oh!

Conversation

SeppoTakalo commented Jun 12, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Pull request type

Uh oh!

SeppoTakalo commented Jun 12, 2018

Uh oh!

SeppoTakalo commented Jun 12, 2018

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!

SeppoTakalo commented Jun 18, 2018

Uh oh!

SeppoTakalo commented Jun 19, 2018

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!

kjbracey Jun 20, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SeppoTakalo commented Jun 20, 2018

SeppoTakalo commented Jun 12, 2018 •

edited

Loading

kjbracey Jun 20, 2018 •

edited

Loading

SeppoTakalo commented Jun 20, 2018 •

edited

Loading