Merge pull request #1157 from jphickey/fix-1153-avoid-fcntl-vxworks

Fix #1153, add os-specifc socket flag function

src/os/portable/os-impl-bsd-sockets.c

@@ -63,6 +63,22 @@
                                      DEFINES
 ****************************************************************************************/
 
+/*
+ * The OS layer may define a macro to set the proper flags on newly-opened sockets.
+ * If not set, then a default implementation is used, which uses fcntl() to set O_NONBLOCK
+ */
+#ifndef OS_IMPL_SOCKET_FLAGS
+#ifdef O_NONBLOCK
+#define OS_IMPL_SOCKET_FLAGS O_NONBLOCK
+#else
+#define OS_IMPL_SOCKET_FLAGS 0 /* do not set any flags */
+#endif
+#endif
+
+#ifndef OS_IMPL_SET_SOCKET_FLAGS
+#define OS_IMPL_SET_SOCKET_FLAGS(tok) OS_SetSocketDefaultFlags_Impl(tok)
+#endif
+
 typedef union
 {
     char               data[OS_SOCKADDR_MAX_LEN];
@@ -82,6 +98,37 @@ typedef union
  */
 CompileTimeAssert(sizeof(OS_SockAddr_Accessor_t) == OS_SOCKADDR_MAX_LEN, SockAddrSize);
 
+/*
+ * Default flags implementation: Set the O_NONBLOCK flag via fcntl().
+ * An implementation can also elect custom configuration by setting
+ * the OS_IMPL_SET_SOCKET_FLAGS macro to point to an alternate function.
+ */
+void OS_SetSocketDefaultFlags_Impl(const OS_object_token_t *token)
+{
+    OS_impl_file_internal_record_t *impl;
+    int                             os_flags;
+
+    impl = OS_OBJECT_TABLE_GET(OS_impl_filehandle_table, *token);
+
+    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));
+        }
+    }
+
+    impl->selectable = true;
+}
+
 /****************************************************************************************
                                     Sockets API
  ***************************************************************************************/
@@ -160,23 +207,7 @@ int32 OS_SocketOpen_Impl(const OS_object_token_t *token)
      * 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));
-        }
-    }
-
-    impl->selectable = OS_IMPL_SOCKET_SELECTABLE;
+    OS_IMPL_SET_SOCKET_FLAGS(token);
 
     return OS_SUCCESS;
 } /* end OS_SocketOpen_Impl */
@@ -397,7 +428,6 @@ int32 OS_SocketAccept_Impl(const OS_object_token_t *sock_token, const OS_object_
     int32                           return_code;
     uint32                          operation;
     socklen_t                       addrlen;
-    int                             os_flags;
     OS_impl_file_internal_record_t *sock_impl;
     OS_impl_file_internal_record_t *conn_impl;
 
@@ -432,32 +462,7 @@ int32 OS_SocketAccept_Impl(const OS_object_token_t *sock_token, const OS_object_
             {
                 Addr->ActualLength = addrlen;
 
-                /*
-                 * 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)
-                {
-                    /* 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(conn_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));
-                    }
-                }
-
-                conn_impl->selectable = OS_IMPL_SOCKET_SELECTABLE;
+                OS_IMPL_SET_SOCKET_FLAGS(conn_token);
             }
         }
     }

src/os/shared/inc/os-shared-sockets.h

@@ -188,5 +188,6 @@ int32 OS_SocketAddrSetPort_Impl(OS_SockAddr_t *Addr, uint16 PortNum);
  * Not normally called outside the local unit, except during unit test
  */
 void OS_CreateSocketName(const OS_object_token_t *token, const OS_SockAddr_t *Addr, const char *parent_name);
+void OS_SetSocketDefaultFlags_Impl(const OS_object_token_t *token);
 
 #endif /* OS_SHARED_SOCKETS_H */

src/os/vxworks/CMakeLists.txt

@@ -65,6 +65,7 @@ if (OSAL_CONFIG_INCLUDE_NETWORK)
     list(APPEND VXWORKS_IMPL_SRCLIST
 	src/os-impl-network.c
         ../portable/os-impl-bsd-sockets.c   # Use BSD socket layer implementation
+        src/os-impl-sockets.c               # Additional vxworks-specific code to handle socket flags
     )
 else()
     list(APPEND VXWORKS_IMPL_SRCLIST

src/os/vxworks/inc/os-impl-sockets.h

@@ -29,6 +29,7 @@
 #define OS_IMPL_SOCKETS_H
 
 #include "os-impl-io.h"
+#include "os-shared-globaldefs.h"
 
 #include <fcntl.h>
 #include <unistd.h>
@@ -37,25 +38,19 @@
 #include <arpa/inet.h>
 #include <netinet/in.h>
 #include <hostLib.h>
+#include <ioLib.h>
 
 /*
- * Socket descriptors should be usable with the select() API
+ * Override the socket flag set routine on this platform.
+ * This is required because some versions of VxWorks do not support
+ * the standard POSIX fcntl() opcodes, and must use ioctl() instead.
  */
-#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
+#define OS_IMPL_SET_SOCKET_FLAGS(impl) OS_VxWorks_SetSocketFlags_Impl(impl)
 
 /* The "in.h" header file supplied in VxWorks 6.9 is missing the "in_port_t" typedef */
 typedef u_short in_port_t;
 
+/* VxWorks-specific helper function to configure the socket flags on a connection */
+void OS_VxWorks_SetSocketFlags_Impl(const OS_object_token_t *token);
+
 #endif /* OS_IMPL_SOCKETS_H */

src/os/vxworks/src/os-impl-sockets.c

@@ -0,0 +1,64 @@
+/*
+ *  NASA Docket No. GSC-18,370-1, and identified as "Operating System Abstraction Layer"
+ *
+ *  Copyright (c) 2019 United States Government as represented by
+ *  the Administrator of the National Aeronautics and Space Administration.
+ *  All Rights Reserved.
+ *
+ *  Licensed under the Apache License, Version 2.0 (the "License");
+ *  you may not use this file except in compliance with the License.
+ *  You may obtain a copy of the License at
+ *
+ *    http://www.apache.org/licenses/LICENSE-2.0
+ *
+ *  Unless required by applicable law or agreed to in writing, software
+ *  distributed under the License is distributed on an "AS IS" BASIS,
+ *  WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ *  See the License for the specific language governing permissions and
+ *  limitations under the License.
+ */
+
+/**
+ * \file     os-impl-sockets.c
+ * \ingroup  vxworks
+ * \author   joseph.p.hickey@nasa.gov
+ *
+ */
+
+/****************************************************************************************
+                                    INCLUDE FILES
+ ***************************************************************************************/
+
+#include "os-vxworks.h"
+#include "os-shared-idmap.h"
+#include "os-impl-io.h"
+#include "os-impl-sockets.h"
+
+/****************************************************************************************
+                                INITIALIZATION FUNCTION
+ ***************************************************************************************/
+
+/*----------------------------------------------------------------
+ *
+ * Function: OS_VxWorks_ModuleAPI_Impl_Init
+ *
+ *  Purpose: Local helper routine, not part of OSAL API.
+ *
+ *-----------------------------------------------------------------*/
+void OS_VxWorks_SetSocketFlags_Impl(const OS_object_token_t *token)
+{
+    OS_impl_file_internal_record_t *impl;
+    int                             os_flags;
+
+    impl = OS_OBJECT_TABLE_GET(OS_impl_filehandle_table, *token);
+
+    /* Use ioctl/FIONBIO on this platform, rather than standard fcntl() */
+    os_flags = 1;
+    if (ioctl(impl->fd, FIONBIO, &os_flags) == -1)
+    {
+        /* No recourse if ioctl fails - just report the error and move on. */
+        OS_DEBUG("ioctl(FIONBIO): %s\n", strerror(errno));
+    }
+
+    impl->selectable = true;
+}

src/unit-test-coverage/portable/src/coveragetest-bsd-sockets.c

@@ -100,21 +100,34 @@ void Test_OS_SocketOpen_Impl(void)
     OS_stream_table[0].socket_type   = OS_SocketType_STREAM;
     OS_stream_table[0].socket_domain = OS_SocketDomain_INET6;
     OSAPI_TEST_FUNCTION_RC(OS_SocketOpen_Impl, (&token), OS_SUCCESS);
-    UtAssert_True(UT_PortablePosixIOTest_Get_Selectable(token.obj_idx), "Socket is selectable");
+}
+
+void Test_OS_SetSocketDefaultFlags_Impl(void)
+{
+    OS_object_token_t token = {0};
 
     /* Failure in fcntl() GETFL */
     UT_PortablePosixIOTest_ResetImpl(token.obj_idx);
     UT_ResetState(UT_KEY(OCS_fcntl));
     UT_SetDeferredRetcode(UT_KEY(OCS_fcntl), 1, -1);
-    OSAPI_TEST_FUNCTION_RC(OS_SocketOpen_Impl, (&token), OS_SUCCESS);
+    UtAssert_VOIDCALL(OS_SetSocketDefaultFlags_Impl(&token));
     UtAssert_STUB_COUNT(OCS_fcntl, 1);
+    UtAssert_True(UT_PortablePosixIOTest_Get_Selectable(token.obj_idx), "Socket is selectable");
 
     /* Failure in fcntl() SETFL */
     UT_PortablePosixIOTest_ResetImpl(token.obj_idx);
     UT_ResetState(UT_KEY(OCS_fcntl));
     UT_SetDeferredRetcode(UT_KEY(OCS_fcntl), 2, -1);
-    OSAPI_TEST_FUNCTION_RC(OS_SocketOpen_Impl, (&token), OS_SUCCESS);
+    UtAssert_VOIDCALL(OS_SetSocketDefaultFlags_Impl(&token));
+    UtAssert_STUB_COUNT(OCS_fcntl, 2);
+    UtAssert_True(UT_PortablePosixIOTest_Get_Selectable(token.obj_idx), "Socket is selectable");
+
+    /* Nominal path */
+    UT_PortablePosixIOTest_ResetImpl(token.obj_idx);
+    UT_ResetState(UT_KEY(OCS_fcntl));
+    UtAssert_VOIDCALL(OS_SetSocketDefaultFlags_Impl(&token));
     UtAssert_STUB_COUNT(OCS_fcntl, 2);
+    UtAssert_True(UT_PortablePosixIOTest_Get_Selectable(token.obj_idx), "Socket is selectable");
 }
 
 void Test_OS_SocketBind_Impl(void)
@@ -255,20 +268,6 @@ void Test_OS_SocketAccept_Impl(void)
 
     /* Success case */
     OSAPI_TEST_FUNCTION_RC(OS_SocketAccept_Impl, (&sock_token, &conn_token, &addr, 0), OS_SUCCESS);
-
-    /* Failure in fcntl() GETFL */
-    UT_PortablePosixIOTest_ResetImpl(conn_token.obj_idx);
-    UT_ResetState(UT_KEY(OCS_fcntl));
-    UT_SetDeferredRetcode(UT_KEY(OCS_fcntl), 1, -1);
-    OSAPI_TEST_FUNCTION_RC(OS_SocketAccept_Impl, (&sock_token, &conn_token, &addr, 0), OS_SUCCESS);
-    UtAssert_STUB_COUNT(OCS_fcntl, 1);
-
-    /* Failure in fcntl() SETFL */
-    UT_PortablePosixIOTest_ResetImpl(conn_token.obj_idx);
-    UT_ResetState(UT_KEY(OCS_fcntl));
-    UT_SetDeferredRetcode(UT_KEY(OCS_fcntl), 2, -1);
-    OSAPI_TEST_FUNCTION_RC(OS_SocketAccept_Impl, (&sock_token, &conn_token, &addr, 0), OS_SUCCESS);
-    UtAssert_STUB_COUNT(OCS_fcntl, 2);
 }
 
 void Test_OS_SocketRecvFrom_Impl(void)
@@ -484,6 +483,7 @@ void Osapi_Test_Teardown(void) {}
 void UtTest_Setup(void)
 {
     ADD_TEST(OS_SocketOpen_Impl);
+    ADD_TEST(OS_SetSocketDefaultFlags_Impl);
     ADD_TEST(OS_SocketBind_Impl);
     ADD_TEST(OS_SocketConnect_Impl);
     ADD_TEST(OS_SocketShutdown_Impl);

src/unit-test-coverage/ut-stubs/inc/OCS_ioLib.h

@@ -37,6 +37,7 @@
 
 #define OCS_FIOCHKDSK  0x1E01
 #define OCS_FIOUNMOUNT 0x1E02
+#define OCS_FIONBIO    0x1E03
 
 /* ----------------------------------------- */
 /* types normally defined in ioLib.h */

src/unit-test-coverage/ut-stubs/override_inc/ioLib.h

@@ -37,6 +37,7 @@
 
 #define FIOCHKDSK  OCS_FIOCHKDSK
 #define FIOUNMOUNT OCS_FIOUNMOUNT
+#define FIONBIO    OCS_FIONBIO
 #define ioctl      OCS_ioctl
 
 #endif /* OVERRIDE_IOLIB_H */

src/unit-test-coverage/ut-stubs/src/os-shared-sockets-impl-stubs.c

@@ -27,6 +27,18 @@
 #include "os-shared-sockets.h"
 #include "utgenstub.h"
 
+/*
+ * ----------------------------------------------------
+ * Generated stub function for OS_SetSocketDefaultFlags_Impl()
+ * ----------------------------------------------------
+ */
+void OS_SetSocketDefaultFlags_Impl(const OS_object_token_t *token)
+{
+    UT_GenStub_AddParam(OS_SetSocketDefaultFlags_Impl, const OS_object_token_t *, token);
+
+    UT_GenStub_Execute(OS_SetSocketDefaultFlags_Impl, Basic, NULL);
+}
+
 /*
  * ----------------------------------------------------
  * Generated stub function for OS_SocketAccept_Impl()

src/unit-test-coverage/vxworks/CMakeLists.txt

@@ -15,6 +15,7 @@ set(VXWORKS_MODULE_LIST
     network
     queues
     shell
+    sockets
     symtab
     tasks
     timebase

src/unit-test-coverage/vxworks/adaptors/CMakeLists.txt

@@ -21,6 +21,7 @@ add_library(ut-adaptor-${SETNAME} STATIC
     src/ut-adaptor-mutex.c
     src/ut-adaptor-queues.c
     src/ut-adaptor-filetable-stub.c
+    src/ut-adaptor-sockets.c
     src/ut-adaptor-symtab.c
     src/ut-adaptor-tasks.c
     src/ut-adaptor-timebase.c