Fixes for comms module (#19)

* Update comms

* Add KeyboardInterrupt catch

* Remove unused prototype

Author: solidpixel

lgl_host_server.py

@@ -23,8 +23,14 @@
 
 # This module implements a host server that provides services over the network
 # to a layer running on a remote device.
+#
+# Run with ...
+#    adb reverse localabstract:lglcomms tcp:63412
+#
 
 import sys
+import threading
+
 import lglpy.server
 import lglpy.service_test
 import lglpy.service_log
@@ -45,10 +51,16 @@ def main():
     print()
 
     # Start it running
-    server.run()
+    serverThread = threading.Thread(target=server.run)
+    serverThread.start()
 
-    return 0
+    # Press to exit
+    try:
+        input("Press any key to exit ...")
+    except KeyboardInterrupt:
+        server.stop()
 
+    return 0
 
 if __name__ == '__main__':
     sys.exit(main())

lglpy/server.py

@@ -91,6 +91,10 @@ def __init__(self, port: int):
         self.endpoints = {}
         self.register_endpoint(self)
 
+        self.shutdown = False
+        self.listen_sockfd = None
+        self.data_sockfd = None
+
     def get_service_name(self) -> str:
         return 'registry'
 
@@ -115,14 +119,20 @@ def run(self):
         listen_sockfd.bind(('localhost', self.port))
         listen_sockfd.listen(1)
 
+        self.listen_sockfd = listen_sockfd
+
         # Accept connections from outside
-        while True:
+        while not self.shutdown:
             print('Waiting for connection')
-            sockfd, _ = listen_sockfd.accept()
+            try:
+                sockfd, _ = listen_sockfd.accept()
+            except OSError:
+                continue
+
+            self.data_sockfd = sockfd
             print('  + Client connected')
 
-            # TODO: Add shutdown code to the loop
-            while True:
+            while not self.shutdown:
                 # Read the header
                 data = self.receive_data(sockfd, 14)
                 if not data:
@@ -150,7 +160,20 @@ def run(self):
                     if not sent:
                         break
 
+            sockfd.close()
+            self.data_sockfd = None
+
         listen_sockfd.close()
+        self.listen_sockfd = None
+
+    def stop(self):
+        self.shutdown = True
+
+        if self.listen_sockfd is not None:
+            self.listen_sockfd.close()
+
+        if self.data_sockfd is not None:
+            self.data_sockfd.shutdown(socket.SHUT_RDWR)
 
     def receive_data(self, sockfd, byte_count):
         data = b''

source_common/comms/CMakeLists.txt

@@ -33,6 +33,8 @@ target_include_directories(
     ${LIB_BINARY} PRIVATE
         ../)
 
+lgl_set_build_options(${LIB_BINARY})
+
 if(${LGL_UNITTEST})
     add_subdirectory(test)
-endif()
+endif()

source_common/comms/comms_module.cpp

@@ -28,15 +28,16 @@
  * The implementation of the main communications module.
  */
 
-#include "comms_module.hpp"
-
 #include <arpa/inet.h>
 #include <iostream>
 #include <sys/socket.h>
 #include <sys/un.h>
 #include <unistd.h>
 #include <cstring>
 
+#include "framework/utils.hpp"
+#include "comms_module.hpp"
+
 
 namespace Comms
 {
@@ -48,7 +49,7 @@ CommsModule::CommsModule(
     sockfd = socket(AF_UNIX, SOCK_STREAM, 0);
     if (sockfd < 0)
     {
-        std::cout << "  - ERROR: Client socket create failed" << std::endl;
+        LAYER_LOG("  - ERROR: Client UDS socket create failed");
         return;
     }
 
@@ -62,10 +63,10 @@ CommsModule::CommsModule(
     int conn = connect(
         sockfd,
         reinterpret_cast<const struct sockaddr*>(&servAddr),
-        sizeof(servAddr));
+        offsetof(struct sockaddr_un, sun_path) + domainAddress.size() + 1);
     if (conn != 0)
     {
-        std::cout << "  - ERROR: Client connection failed" << std::endl;
+        LAYER_LOG("  - ERROR: Client UDS connection failed");
         close(sockfd);
         sockfd = -1;
         return;
@@ -83,7 +84,7 @@ CommsModule::CommsModule(
     sockfd = socket(AF_INET, SOCK_STREAM, 0);
     if (sockfd < 0)
     {
-        std::cout << "  - ERROR: Client socket create failed" << std::endl;
+        LAYER_LOG("  - ERROR: Client TCP socket create failed");
         return;
     }
 
@@ -98,7 +99,7 @@ CommsModule::CommsModule(
         sizeof(servAddr));
     if (conn != 0)
     {
-        std::cout << "  - ERROR: Client connection failed" << std::endl;
+        LAYER_LOG("  - ERROR: Client TCP connection failed");
         close(sockfd);
         sockfd = -1;
         return;
@@ -157,7 +158,7 @@ EndpointID CommsModule::getEndpointID(
                 break;
             }
 
-            uint8_t id = (*resp)[0];
+            uint8_t svcId = (*resp)[0];
             size_t size = static_cast<size_t>((*resp)[4] << 24)
                         | static_cast<size_t>((*resp)[3] << 16)
                         | static_cast<size_t>((*resp)[2] << 8)
@@ -169,13 +170,13 @@ EndpointID CommsModule::getEndpointID(
                 break;
             }
 
-            std::string name(resp->begin() + 5, resp->begin() + 5 + size);
+            std::string svcName(resp->begin() + 5, resp->begin() + 5 + size);
 
             // Remove the entry we've read
             resp->erase(resp->begin(), resp->begin() + 5 + size);
 
             // Store the persistent registry entry
-            registry[name] = id;
+            registry[svcName] = svcId;
         }
     }
 

source_common/comms/comms_module.hpp

@@ -172,13 +172,6 @@ class CommsModule: public CommsInterface
      */
     std::shared_ptr<Message> dequeueMessage();
 
-    /**
-     * @brief Get the host service endpoint list.
-     *
-     * @return The message to send.
-     */
-    void getHostServiceEndpoints();
-
 private:
     /**
      * @brief The socket for communications.

source_common/comms/comms_receiver.cpp

@@ -28,13 +28,15 @@
  * The implementation of the communications module receiver worker.
  */
 
+#include <cinttypes>
 #include <iostream>
 #include <sys/socket.h>
 #include <unistd.h>
 #include <unordered_map>
 
 #include "comms/comms_receiver.hpp"
-#include "comms_module.hpp"
+#include "comms/comms_module.hpp"
+#include "framework/utils.hpp"
 
 namespace Comms
 {
@@ -46,7 +48,7 @@ Receiver::Receiver(
     int pipe_err = pipe(stopRequestPipe);
     if (pipe_err)
     {
-        std::cout << "  - ERROR: Client pipe create failed" << std::endl;
+        LAYER_LOG("  - ERROR: Client pipe create failed");
     }
 
     // Create and start a worker thread
@@ -127,7 +129,7 @@ void Receiver::wakeMessage(
     // Handle message not found ...
     if (parkingBuffer.count(messageID) == 0)
     {
-        std::cout << "  - ERROR: Cln: Message " << messageID << " not found" << std::endl;
+        LAYER_LOG("  - ERROR: Client message %" PRIu64 " not found", messageID);
         return;
     }
 

source_common/comms/comms_transmitter.cpp

@@ -27,12 +27,13 @@
  * @file
  * The implementation of the communications module transmitter worker.
  */
-#include "comms_transmitter.hpp"
-#include "comms_module.hpp"
-
 #include <iostream>
 #include <sys/socket.h>
 
+#include "comms/comms_transmitter.hpp"
+#include "comms/comms_module.hpp"
+#include "framework/utils.hpp"
+
 namespace Comms
 {
 

source_common/comms/test/comms_test_server.cpp

@@ -70,7 +70,7 @@ CommsTestServer::CommsTestServer(
     int bindErr = bind(
         listenSockfd,
         reinterpret_cast<const struct sockaddr*>(&servAddr),
-        sizeof(struct sockaddr_un));
+        offsetof(struct sockaddr_un, sun_path) + domainAddress.size() + 1);
     if (bindErr)
     {
         std::cout << "  - ERROR: Svr socket bind failed" << std::endl;

source_common/compiler_helper.cmake

@@ -66,10 +66,7 @@ macro(lgl_set_build_options BUILD_TARGET_NAME)
             $<${is_clang}:-Wdocumentation>
 
             # Disable warnings we don't want
-            $<${is_gnu_fe}:-Wno-unused-private-field>
-
-            # Disable features we don't want
-            $<${is_gnu_fe}:-fno-exceptions>)
+            $<${is_gnu_fe}:-Wno-unused-private-field>)
 
     target_compile_definitions(
         ${BUILD_TARGET_NAME} PRIVATE