Skip to content

Commit

Permalink
Fix #988, do not require nonblock mode
Browse files Browse the repository at this point in the history 
In some versions of VxWorks the fcntl F_GETFL/F_SETFL opcodes
do not appear to be implemented, and thus it is not possible
to set O_NONBLOCK mode.  However, this mode is not necessarily
required, it is more of a backup/failsafe.

The "selectable" flag should not be dependent on whether
O_NONBLOCK flag got set.

This also adjust some timeouts and adds some delays to improve
the reliability of network-api-test on VxWorks.  The timeouts
were only 10ms, and this is much too short as messages are
getting written on a 9600 baud console (avg 1ms/char).  A
single log message can easily take 50-60ms alone.
  • Loading branch information
@jphickey
jphickey committed May 7, 2021
1 parent 706f0de commit 00eab63
Show file tree
Hide file tree
Showing 5 changed files with 79 additions and 36 deletions.
21 changes: 12 additions & 9 deletions src/os/portable/os-impl-bsd-sockets.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -155,27 +155,28 @@ int32 OS_SocketOpen_Impl(const OS_object_token_t *token)
* Set the standard options on the filehandle by default --
* this may set it to non-blocking mode if the implementation supports it.
* any blocking would be done explicitly via the select() wrappers
*
* NOTE: The implementation still generally works without this flag set, but
* nonblock mode does improve robustness in the event that multiple tasks
* attempt to accept new connections from the same server socket at the same time.
*/
os_flags = fcntl(impl->fd, F_GETFL);
if (os_flags == -1)
{
/* No recourse if F_GETFL fails - just report the error and move on. */
OS_DEBUG("fcntl(F_GETFL): %s\n", strerror(errno));
}
else
{
os_flags |= OS_IMPL_SOCKET_FLAGS;
if (fcntl(impl->fd, F_SETFL, os_flags) == -1)
{
/* No recourse if F_SETFL fails - just report the error and move on. */
OS_DEBUG("fcntl(F_SETFL): %s\n", strerror(errno));
}
else
{
impl->selectable = ((os_flags & O_NONBLOCK) != 0);
}
}

impl->selectable = OS_IMPL_SOCKET_SELECTABLE;

return OS_SUCCESS;
} /* end OS_SocketOpen_Impl */

Expand Down Expand Up @@ -434,6 +435,10 @@ int32 OS_SocketAccept_Impl(const OS_object_token_t *sock_token, const OS_object_
* Set the standard options on the filehandle by default --
* this may set it to non-blocking mode if the implementation supports it.
* any blocking would be done explicitly via the select() wrappers
*
* NOTE: The implementation still generally works without this flag set, but
* nonblock mode does improve robustness in the event that multiple tasks
* attempt to read from the same socket at the same time.
*/
os_flags = fcntl(conn_impl->fd, F_GETFL);
if (os_flags == -1)
Expand All @@ -449,11 +454,9 @@ int32 OS_SocketAccept_Impl(const OS_object_token_t *sock_token, const OS_object_
/* No recourse if F_SETFL fails - just report the error and move on. */
OS_DEBUG("fcntl(F_SETFL): %s\n", strerror(errno));
}
else
{
conn_impl->selectable = ((os_flags & O_NONBLOCK) != 0);
}
}

conn_impl->selectable = OS_IMPL_SOCKET_SELECTABLE;
}
}
}
Expand Down
5 changes: 5 additions & 0 deletions src/os/posix/inc/os-impl-sockets.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,11 @@

#define OS_NETWORK_SUPPORTS_IPV6

/*
* Socket descriptors should be usable with the select() API
*/
#define OS_IMPL_SOCKET_SELECTABLE true

/*
* A full POSIX-compliant I/O layer should support using
* nonblocking I/O calls in combination with select().
Expand Down
5 changes: 5 additions & 0 deletions src/os/rtems/inc/os-impl-sockets.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,11 @@
#include <arpa/inet.h>
#include <netinet/in.h>

/*
* Socket descriptors should be usable with the select() API
*/
#define OS_IMPL_SOCKET_SELECTABLE true

/*
* A RTEMS socket I/O layer should support using
* nonblocking I/O calls in combination with select().
Expand Down
12 changes: 12 additions & 0 deletions src/os/vxworks/inc/os-impl-sockets.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -38,8 +38,20 @@
#include <netinet/in.h>
#include <hostLib.h>

/*
* Socket descriptors should be usable with the select() API
*/
#define OS_IMPL_SOCKET_SELECTABLE true

/*
* Use the O_NONBLOCK flag on sockets
*
* NOTE: the fcntl() F_GETFL/F_SETFL opcodes that set descriptor flags may not
* work correctly on some version of VxWorks.
*
* This flag is not strictly required, things still mostly work without it,
* but lack of this mode does introduce some potential race conditions if more
* than one task attempts to use the same descriptor handle at the same time.
*/
#define OS_IMPL_SOCKET_FLAGS O_NONBLOCK

Expand Down
72 changes: 45 additions & 27 deletions src/tests/network-api-test/network-api-test.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,16 @@

#define UT_EXIT_LOOP_MAX 100

/*
* Timeouts for various socket ops in the test cases
*
* Note that the act of calling any "assert" routine causes console output, which
* can easily take tens or even hundreds of milliseconds to execute on platforms
* where the console is on a 9600 baud serial port. Therefore this timeout must
* not be too short.
*/
#define UT_TIMEOUT 1000

/*
* Variations of client->server connections to create.
* This tests that the server socket can accept multiple connections,
Expand Down Expand Up @@ -248,7 +258,7 @@ void TestDatagramNetworkApi(void)

/* Recieve data from peer1 to peer2 */
expected = sizeof(Buf2);
actual = OS_SocketRecvFrom(p2_socket_id, &Buf2, sizeof(Buf2), &l_addr, 100);
actual = OS_SocketRecvFrom(p2_socket_id, &Buf2, sizeof(Buf2), &l_addr, UT_TIMEOUT);
UtAssert_True(actual == expected, "OS_SocketRecvFrom() Passed. sizeof(Buf2) (%ld) == 1", (long)actual);
UtAssert_True(Buf1 == Buf2, "Buf1 (%ld) == Buf2 (%ld)", (long)Buf1, (long)Buf2);

Expand All @@ -275,7 +285,7 @@ void TestDatagramNetworkApi(void)

/* Recieve data from peer2 to peer1 */
expected = sizeof(Buf4);
actual = OS_SocketRecvFrom(p1_socket_id, &Buf4, sizeof(Buf4), &l_addr, 100);
actual = OS_SocketRecvFrom(p1_socket_id, &Buf4, sizeof(Buf4), &l_addr, UT_TIMEOUT);
UtAssert_True(actual == expected, "OS_SocketRecvFrom() Passed. sizeof(Buf3) (%ld) == 1", (long)actual);
UtAssert_True(Buf3 == Buf4, "Buf3 (%ld) == Buf4 (%ld)", (long)Buf3, (long)Buf4);

Expand Down Expand Up @@ -328,7 +338,7 @@ void TestDatagramNetworkApi(void)

/* OS_SocketRecvFrom */
expected = OS_INVALID_POINTER;
actual = OS_SocketRecvFrom(p2_socket_id, NULL, OSAL_SIZE_C(1), NULL, 100);
actual = OS_SocketRecvFrom(p2_socket_id, NULL, OSAL_SIZE_C(1), NULL, UT_TIMEOUT);
UtAssert_True(actual == expected, "OS_SocketRecvFrom() (%ld) == OS_INVALID_POINTER", (long)actual);

expected = OS_INVALID_POINTER;
Expand All @@ -337,15 +347,15 @@ void TestDatagramNetworkApi(void)

expected = OS_ERR_INVALID_ID;
objid = OS_ObjectIdFromInteger(0xFFFFFFFF);
actual = OS_SocketRecvFrom(objid, &Buf2, sizeof(Buf2), &l_addr, 100);
actual = OS_SocketRecvFrom(objid, &Buf2, sizeof(Buf2), &l_addr, UT_TIMEOUT);
UtAssert_True(actual == expected, "OS_SocketRecvFrom() (%ld) == OS_ERR_INVALID_ID", (long)actual);

expected = OS_ERR_INVALID_SIZE;
actual = OS_SocketRecvFrom(p2_socket_id, &Buf2, OSAL_SIZE_C(0), &l_addr, 100);
actual = OS_SocketRecvFrom(p2_socket_id, &Buf2, OSAL_SIZE_C(0), &l_addr, UT_TIMEOUT);
UtAssert_True(actual == expected, "OS_SocketRecvFrom() (%ld) == OS_ERR_INVALID_SIZE", (long)actual);

expected = OS_ERR_INVALID_SIZE;
actual = OS_SocketRecvFrom(p2_socket_id, &Buf2, OSAL_SIZE_C(0), NULL, 100);
actual = OS_SocketRecvFrom(p2_socket_id, &Buf2, OSAL_SIZE_C(0), NULL, UT_TIMEOUT);
UtAssert_True(actual == expected, "OS_SocketRecvFrom() (%ld) == OS_ERR_INVALID_SIZE", (long)actual);

/* OS_SocketAddrToString */
Expand Down Expand Up @@ -433,6 +443,8 @@ void Server_Fn(void)

for (iter = UT_STREAM_CONNECTION_INITIAL; iter < UT_STREAM_CONNECTION_MAX; ++iter)
{
UtPrintf("SERVER: accepting connection %u", (unsigned int)iter);

/* Accept incoming connections */
Status = OS_SocketAccept(s_socket_id, &connsock_id, &addr, OS_PEND);
if (Status != OS_SUCCESS)
Expand All @@ -442,6 +454,8 @@ void Server_Fn(void)
break;
}

UtPrintf("SERVER: handling connection %u", (unsigned int)iter);

/* Recieve incoming data from client -
* should be exactly 4 bytes on most cycles, but 0 bytes on the cycle
* where write shutdown was done by client side prior to initial write. */
Expand All @@ -453,7 +467,7 @@ void Server_Fn(void)
{
ExpectedStatus = 4;
}
Status = OS_TimedRead(connsock_id, Buf_trans, sizeof(Buf_trans), 10);
Status = OS_TimedRead(connsock_id, Buf_trans, sizeof(Buf_trans), UT_TIMEOUT);
if (Status != ExpectedStatus)
{
snprintf(ServerFn_ErrorString, sizeof(ServerFn_ErrorString), "OS_TimedRead() iter=%u, return code=%d/%d",
Expand All @@ -472,23 +486,23 @@ void Server_Fn(void)
* 2. Original value recieved above (4 bytes)
* 3. String of all possible 8-bit chars [0-255] (256 bytes)
*/
Status = OS_TimedWrite(connsock_id, &iter, sizeof(iter), 10);
Status = OS_TimedWrite(connsock_id, &iter, sizeof(iter), UT_TIMEOUT);
if (Status != sizeof(iter))
{
snprintf(ServerFn_ErrorString, sizeof(ServerFn_ErrorString),
"OS_TimedWrite(uint32) iter=%u, return code=%d", (unsigned int)iter, (int)Status);
break;
}

Status = OS_TimedWrite(connsock_id, Buf_trans, 4, 10);
Status = OS_TimedWrite(connsock_id, Buf_trans, 4, UT_TIMEOUT);
if (Status != 4)
{
snprintf(ServerFn_ErrorString, sizeof(ServerFn_ErrorString),
"OS_TimedWrite(Buf_trans) iter=%u, return code=%d", (unsigned int)iter, (int)Status);
break;
}

Status = OS_TimedWrite(connsock_id, Buf_each_char_s, sizeof(Buf_each_char_s), 10);
Status = OS_TimedWrite(connsock_id, Buf_each_char_s, sizeof(Buf_each_char_s), UT_TIMEOUT);
if (Status != sizeof(Buf_each_char_s))
{
snprintf(ServerFn_ErrorString, sizeof(ServerFn_ErrorString),
Expand Down Expand Up @@ -593,6 +607,10 @@ void TestStreamNetworkApi(void)
*/
for (iter = UT_STREAM_CONNECTION_INITIAL; iter < UT_STREAM_CONNECTION_MAX; ++iter)
{
/* An extra delay here to increases the chance that the server task has reached the "accept" call */
OS_TaskDelay(UT_TIMEOUT);
UtPrintf("CLIENT: initiating connection %u", (unsigned int)iter);

/* Open a client socket */
expected = OS_SUCCESS;
c_socket_id = OS_OBJECT_ID_UNDEFINED;
Expand All @@ -601,7 +619,7 @@ void TestStreamNetworkApi(void)
UtAssert_True(actual == expected, "OS_SocketOpen() (%ld) == OS_SUCCESS", (long)actual);
UtAssert_True(OS_ObjectIdDefined(c_socket_id), "c_socket_id (%lu) != 0", OS_ObjectIdToInteger(c_socket_id));

actual = OS_SocketConnect(c_socket_id, &s_addr, 10);
actual = OS_SocketConnect(c_socket_id, &s_addr, UT_TIMEOUT);
UtAssert_True(actual == expected, "OS_SocketConnect() (%ld) == OS_SUCCESS", (long)actual);

/*
Expand All @@ -614,11 +632,11 @@ void TestStreamNetworkApi(void)
/* OS_TimedRead */
expected = OS_ERR_INVALID_ID;
temp_id = OS_ObjectIdFromInteger(0xFFFFFFFF);
actual = OS_TimedRead(temp_id, Buf_rcv_c, sizeof(Buf_rcv_c), 10);
actual = OS_TimedRead(temp_id, Buf_rcv_c, sizeof(Buf_rcv_c), UT_TIMEOUT);
UtAssert_True(actual == expected, "OS_TimedRead() (%ld) == %ld", (long)actual, (long)expected);

expected = OS_INVALID_POINTER;
actual = OS_TimedRead(c_socket_id, NULL, sizeof(Buf_rcv_c), 10);
actual = OS_TimedRead(c_socket_id, NULL, sizeof(Buf_rcv_c), UT_TIMEOUT);
UtAssert_True(actual == expected, "OS_TimedRead() (%ld) == %ld", (long)actual, (long)expected);

expected = OS_ERROR_TIMEOUT;
Expand All @@ -628,11 +646,11 @@ void TestStreamNetworkApi(void)
/* OS_TimedWrite */
expected = OS_ERR_INVALID_ID;
temp_id = OS_ObjectIdFromInteger(0xFFFFFFFF);
actual = OS_TimedWrite(temp_id, Buf_rcv_c, sizeof(Buf_rcv_c), 10);
actual = OS_TimedWrite(temp_id, Buf_rcv_c, sizeof(Buf_rcv_c), UT_TIMEOUT);
UtAssert_True(actual == expected, "OS_TimedWrite() (%ld) == %ld", (long)actual, (long)expected);

expected = OS_INVALID_POINTER;
actual = OS_TimedWrite(c_socket_id, NULL, sizeof(Buf_rcv_c), 10);
actual = OS_TimedWrite(c_socket_id, NULL, sizeof(Buf_rcv_c), UT_TIMEOUT);
UtAssert_True(actual == expected, "OS_TimedWrite() (%ld) == %ld", (long)actual, (long)expected);

/* OS_SocketAccept */
Expand All @@ -641,16 +659,16 @@ void TestStreamNetworkApi(void)
UtAssert_True(actual == expected, "OS_SocketAccept() (%ld) == OS_INVALID_POINTER", (long)actual);

expected = OS_INVALID_POINTER;
actual = OS_SocketAccept(s_socket_id, NULL, &temp_addr, 10);
actual = OS_SocketAccept(s_socket_id, NULL, &temp_addr, UT_TIMEOUT);
UtAssert_True(actual == expected, "OS_SocketAccept() (%ld) == OS_INVALID_POINTER", (long)actual);

expected = OS_INVALID_POINTER;
actual = OS_SocketAccept(s_socket_id, &temp_id, NULL, 10);
actual = OS_SocketAccept(s_socket_id, &temp_id, NULL, UT_TIMEOUT);
UtAssert_True(actual == expected, "OS_SocketAccept() (%ld) == OS_INVALID_POINTER", (long)actual);

/* OS_SocketConnect */
expected = OS_INVALID_POINTER;
actual = OS_SocketConnect(c_socket_id, NULL, 10);
actual = OS_SocketConnect(c_socket_id, NULL, UT_TIMEOUT);
UtAssert_True(actual == expected, "OS_SocketConnect() (%ld) == OS_INVALID_POINTER", (long)actual);

expected = OS_ERR_INCORRECT_OBJ_STATE;
Expand All @@ -660,7 +678,7 @@ void TestStreamNetworkApi(void)

expected = OS_ERR_INVALID_ID;
temp_id = OS_ObjectIdFromInteger(0xFFFFFFFF);
actual = OS_SocketConnect(temp_id, &s_addr, 10);
actual = OS_SocketConnect(temp_id, &s_addr, UT_TIMEOUT);
UtAssert_True(actual == expected, "OS_SocketConnect() (%ld) == OS_ERR_INVALID_ID", (long)actual);
}

Expand Down Expand Up @@ -694,7 +712,7 @@ void TestStreamNetworkApi(void)
/* Attempt to read data, would block/timeout normally, but
* due to read shutdown it should immediately return instead. */
expected = 0;
actual = OS_TimedRead(c_socket_id, Buf_rcv_c, sizeof(Buf_rcv_c), 10);
actual = OS_TimedRead(c_socket_id, Buf_rcv_c, sizeof(Buf_rcv_c), UT_TIMEOUT);
UtAssert_True(actual == expected, "OS_TimedRead() after read shutdown (%ld) == %ld", (long)actual,
(long)expected);
}
Expand All @@ -703,7 +721,7 @@ void TestStreamNetworkApi(void)
{
/* Send data to server - this should still work after read shutdown, but not after write shutdown */
expected = sizeof(Buf_send_c);
actual = OS_TimedWrite(c_socket_id, Buf_send_c, sizeof(Buf_send_c), 10);
actual = OS_TimedWrite(c_socket_id, Buf_send_c, sizeof(Buf_send_c), UT_TIMEOUT);
UtAssert_True(actual == expected, "OS_TimedWrite() (%ld) == %ld", (long)actual, (long)expected);
}

Expand All @@ -720,7 +738,7 @@ void TestStreamNetworkApi(void)
{
/* If write shutdown worked as expected, write should return an error */
expected = OS_ERROR;
actual = OS_TimedWrite(c_socket_id, Buf_send_c, sizeof(Buf_send_c), 10);
actual = OS_TimedWrite(c_socket_id, Buf_send_c, sizeof(Buf_send_c), UT_TIMEOUT);
UtAssert_True(actual == expected, "OS_TimedWrite() after SHUT_WRITE (%ld) == %ld", (long)actual,
(long)expected);
}
Expand All @@ -731,26 +749,26 @@ void TestStreamNetworkApi(void)
{
/* Recieve back data from server, first is loop count */
expected = sizeof(loopcnt);
actual = OS_TimedRead(c_socket_id, &loopcnt, sizeof(loopcnt), 10);
actual = OS_TimedRead(c_socket_id, &loopcnt, sizeof(loopcnt), UT_TIMEOUT);
UtAssert_True(actual == expected, "OS_TimedRead() (%ld) == %ld", (long)actual, (long)expected);
UtAssert_UINT32_EQ(iter, loopcnt);

/* Recieve back data from server, next is original string */
expected = sizeof(Buf_rcv_c);
actual = OS_TimedRead(c_socket_id, Buf_rcv_c, sizeof(Buf_rcv_c), 10);
actual = OS_TimedRead(c_socket_id, Buf_rcv_c, sizeof(Buf_rcv_c), UT_TIMEOUT);
UtAssert_True(actual == expected, "OS_TimedRead() (%ld) == %ld", (long)actual, (long)expected);
UtAssert_True(strcmp(Buf_send_c, Buf_rcv_c) == 0, "Buf_rcv_c (%s) == Buf_send_c (%s)", Buf_rcv_c,
Buf_send_c);

/* Recieve back data from server, next is 8-bit charset */
expected = sizeof(Buf_each_char_rcv);
actual = OS_TimedRead(c_socket_id, Buf_each_char_rcv, sizeof(Buf_each_char_rcv), 10);
actual = OS_TimedRead(c_socket_id, Buf_each_char_rcv, sizeof(Buf_each_char_rcv), UT_TIMEOUT);
UtAssert_True(actual == expected, "OS_TimedRead() (%ld) == %ld", (long)actual, (long)expected);
UtAssert_MemCmpCount(Buf_each_char_rcv, sizeof(Buf_each_char_rcv), "Verify byte count pattern");

/* Server should close the socket, reads will return 0 indicating EOF */
expected = 0;
actual = OS_TimedRead(c_socket_id, Buf_rcv_c, sizeof(Buf_rcv_c), 10);
actual = OS_TimedRead(c_socket_id, Buf_rcv_c, sizeof(Buf_rcv_c), UT_TIMEOUT);
UtAssert_True(actual == expected, "OS_TimedRead() (%ld) == %ld", (long)actual, (long)expected);
}

Expand All @@ -767,7 +785,7 @@ void TestStreamNetworkApi(void)
loopcnt = 0;
while ((OS_TaskGetInfo(s_task_id, &taskprop) == OS_SUCCESS) && (loopcnt < UT_EXIT_LOOP_MAX))
{
OS_TaskDelay(10);
OS_TaskDelay(UT_TIMEOUT);
loopcnt++;
}
UtAssert_True(loopcnt < UT_EXIT_LOOP_MAX, "Task exited after %ld iterations", (long)loopcnt);
Expand Down

0 comments on commit 00eab63

Please sign in to comment.