From a35f25e68809c2df26b924ff101af32b2f5bd3b4 Mon Sep 17 00:00:00 2001 From: Sebastien Sikora Date: Sun, 5 Dec 2021 21:45:52 +0000 Subject: [PATCH] Significant streamlining. Removed the two Port sub-classes, we now just have the Port class, which takes an is_server bool argument in it's constructor. All ports create their own in-fifo, that way a server-client pair between them create both required fifos. The is_server flag determines which order each Port attempts to open it's in and out fifos. Previously when opening it's out fifo, we polled until timeout trying to open the fifo for writing, ignoring ENXIO in case the corresponding reader hadn't yet opened the file for reading. Now we explicitly catch (and ignore) ENOENT, in case the corresponding reader hasn't yet created the fifo file. This means we don't need to first create the server ports (so they can create all the required fifo files), then launch the child process, then open all the ports. Now we can launch the child process *first* and then create the ports in both parent and child process. As long as the ports in both server and client are all created within timeout_seconds of one-another, they will all link up. More comments to follow on significantly revised shutdown & destructors. --- demos/server_demo.cpp | 2 +- src/satellite_terminal.h | 11 ++-- src/satterm_agent.cpp | 20 +++++-- src/satterm_client.cpp | 28 ++-------- src/satterm_port.cpp | 97 +++++++++++++++++++++++++++++++++ src/satterm_port.h | 38 +++---------- src/satterm_port_client.cpp | 52 ------------------ src/satterm_port_server.cpp | 103 ------------------------------------ src/satterm_server.cpp | 32 +++-------- 9 files changed, 136 insertions(+), 247 deletions(-) delete mode 100644 src/satterm_port_client.cpp delete mode 100644 src/satterm_port_server.cpp diff --git a/demos/server_demo.cpp b/demos/server_demo.cpp index 80cc86d..2de76ed 100644 --- a/demos/server_demo.cpp +++ b/demos/server_demo.cpp @@ -7,7 +7,7 @@ int main (void) { - SatTerm_Server sts("test_server", "./client_demo", true, {"com_1", "com_2"}); + SatTerm_Server sts("test_server", "./client_demo", true, {"com_1", "com_2", "com_3", "com_4"}); if (sts.IsConnected()) { size_t message_count = 10; diff --git a/src/satellite_terminal.h b/src/satellite_terminal.h index c9fde14..d983a1f 100644 --- a/src/satellite_terminal.h +++ b/src/satellite_terminal.h @@ -20,7 +20,7 @@ class SatTerm_Agent { public: SatTerm_Agent() {} - virtual ~SatTerm_Agent() {} + virtual ~SatTerm_Agent(); std::string GetMessage(bool capture_end_char = false, unsigned long timeout_seconds = 0); std::string GetMessage(std::string const& port_identifier, bool capture_end_char = false, unsigned long timeout_seconds = 0); @@ -36,8 +36,8 @@ class SatTerm_Agent { void SetConnectedFlag(bool is_connected); protected: - bool OpenPorts(std::map>& ports, unsigned long timeout_seconds = 5); - + bool CreatePorts(bool is_server, std::string const& working_path, std::vector port_identifiers, + bool display_messages, char end_char, std::map>& ports); error_descriptor m_error_code = {0, ""}; bool m_display_messages = false; std::map> m_ports = {}; @@ -60,8 +60,6 @@ class SatTerm_Server : public SatTerm_Agent { private: std::string GetWorkingPath(void); - bool CreateServerPorts(std::string const& working_path, std::vector port_identifiers, - bool display_messages, char end_char, std::map>& ports); std::vector LoadTerminalEmulatorPaths(std::string const& file_path); pid_t StartClient(std::string const& path_to_terminal_emulator_paths, std::string const& path_to_client_binary, std::string const& working_path, char end_char, std::string const& stop_message, std::vector port_identifiers); @@ -75,6 +73,5 @@ class SatTerm_Client : public SatTerm_Agent { private: size_t GetArgStartIndex(std::string const& arg_start_delimiter, int argc, char* argv[]); std::vector ParseFifoPaths(size_t argv_start_index, size_t argv_count, char* argv[]); - void CreateClientPorts(std::string const& working_path, std::vector port_identifiers, - bool display_messages, char end_char, std::map>& ports); + }; diff --git a/src/satterm_agent.cpp b/src/satterm_agent.cpp index 94b75ac..342228d 100644 --- a/src/satterm_agent.cpp +++ b/src/satterm_agent.cpp @@ -19,12 +19,24 @@ #include "satellite_terminal.h" -bool SatTerm_Agent::OpenPorts(std::map>& ports, unsigned long timeout_seconds) { +SatTerm_Agent::~SatTerm_Agent() { + // NOTE - STL containers apart from std::array make no guarantees about order of element destruction. + // + // If we want to destroy all the Ports 'in order' at each end, we need to force the issue rather than relying on the automatic behaviour. + std::map>::iterator itr = m_ports.begin(); + while (itr != m_ports.end()) { + itr = m_ports.erase(itr); + } +} + +bool SatTerm_Agent::CreatePorts(bool is_server, std::string const& working_path, std::vector port_identifiers, + bool display_messages, char end_char, std::map>& ports) { bool success = true; - for (auto const& current_port : ports) { - if (!(current_port.second->Open(timeout_seconds))) { + for (auto const& port_identifier : port_identifiers) { + ports.emplace(port_identifier, std::make_unique(is_server, working_path, port_identifier, display_messages, end_char)); + if (!(ports.at(port_identifier).get()->IsOpened())) { success = false; - m_error_code = current_port.second->GetErrorCode(); + m_error_code = ports.at(port_identifier)->GetErrorCode(); break; } } diff --git a/src/satterm_client.cpp b/src/satterm_client.cpp index 4949c63..1a5bbb6 100644 --- a/src/satterm_client.cpp +++ b/src/satterm_client.cpp @@ -28,7 +28,7 @@ SatTerm_Client::SatTerm_Client(std::string const& identifier, int argc, char* ar bool success = false; if (argv_start_index != 0) { - success = true; + m_working_path = std::string(argv[argv_start_index]); m_end_char = (char)(std::stoi(std::string(argv[argv_start_index + 1]))); m_stop_message = std::string(argv[argv_start_index + 2]); @@ -42,13 +42,7 @@ SatTerm_Client::SatTerm_Client(std::string const& identifier, int argc, char* ar std::cerr << message << std::endl; } - CreateClientPorts(m_working_path, port_identifiers, m_display_messages, m_end_char, m_ports); - - unsigned long timeout_seconds = 5; - - if (success) { - success = OpenPorts(m_ports, timeout_seconds); - } + success = CreatePorts(false, m_working_path, port_identifiers, m_display_messages, m_end_char, m_ports); if (success) { if (m_display_messages) { @@ -72,18 +66,9 @@ SatTerm_Client::SatTerm_Client(std::string const& identifier, int argc, char* ar } SatTerm_Client::~SatTerm_Client() { - // If shutdown is occurring, the server will poll m_default_port.in until it gets EOF, then everything will be closed and unlinked at the - // server end. To make sure that unlink() deletes the files, we close any that are open here, saving m_default_port.out at this end for last. if (IsConnected()) { - for (const auto& port : m_ports) { - if (port.first != m_default_port_identifier) { - port.second->Close(); - } - } - m_ports.at(m_default_port_identifier)->Close(); + SendMessage(m_stop_message, m_stop_port_identifier); } - // There should not be any more to do. Pointers in m_in_ports and m_out_ports are stored as unique_ptr, so when the maps - // are destroyed now the destructors for the Ports should be called automatically. In these the fifos will be unlink()ed if m_is_server is set. } size_t SatTerm_Client::GetArgStartIndex(std::string const& arg_start_delimiter, int argc, char* argv[]) { @@ -115,10 +100,3 @@ std::vector SatTerm_Client::ParseFifoPaths(size_t argv_start_index, } return paths_container; } - -void SatTerm_Client::CreateClientPorts(std::string const& working_path, std::vector port_identifiers, - bool display_messages, char end_char, std::map>& ports) { - for (auto const& port_identifier : port_identifiers) { - ports.emplace(port_identifier, std::make_unique(working_path, port_identifier, display_messages, end_char)); - } -} diff --git a/src/satterm_port.cpp b/src/satterm_port.cpp index 0f739bd..95fbf77 100644 --- a/src/satterm_port.cpp +++ b/src/satterm_port.cpp @@ -17,12 +17,103 @@ #include // time(). #include // perror(). +#include // open() and O_RDONLY, O_WRONLY, etc. #include // open() and O_RDONLY, O_WRONLY, etc. #include // write(), read(), close(), unlink(). #include // errno. +#include // SIGPIPE, SIG_IGN. + #include "satterm_port.h" +Port::Port(bool is_server, std::string const& working_path, std::string const& identifier, bool display_messages, char end_char) { + m_working_path = working_path; + m_identifier = identifier; + + m_display_messages = display_messages; + m_end_char = end_char; + if (is_server) { + m_fifos.in.identifier = identifier + "_sin"; + m_fifos.out.identifier = identifier + "_sout"; + } else { + m_fifos.in.identifier = identifier + "_sout"; + m_fifos.out.identifier = identifier + "_sin"; + } + + signal(SIGPIPE, SIG_IGN); + + m_fifos.in.created = CreateFifo(m_working_path + m_fifos.in.identifier); + + OpenFifos(is_server, 5); +} + +Port::~Port() { + CloseFifos(); + UnlinkInFifo(); +} + +bool Port::OpenFifos(bool is_server, unsigned long timeout_seconds) { + if (is_server) { + m_fifos.in.opened = OpenRxFifo(m_working_path + m_fifos.in.identifier, timeout_seconds); + if (m_fifos.in.opened) { + m_fifos.out.opened = OpenTxFifo(m_working_path + m_fifos.out.identifier, timeout_seconds); + } + } else { + m_fifos.out.opened = OpenTxFifo(m_working_path + m_fifos.out.identifier, timeout_seconds); + if (m_fifos.out.opened) { + m_fifos.in.opened = OpenRxFifo(m_working_path + m_fifos.in.identifier, timeout_seconds); + } + } + return (m_fifos.in.opened && m_fifos.out.opened); +} + +void Port::CloseFifos(void) { + if (m_fifos.out.opened) { + m_fifos.out.opened = false; + close(m_fifos.out.descriptor); + } + if (m_fifos.in.opened) { + while (m_fifos.in.opened) { + usleep(10); + GetMessage(false, 0); + } + close(m_fifos.in.descriptor); + } +} + +void Port::UnlinkInFifo(void) { + if (m_fifos.in.created) { + std::string fifo_path = m_working_path + m_fifos.in.identifier; + int status = unlink(fifo_path.c_str()); + if ((status < 0) && m_display_messages) { + std::string error_message = "Unable to unlink() fifo at " + fifo_path; + perror(error_message.c_str()); + } + } +} + +bool Port::CreateFifo(std::string const& fifo_path) { + m_error_code = {0, ""}; + bool success = false; + + remove(fifo_path.c_str()); // If temporary file already exists, delete it. + + int status = mkfifo(fifo_path.c_str(), S_IFIFO|0666); + + if (status < 0) { + // Info on possible errors for mkfifo() - https://pubs.opengroup.org/onlinepubs/009696799/functions/mkfifo.html + m_error_code = {errno, "mkfifo()"}; + if (m_display_messages) { + std::string error_message = "mkfifo() error trying to create fifo " + fifo_path; + perror(error_message.c_str()); + } + success = false; + } else { + success = true; + } + return success; +} + bool Port::OpenRxFifo(std::string const& fifo_path, unsigned long timeout_seconds) { m_error_code = {0, ""}; @@ -112,6 +203,12 @@ int Port::PollToOpenTxFifo(std::string const& fifo_path, unsigned long timeout_s m_error_code = {-1, "PollToOpenTxFifo()_rx_conn_timeout"}; } break; + case ENOENT: + finished = ((time(0) - start_time) > timeout_seconds); + if (finished) { + m_error_code = {-1, "PollToOpenTxFifo()_no_tx_fifo_timeout"}; + } + break; default: m_error_code = {errno, "open()_tx"}; if (m_display_messages) { diff --git a/src/satterm_port.h b/src/satterm_port.h index 03220f2..f3d3322 100644 --- a/src/satterm_port.h +++ b/src/satterm_port.h @@ -17,22 +17,23 @@ class Port { public: - Port() {} - virtual ~Port() {} - - virtual bool Open(unsigned long timeout_seconds) = 0; - virtual void Close(void) = 0; + Port(bool is_server, std::string const& working_path, std::string const& identifier, bool display_messages, char end_char); + ~Port(); + bool IsOpened(void); std::string GetMessage(bool capture_end_char, unsigned long timeout_seconds); std::string SendMessage(std::string const& message, unsigned long timeout_seconds); size_t SendBytes(const char* bytes, size_t byte_count, unsigned long timeout_seconds); error_descriptor GetErrorCode(void); - bool IsOpened(void); protected: + bool CreateFifo(std::string const& fifo_path); + bool OpenFifos(bool is_server, unsigned long timeout_seconds); bool OpenRxFifo(std::string const& fifo_path, unsigned long timeout_seconds); bool OpenTxFifo(std::string const& fifo_path, unsigned long timeout_seconds); int PollToOpenTxFifo(std::string const& fifo_path, unsigned long timeout_seconds); + void CloseFifos(void); + void UnlinkInFifo(void); error_descriptor m_error_code = {0, ""}; bool m_display_messages = false; @@ -43,28 +44,3 @@ class Port { std::string m_current_message = ""; }; - -class Port_Server : public Port { - public: - Port_Server(std::string const& working_path, std::string const& identifier, bool display_messages, char end_char); - ~Port_Server(); - - bool Open(unsigned long timeout_seconds) override; - void Close(void) override; - - bool IsCreated(void); - - private: - bool CreateFifo(std::string const& fifo_path); - -}; - -class Port_Client : public Port { - public: - Port_Client(std::string const& working_path, std::string const& identifier, bool display_messages, char end_char); - ~Port_Client(); - - bool Open(unsigned long timeout_seconds) override; - void Close(void) override; - -}; diff --git a/src/satterm_port_client.cpp b/src/satterm_port_client.cpp deleted file mode 100644 index eef2b1d..0000000 --- a/src/satterm_port_client.cpp +++ /dev/null @@ -1,52 +0,0 @@ -// ----------------------------------------------------------------------------------------------------- -// satellite_terminal - Easily spawn and communicate bidirectionally with client processes in separate -// terminal emulator instances. -// ----------------------------------------------------------------------------------------------------- -// seb.nf.sikora@protonmail.com -// -// Copyright © 2021 Dr Seb N.F. Sikora. -// -// This work is licensed under the terms of the MIT license. -// For a copy, see . -// ----------------------------------------------------------------------------------------------------- - -#include // SIGPIPE, SIG_IGN. -#include // close(), unlink(). - -#include "satterm_port.h" - -Port_Client::Port_Client(std::string const& working_path, std::string const& identifier, bool display_messages, char end_char) { - m_working_path = working_path; - m_identifier = identifier; - m_fifos.in.identifier = identifier + "_sout"; - m_fifos.out.identifier = identifier + "_sin"; - m_display_messages = display_messages; - m_end_char = end_char; - - signal(SIGPIPE, SIG_IGN); - -} - -Port_Client::~Port_Client() { - Close(); -} - -bool Port_Client::Open(unsigned long timeout_seconds) { - m_fifos.out.opened = OpenTxFifo(m_working_path + m_fifos.out.identifier, timeout_seconds); - if (m_fifos.out.opened) { - m_fifos.in.opened = OpenRxFifo(m_working_path + m_fifos.in.identifier, timeout_seconds); - } - return (m_fifos.out.opened && m_fifos.in.opened); -} - -void Port_Client::Close(void) { - if (m_fifos.in.opened) { - close(m_fifos.in.descriptor); - m_fifos.in.opened = false; - } - if (m_fifos.out.opened) { - close(m_fifos.out.descriptor); - m_fifos.out.opened = false; - } -} - diff --git a/src/satterm_port_server.cpp b/src/satterm_port_server.cpp deleted file mode 100644 index d5dafa4..0000000 --- a/src/satterm_port_server.cpp +++ /dev/null @@ -1,103 +0,0 @@ -// ----------------------------------------------------------------------------------------------------- -// satellite_terminal - Easily spawn and communicate bidirectionally with client processes in separate -// terminal emulator instances. -// ----------------------------------------------------------------------------------------------------- -// seb.nf.sikora@protonmail.com -// -// Copyright © 2021 Dr Seb N.F. Sikora. -// -// This work is licensed under the terms of the MIT license. -// For a copy, see . -// ----------------------------------------------------------------------------------------------------- - -#include // std::cout, std::cerr, std::endl; -#include // std::string, std::to_string. - -#include // SIGPIPE, SIG_IGN. -#include // perror(). -#include // mkfifo(). -#include // close(), unlink(). - -#include "satterm_port.h" - -Port_Server::Port_Server(std::string const& working_path, std::string const& identifier, bool display_messages, char end_char) { - m_working_path = working_path; - m_identifier = identifier; - m_fifos.in.identifier = identifier + "_sin"; - m_fifos.out.identifier = identifier + "_sout"; - m_display_messages = display_messages; - m_end_char = end_char; - - signal(SIGPIPE, SIG_IGN); - - m_fifos.in.created = CreateFifo(m_working_path + m_fifos.in.identifier); - if (m_fifos.in.created) { - m_fifos.out.created = CreateFifo(m_working_path + m_fifos.out.identifier); - } -} - -Port_Server::~Port_Server() { - Close(); - if (m_fifos.in.created) { - std::string fifo_path = m_working_path + m_fifos.in.identifier; - int status = unlink(fifo_path.c_str()); - if ((status < 0) && m_display_messages) { - std::string error_message = "Unable to unlink() fifo at " + fifo_path; - perror(error_message.c_str()); - } - } - if (m_fifos.out.created) { - std::string fifo_path = m_working_path + m_fifos.out.identifier; - int status = unlink(fifo_path.c_str()); - if ((status < 0) && m_display_messages) { - std::string error_message = "Unable to unlink() fifo at " + fifo_path; - perror(error_message.c_str()); - } - } - -} - -bool Port_Server::CreateFifo(std::string const& fifo_path) { - m_error_code = {0, ""}; - bool success = false; - - remove(fifo_path.c_str()); // If temporary file already exists, delete it. - - int status = mkfifo(fifo_path.c_str(), S_IFIFO|0666); - - if (status < 0) { - // Info on possible errors for mkfifo() - https://pubs.opengroup.org/onlinepubs/009696799/functions/mkfifo.html - m_error_code = {errno, "mkfifo()"}; - if (m_display_messages) { - std::string error_message = "mkfifo() error trying to create fifo " + fifo_path; - perror(error_message.c_str()); - } - success = false; - } else { - success = true; - } - return success; -} - -bool Port_Server::Open(unsigned long timeout_seconds) { - m_fifos.in.opened = OpenRxFifo(m_working_path + m_fifos.in.identifier, timeout_seconds); - if (m_fifos.in.opened) { - m_fifos.out.opened = OpenTxFifo(m_working_path + m_fifos.out.identifier, timeout_seconds); - } - return (m_fifos.in.opened && m_fifos.out.opened); -} - -void Port_Server::Close(void) { - if (m_fifos.out.opened) { - close(m_fifos.out.descriptor); - m_fifos.out.opened = false; - } - if (m_fifos.in.opened) { - close(m_fifos.in.descriptor); - m_fifos.in.opened = false; - } -} - -bool Port_Server::IsCreated(void) { - return (m_fifos.in.created && m_fifos.out.created); -} diff --git a/src/satterm_server.cpp b/src/satterm_server.cpp index 24ef760..28244e2 100644 --- a/src/satterm_server.cpp +++ b/src/satterm_server.cpp @@ -47,8 +47,6 @@ SatTerm_Server::SatTerm_Server(std::string const& identifier, std::string const& bool success = true; if (m_working_path != "") { - success = CreateServerPorts(m_working_path, port_identifiers, m_display_messages, m_end_char, m_ports); - if (success) { pid_t client_pid = StartClient(path_to_terminal_emulator_paths, path_to_client_binary, m_working_path, m_end_char, m_stop_message, port_identifiers); if (client_pid < 0) { @@ -57,7 +55,7 @@ SatTerm_Server::SatTerm_Server(std::string const& identifier, std::string const& } if (success) { - success = OpenPorts(m_ports, timeout_seconds); + success = CreatePorts(true, m_working_path, port_identifiers, m_display_messages, m_end_char, m_ports); } if (success) { @@ -83,13 +81,15 @@ SatTerm_Server::~SatTerm_Server() { if (m_display_messages) { std::cerr << "Waiting for client process to terminate..." << std::endl; } - // Poll for EOF on read on m_default_port_identifier (tells us that client write end has closed). - while(IsConnected()) { - GetMessage(false, 0); + + unsigned long start_time = time(0); + while(IsConnected() && ((time(0) - start_time) < 5)) { + std::string shutdown_confirmation = GetMessage(m_stop_port_identifier, false, 0); + if (shutdown_confirmation == m_stop_message) { + break; + } } } - // There should not be any more to do. Pointers in m_in_ports and m_out_ports are stored as unique_ptr, so when the maps - // are destroyed now the destructors for the Ports should be called automatically. In these the fifos will be unlink()ed if m_is_server is set. } std::string SatTerm_Server::GetWorkingPath(void) { @@ -115,22 +115,6 @@ std::string SatTerm_Server::GetWorkingPath(void) { } } -bool SatTerm_Server::CreateServerPorts(std::string const& working_path, std::vector port_identifiers, - bool display_messages, char end_char, std::map>& ports) { - bool success = true; - for (auto const& port_identifier : port_identifiers) { - ports.emplace(port_identifier, std::make_unique(working_path, port_identifier, display_messages, end_char)); - Port* port_pointer = ports.at(port_identifier).get(); - Port_Server* port_server_pointer = static_cast(port_pointer); - if (!(port_server_pointer->IsCreated())) { - success = false; - m_error_code = ports.at(port_identifier)->GetErrorCode(); - break; - } - } - return success; -} - pid_t SatTerm_Server::StartClient(std::string const& path_to_terminal_emulator_paths, std::string const& path_to_client_binary, std::string const& working_path, char end_char, std::string const& stop_message, std::vector port_identifiers) { m_error_code = {0, ""};