squash. Adding more TODOs.

server/main_server/src/agent_main.cpp

@@ -145,7 +145,7 @@ namespace
     auto reap_agent_processes(bool _shutting_down) -> void;
 } // anonymous namespace
 
-int main(int _argc, char* _argv[])
+auto main(int _argc, char* _argv[]) -> int
 {
     ProcessType = AGENT_PT; // This process identifies itself as the agent factory or an agent.
 
@@ -203,7 +203,7 @@ int main(int _argc, char* _argv[])
         // Load configuration.
         const auto config_file_path = irods::get_irods_config_directory() / "server_config.json";
         irods::server_properties::instance().init(config_file_path.c_str());
-        irods::environment_properties::instance().capture(); // TODO This MUST NOT assume /var/lib/irods.
+        irods::environment_properties::instance(); // Load the local environment file.
 
         // Initialize global pointer to ips data directory for agent cleanup.
         // This is required so that the signal handler for reaping agents remains async-signal-safe.
@@ -300,7 +300,7 @@ int main(int _argc, char* _argv[])
         // Initialize zone information for request processing.
         // initServerMain starts the listening socket and stores it in svrComm.
         // TODO initServerMain can likely be simplified.
-        RsComm svrComm{}; // TODO RsComm contains a std::string, so never memset this type!
+        RsComm svrComm{}; // RsComm contains a std::string, so never use memset() on this type!
         if (const auto ec = initServerMain(svrComm, false, false); ec < 0) {
             log_af::error("{}: initServerMain error. [error code={}]", __func__, ec);
             return 1;
@@ -354,6 +354,7 @@ int main(int _argc, char* _argv[])
             struct timeval time_out; // NOLINT(cppcoreguidelines-pro-type-member-init)
             time_out.tv_sec  = 0;
             time_out.tv_usec = 500 * 1000;
+            const auto original_time_out = time_out;
 
             while ((numSock = select(svrComm.sock + 1, &sockMask, nullptr, nullptr, &time_out)) == -1) {
                 if (g_terminate) {
@@ -393,8 +394,7 @@ int main(int _argc, char* _argv[])
 
             if (0 == numSock) {
                 // "select" modifies the timeval structure, so reset it.
-                time_out.tv_sec  = 0;
-                time_out.tv_usec = 500 * 1000;
+                time_out = original_time_out;
                 continue;
             }
 
@@ -784,33 +784,35 @@ namespace
         // shutdown of the iRODS server.
         std::signal(SIGTERM, SIG_IGN);
 
-        // To avoid unnecessary complexity for configuration reload, shutdown, and signals,
-        // we use SIGUSR1 as the signal for instructing the agents to shutdown.
-        struct sigaction sa_terminate; // NOLINT(cppcoreguidelines-pro-type-member-init)
-        sigemptyset(&sa_terminate.sa_mask);
-        sa_terminate.sa_flags = SA_SIGINFO;
-        // NOLINTNEXTLINE(cppcoreguidelines-pro-type-union-access)
-        sa_terminate.sa_sigaction = [](int, siginfo_t* _siginfo, void*) {
-            // Only respond to SIGUSR1 if the agent factory triggered it.
+        {
+            // To avoid unnecessary complexity for configuration reload, shutdown, and signals,
+            // we use SIGUSR1 as the signal for instructing the agents to shutdown.
+            struct sigaction sa_terminate; // NOLINT(cppcoreguidelines-pro-type-member-init)
+            sigemptyset(&sa_terminate.sa_mask);
+            sa_terminate.sa_flags = SA_SIGINFO;
             // NOLINTNEXTLINE(cppcoreguidelines-pro-type-union-access)
-            if (getppid() == _siginfo->si_pid) {
-                g_terminate = 1;
+            sa_terminate.sa_sigaction = [](int, siginfo_t* _siginfo, void*) {
+                // Only respond to SIGUSR1 if the agent factory triggered it.
+                // NOLINTNEXTLINE(cppcoreguidelines-pro-type-union-access)
+                if (getppid() == _siginfo->si_pid) {
+                    g_terminate = 1;
+                }
+            };
+            if (sigaction(SIGUSR1, &sa_terminate, nullptr) == -1) {
+                return -1; // TODO Use a better error code or log instead.
             }
-        };
-        if (sigaction(SIGUSR1, &sa_terminate, nullptr) == -1) {
-            return -1; // TODO Use a better error code or log instead.
         }
 
         // SIGPIPE is ignored so that agents can detect socket issues to other servers.
         std::signal(SIGPIPE, SIG_IGN);
 
+        // Let the registered signal handler reap children and remove the agent info files for ips.
         std::signal(SIGCHLD, SIG_DFL);
 
+        // Do not allow admins to trigger a configuration reload directly. They must send
+        // the signal to the main server process.
         std::signal(SIGHUP, SIG_IGN);
 
-        //std::signal(SIGABRT, SIG_DFL);
-        //std::signal(SIGUSR1, SIG_DFL);
-
         irods::experimental::log::set_server_type("agent");
 
         // Artificially create a conn object in order to create a network object.

server/main_server/src/main.cpp

@@ -161,10 +161,8 @@ auto main(int _argc, char* _argv[]) -> int
             daemonize();
         }
 
-        // TODO What does it mean to daemonize and use a unique pidfile name?
-        // Perhaps daemonization means there's only one instance running on the machine?
-        // What if the pidfile name was derived from the config file path? Only one instance can work.
-        // But, what if server redirection is disabled by the admin?
+        // TODO /var/run/irods.pid fails as a default due to permissions.
+        // How do other processes handle this outside of just putting the file in another directory?
         std::string pid_file = (irods::get_irods_runstate_directory() / "irods.pid").string();
         if (const auto iter = vm.find("pid-file"); std::end(vm) != iter) {
             pid_file = std::move(iter->second.as<std::string>());
@@ -181,14 +179,13 @@ auto main(int _argc, char* _argv[]) -> int
     }
 
     try {
-        // Load configuration.
         if (!validate_configuration()) {
             return 1;
         }
 
         const auto config_file_path = irods::get_irods_config_directory() / "server_config.json";
         irods::server_properties::instance().init(config_file_path.c_str());
-        irods::environment_properties::instance().capture();
+        irods::environment_properties::instance(); // Load the local environment file.
 
         // TODO Consider removing the need for these along with all options.
         // All logging should be controlled via the new logging system.
@@ -207,12 +204,10 @@ auto main(int _argc, char* _argv[]) -> int
         log_server::info("{}: Initializing shared memory for main server process.", __func__);
 
         namespace hnc = irods::experimental::net::hostname_cache;
-        hnc::init("irods_hostname_cache5", irods::get_hostname_cache_shared_memory_size());
-        //irods::at_scope_exit deinit_hostname_cache{[] { hnc::deinit(); }};
+        hnc::init("irods_hostname_cache5", irods::get_hostname_cache_shared_memory_size()); // TODO Rename
 
         namespace dnsc = irods::experimental::net::dns_cache;
-        dnsc::init("irods_dns_cache5", irods::get_dns_cache_shared_memory_size());
-        //irods::at_scope_exit deinit_dns_cache{[] { dnsc::deinit(); }};
+        dnsc::init("irods_dns_cache5", irods::get_dns_cache_shared_memory_size()); // TODO Rename
 
         // Load server API table so that API plugins which are needed to stand up the server are
         // available for use.
@@ -247,6 +242,9 @@ auto main(int _argc, char* _argv[]) -> int
         // dsm = Short for delay server migration
         // This is used to control the frequency of the delay server migration logic.
         auto dsm_time_start = std::chrono::steady_clock::now();
+
+        // TODO Remove this and just delay the startup of the delay server. Even better if the delay server
+        // only starts up after the agent factory is accepting connections.
         auto first_boot = true;
 
         while (true) {
@@ -274,6 +272,8 @@ auto main(int _argc, char* _argv[]) -> int
                 waitpid(-1, nullptr, WNOHANG);
             }
 
+            // TODO Add logic to fork a new agent factory if not running.
+
             log_stacktrace_files();
             remove_leftover_agent_info_files_for_ips();
             migrate_and_launch_delay_server(first_boot, dsm_time_start);
@@ -295,6 +295,7 @@ namespace
 {
     auto print_usage() -> void
     {
+        // TODO Update help text.
         fmt::print(
 R"__(irodsServer - Launch an iRODS server
 
@@ -401,7 +402,7 @@ Mandatory arguments to long options are mandatory for short options too.
         }
 
         umask(0);
-        chdir("/");
+        chdir("/"); // TODO Should we keep this? Need to refresh my memory on why this is important.
 
         // Get max number of open file descriptors.
         auto max_fd = sysconf(_SC_OPEN_MAX);