From 423d9ca62898ee735cec55ec346e518affc49b13 Mon Sep 17 00:00:00 2001
From: Eric Cano <Eric.Cano@cern.ch>
Date: Wed, 24 Feb 2016 18:45:45 +0100
Subject: [PATCH] Removed the passing of the command line parameters to sub
 objects as the are parsed early on. Simplified the handling of the
 configuration file.

---
 common/threading/Daemon.cpp                   | 68 +------------------
 common/threading/Daemon.hpp                   | 26 +------
 common/threading/DaemonTest.cpp               | 35 +---------
 .../tape/tapeserver/daemon/TapeDaemon.cpp     | 11 +--
 tapeserver/cta-taped.cpp                      | 58 ++++++++--------
 tapeserver/cta-tapedSystemtests.cpp           |  7 ++
 tapeserver/daemon/CommandLineParams.cpp       | 16 ++++-
 tapeserver/daemon/CommandLineParams.hpp       |  3 +
 tapeserver/daemon/GlobalConfiguration.cpp     |  5 +-
 tapeserver/daemon/GlobalConfiguration.hpp     |  1 -
 tapeserver/daemon/TapeDaemon.cpp              | 23 ++-----
 tapeserver/daemon/TapeDaemon.hpp              | 27 ++------
 version.hpp.in                                |  2 +-
 13 files changed, 71 insertions(+), 211 deletions(-)

diff --git a/common/threading/Daemon.cpp b/common/threading/Daemon.cpp
index 358746784b..2d56c740e0 100644
--- a/common/threading/Daemon.cpp
+++ b/common/threading/Daemon.cpp
@@ -32,10 +32,7 @@
 //------------------------------------------------------------------------------
 // constructor
 //------------------------------------------------------------------------------
-cta::server::Daemon::Daemon(std::ostream &stdOut, std::ostream &stdErr,
-  log::Logger &log) throw():
-  m_stdOut(stdOut),
-  m_stdErr(stdErr),
+cta::server::Daemon::Daemon(log::Logger &log) throw():
   m_log(log),
   m_foreground(false),
   m_commandLineHasBeenParsed(false) {
@@ -47,69 +44,6 @@ cta::server::Daemon::Daemon(std::ostream &stdOut, std::ostream &stdErr,
 cta::server::Daemon::~Daemon() {
 }
 
-//------------------------------------------------------------------------------
-// parseCommandLine
-//------------------------------------------------------------------------------
-void cta::server::Daemon::parseCommandLine(int argc,
-  char *argv[])  {
-  struct ::option longopts[4];
-
-  longopts[0].name = "foreground";
-  longopts[0].has_arg = no_argument;
-  longopts[0].flag = NULL;
-  longopts[0].val = 'f';
-
-  longopts[1].name = "config";
-  longopts[1].has_arg = required_argument;
-  longopts[1].flag = NULL;
-  longopts[1].val = 'c';
-
-  longopts[2].name = "help";
-  longopts[2].has_arg = no_argument;
-  longopts[2].flag = NULL;
-  longopts[2].val = 'h';
-
-  longopts[3].name = 0;
-
-  char c;
-  while ((c = getopt_long(argc, argv, "fc:h", longopts, NULL)) != -1) {
-    switch (c) {
-    case 'f':
-      m_foreground = true;
-      break;
-    case 'c':
-      setenv("PATH_CONFIG", optarg, 1);
-      m_stdOut << "Using configuration file " << optarg << std::endl;
-      break;
-    case 'h':
-      help(argv[0]);
-      exit(0);
-      break;
-    default:
-      break;
-    }
-  }
-
-  m_commandLineHasBeenParsed = true;
-}
-
-//------------------------------------------------------------------------------
-// help
-//------------------------------------------------------------------------------
-void cta::server::Daemon::help(const std::string &programName)
-  throw() {
-  m_stdOut << "Usage: " << programName << " [options]\n"
-    "\n"
-    "where options can be:\n"
-    "\n"
-    "\t--foreground            or -f         \tRemain in the Foreground\n"
-    "\t--config <config-file>  or -c         \tConfiguration file\n"
-    "\t--metrics               or -m         \tEnable metrics collection\n"
-    "\t--help                  or -h         \tPrint this help and exit\n"
-    "\n"
-    "Comments to: Castor.Support@cern.ch\n";
-}
-
 //------------------------------------------------------------------------------
 // getServerName
 //------------------------------------------------------------------------------
diff --git a/common/threading/Daemon.hpp b/common/threading/Daemon.hpp
index 542dca8a3b..c2af2c05bd 100644
--- a/common/threading/Daemon.hpp
+++ b/common/threading/Daemon.hpp
@@ -46,7 +46,7 @@ public:
    * @param stdErr Stream representing standard error.
    * @param log Object representing the API of the CASTOR logging system.
    */
-  Daemon(std::ostream &stdOut, std::ostream &stdErr, log::Logger &log)
+  Daemon(log::Logger &log)
     throw();
 
   /**
@@ -54,20 +54,6 @@ public:
    */
   virtual ~Daemon();
 
-  /**
-   * Parses a command line to set the server options.
-   *
-   * @param argc The size of the command-line vector.
-   * @param argv The command-line vector.
-   */
-  virtual void parseCommandLine(int argc, char *argv[])
-    ;
-
-  /**
-   * Prints out the online help
-   */
-  virtual void help(const std::string &programName) throw();
-
   /**
    * Returns this server's name as used by the CASTOR logging system.
    */
@@ -105,16 +91,6 @@ protected:
    */
   void daemonizeIfNotRunInForeground(const bool runAsStagerSuperuser);
 
-  /**
-   * Stream representing standard out.
-   */
-  std::ostream &m_stdOut;
-
-  /**
-   * Stream representing standard in.
-   */
-  std::ostream &m_stdErr;
-
   /**
    * Object representing the API of the CASTOR logging system.
    */
diff --git a/common/threading/DaemonTest.cpp b/common/threading/DaemonTest.cpp
index c3d2b69370..680a32231a 100644
--- a/common/threading/DaemonTest.cpp
+++ b/common/threading/DaemonTest.cpp
@@ -59,43 +59,10 @@ protected:
 };
 
 TEST_F(cta_threading_DaemonTest, getForegroundBeforeParseCommandLine) {
-  std::ostringstream dummyStdOut;
-  std::ostringstream dummyStdErr;
   cta::log::DummyLogger log(m_programName);
-  cta::server::Daemon daemon(dummyStdOut, dummyStdErr, log);
+  cta::server::Daemon daemon(log);
   
   ASSERT_THROW(daemon.getForeground(), cta::server::Daemon::CommandLineNotParsed);
 }
 
-TEST_F(cta_threading_DaemonTest, parseEmptyCmdLine) {
-  m_argv = new char *[2];
-  m_argv[0] = strdup(m_programName.c_str());
-  m_argv[1] = NULL;
-  m_argc = 1;
-
-  std::ostringstream dummyStdOut;
-  std::ostringstream dummyStdErr;
-  cta::log::DummyLogger log(m_programName);
-  cta::server::Daemon daemon(dummyStdOut, dummyStdErr, log);
-
-  ASSERT_NO_THROW(daemon.parseCommandLine(m_argc, m_argv));
-  ASSERT_FALSE(daemon.getForeground());
-}
-
-TEST_F(cta_threading_DaemonTest, parseFOnCmdLine) {
-  m_argv = new char *[3];
-  m_argv[0] = strdup(m_programName.c_str());
-  m_argv[1] = strdup("-f");
-  m_argv[2] = NULL;
-  m_argc = 2;
-
-  std::ostringstream dummyStdOut;
-  std::ostringstream dummyStdErr;
-  cta::log::DummyLogger log(m_programName);
-  cta::server::Daemon daemon(dummyStdOut, dummyStdErr, log);
-
-  ASSERT_NO_THROW(daemon.parseCommandLine(m_argc, m_argv));
-  ASSERT_EQ(true, daemon.getForeground());
-}
-
 } // namespace unitTests
diff --git a/tapeserver/castor/tape/tapeserver/daemon/TapeDaemon.cpp b/tapeserver/castor/tape/tapeserver/daemon/TapeDaemon.cpp
index c6e392c5ed..e6ec890879 100644
--- a/tapeserver/castor/tape/tapeserver/daemon/TapeDaemon.cpp
+++ b/tapeserver/castor/tape/tapeserver/daemon/TapeDaemon.cpp
@@ -152,24 +152,17 @@ void castor::tape::tapeserver::daemon::TapeDaemon::destroyZmqContext() throw() {
 //------------------------------------------------------------------------------
 int castor::tape::tapeserver::daemon::TapeDaemon::main() throw() {
   try {
-
     exceptionThrowingMain(m_argc, m_argv);
-
   } catch (castor::exception::Exception &ex) {
-    // Write the error to standard error
-    m_stdErr << std::endl << "Aborting: " << ex.getMessage().str() << std::endl
-      << std::endl;
-
     // Log the error
     std::list<log::Param> params = {
       log::Param("Message", ex.getMessage().str()),
       log::Param("Code"   , ex.code())};
     m_log(LOG_ERR, "Aborting", params);
 
-    return 1;
+    return EXIT_FAILURE;
   }
-
-  return 0;
+  return EXIT_SUCCESS;
 }
 
 //------------------------------------------------------------------------------
diff --git a/tapeserver/cta-taped.cpp b/tapeserver/cta-taped.cpp
index ebfc77cf86..2beb0c88b0 100644
--- a/tapeserver/cta-taped.cpp
+++ b/tapeserver/cta-taped.cpp
@@ -45,7 +45,7 @@ namespace cta { namespace taped {
 // @param argv The command-line arguments.
 // @param log The logging system.
 //------------------------------------------------------------------------------
-static int exceptionThrowingMain(const int argc, char **const argv,
+static int exceptionThrowingMain(const cta::daemon::CommandLineParams & commandLine,
   cta::log::Logger &log);
 
 //------------------------------------------------------------------------------
@@ -66,8 +66,8 @@ std::string gHelpString =
 //------------------------------------------------------------------------------
 // Logs the start of the daemon.
 //------------------------------------------------------------------------------
-static void logStartOfDaemon(cta::log::Logger &log, const int argc,
-  const char *const *const argv);
+void logStartOfDaemon(cta::log::Logger &log,
+  const daemon::CommandLineParams& commandLine);
 
 //------------------------------------------------------------------------------
 // Creates a string that contains the specified command-line arguments
@@ -76,7 +76,7 @@ static void logStartOfDaemon(cta::log::Logger &log, const int argc,
 // @param argc The number of command-line arguments.
 // @param argv The array of command-line arguments.
 //------------------------------------------------------------------------------
-static std::string argvToString(const int argc, const char *const *const argv);
+//static std::string argvToString(const int argc, const char *const *const argv);
 
 ////------------------------------------------------------------------------------
 //// Writes the specified TPCONFIG lines to the specified logging system.
@@ -99,25 +99,23 @@ static std::string argvToString(const int argc, const char *const *const argv);
 //------------------------------------------------------------------------------
 // exceptionThrowingMain
 //------------------------------------------------------------------------------
-static int exceptionThrowingMain(const int argc, char **const argv,
+static int exceptionThrowingMain(
+  const cta::daemon::CommandLineParams & commandLine,
   cta::log::Logger &log) {
   using namespace cta::tape::daemon;
 
-  logStartOfDaemon(log, argc, argv);
+  logStartOfDaemon(log, commandLine);
 
   // Parse /etc/cta/cta.conf and /etc/cta/TPCONFIG for global parameters
   const GlobalConfiguration globalConfig =
-    GlobalConfiguration::createFromCtaConf(log);
+    GlobalConfiguration::createFromCtaConf(commandLine.configFileLocation, log);
 
   // Create the object providing utilities for working with UNIX capabilities
   cta::server::ProcessCap capUtils;
 
   // Create the main tapeserverd object
   cta::tape::daemon::TapeDaemon daemon(
-    argc,
-    argv,
-    std::cout,
-    std::cerr,
+    commandLine,
     log,
     globalConfig,
     capUtils);
@@ -129,32 +127,30 @@ static int exceptionThrowingMain(const int argc, char **const argv,
 //------------------------------------------------------------------------------
 // logStartOfDaemon
 //------------------------------------------------------------------------------
-static void logStartOfDaemon(cta::log::Logger &log, const int argc,
-  const char *const *const argv) {
+void logStartOfDaemon(cta::log::Logger &log,
+  const cta::daemon::CommandLineParams & commandLine) {
   using namespace cta;
 
-  const std::string concatenatedArgs = argvToString(argc, argv);
-  std::list<log::Param> params = {
-    log::Param("version", CTA_VERSION),
-    log::Param("argv", concatenatedArgs)};
-  log(log::INFO, "tapeserverd started", params);
+  std::list<log::Param> params = {log::Param("version", CTA_VERSION)};
+  params.splice(params.end(), commandLine.toLogParams());
+  log(log::INFO, "cta-taped started", params);
 }
 
 //------------------------------------------------------------------------------
 // argvToString
 //------------------------------------------------------------------------------
-static std::string argvToString(const int argc, const char *const *const argv) {
-  std::string str;
-
-  for(int i=0; i < argc; i++) {
-    if(i != 0) {
-      str += " ";
-    }
-
-    str += argv[i];
-  }
-  return str;
-}
+//static std::string argvToString(const int argc, const char *const *const argv) {
+//  std::string str;
+//
+//  for(int i=0; i < argc; i++) {
+//    if(i != 0) {
+//      str += " ";
+//    }
+//
+//    str += argv[i];
+//  }
+//  return str;
+//}
 
 ////------------------------------------------------------------------------------
 //// logTpconfigLines
@@ -225,7 +221,7 @@ int main(const int argc, char **const argv) {
 
   int programRc = EXIT_FAILURE; // Default return code when receiving an exception.
   try {
-    programRc = cta::taped::exceptionThrowingMain(argc, argv, log);
+    programRc = cta::taped::exceptionThrowingMain(*commandLine, log);
   } catch(exception::Exception &ex) {
     std::list<log::Param> params = {
       log::Param("message", ex.getMessage().str())};
diff --git a/tapeserver/cta-tapedSystemtests.cpp b/tapeserver/cta-tapedSystemtests.cpp
index 3422824404..6dad0973e7 100644
--- a/tapeserver/cta-tapedSystemtests.cpp
+++ b/tapeserver/cta-tapedSystemtests.cpp
@@ -33,5 +33,12 @@ TEST(cta_taped, InvocationTests) {
   ASSERT_NE(std::string::npos, spHelpLong.stdout().find("Usage: cta-taped [options]"));
   ASSERT_TRUE(spHelpLong.stderr().empty());
   ASSERT_EQ(EXIT_SUCCESS, spHelpLong.exitValue());
+  
+  // Does the tape server complain about absence of drive configuration?
+  Subprocess spNoDrive("cta-taped", std::list<std::string>({"cta-taped", "-f", "-s"}));
+  spNoDrive.wait();
+  ASSERT_NE(std::string::npos, spNoDrive.stdout().find("MSG=\"Aborting\" Message=\"No drive found in configuration\""));
+  ASSERT_TRUE(spNoDrive.stderr().empty());
+  ASSERT_EQ(EXIT_FAILURE, spNoDrive.exitValue());
 }
 }
\ No newline at end of file
diff --git a/tapeserver/daemon/CommandLineParams.cpp b/tapeserver/daemon/CommandLineParams.cpp
index 19bf884220..685e9bbc25 100644
--- a/tapeserver/daemon/CommandLineParams.cpp
+++ b/tapeserver/daemon/CommandLineParams.cpp
@@ -21,7 +21,9 @@
 #include <getopt.h>
 #include <string.h>
 
-cta::daemon::CommandLineParams::CommandLineParams(int argc, char** argv):
+namespace cta { namespace daemon {
+
+CommandLineParams::CommandLineParams(int argc, char** argv):
   foreground(false), logToStdout(false), 
   configFileLocation("/etc/cta/cta.conf"),
   helpRequested(false){
@@ -77,3 +79,15 @@ cta::daemon::CommandLineParams::CommandLineParams(int argc, char** argv):
     throw cta::exception::Exception("In CommandLineParams::CommandLineParams(): cannot log to stdout without running in the foreground");
   }
 }
+
+std::list<log::Param> CommandLineParams::toLogParams() const {
+  std::list<log::Param> ret;
+  ret.push_back(log::Param("foreground", foreground));
+  ret.push_back(log::Param("logToStdout", logToStdout));
+  ret.push_back(log::Param("configFileLocation", configFileLocation));
+  ret.push_back(log::Param("helpRequested", helpRequested));
+  return ret;
+}
+
+}} //  namespace cta::daemon
+
diff --git a/tapeserver/daemon/CommandLineParams.hpp b/tapeserver/daemon/CommandLineParams.hpp
index d69224be6e..8db291fb79 100644
--- a/tapeserver/daemon/CommandLineParams.hpp
+++ b/tapeserver/daemon/CommandLineParams.hpp
@@ -19,6 +19,8 @@
 #pragma once
 
 #include <string>
+#include <list>
+#include "common/log/Param.hpp"
 
 namespace cta { namespace daemon {
 /// A class parsing the command line and turning it into a struct.
@@ -33,5 +35,6 @@ struct CommandLineParams{
   bool logToStdout;                 ///< Log to stdout instead of syslog. Foreground is required.
   std::string configFileLocation;   ///< Location of the configuration file. Defaults to /etc/cta/cta.conf
   bool helpRequested;               ///< Help requested: will print out help and exit.
+  std::list<log::Param> toLogParams() const; ///< Convert the command line into set of parameters for logging.
 };
 }}
\ No newline at end of file
diff --git a/tapeserver/daemon/GlobalConfiguration.cpp b/tapeserver/daemon/GlobalConfiguration.cpp
index 6d14b9f24e..5048620d08 100644
--- a/tapeserver/daemon/GlobalConfiguration.cpp
+++ b/tapeserver/daemon/GlobalConfiguration.cpp
@@ -21,12 +21,11 @@
 namespace cta { namespace tape { namespace daemon {
   
 GlobalConfiguration GlobalConfiguration::createFromCtaConf(cta::log::Logger& log) {
-  return createFromCtaConf("/etc/cta/cta.conf", "/etc/cta/TPCONFIG", log);
+  return createFromCtaConf("/etc/cta/cta.conf", log);
 }
 
 GlobalConfiguration GlobalConfiguration::createFromCtaConf(
-  const std::string& generalConfigPath, 
-  const std::string& tapeConfigFile, cta::log::Logger& log) {
+  const std::string& generalConfigPath, cta::log::Logger& log) {
   GlobalConfiguration ret;
   return ret;
 }
diff --git a/tapeserver/daemon/GlobalConfiguration.hpp b/tapeserver/daemon/GlobalConfiguration.hpp
index d136c0cffc..f83503b62d 100644
--- a/tapeserver/daemon/GlobalConfiguration.hpp
+++ b/tapeserver/daemon/GlobalConfiguration.hpp
@@ -34,7 +34,6 @@ struct GlobalConfiguration {
           cta::log::Logger &log = gDummyLogger);
   static GlobalConfiguration createFromCtaConf(
           const std::string & generalConfigPath,
-          const std::string & tapeConfigFile,
           cta::log::Logger & log = gDummyLogger);
   std::map<std::string, DriveConfiguration> driveConfigs;
 private:
diff --git a/tapeserver/daemon/TapeDaemon.cpp b/tapeserver/daemon/TapeDaemon.cpp
index 9b193530b3..18925c5446 100644
--- a/tapeserver/daemon/TapeDaemon.cpp
+++ b/tapeserver/daemon/TapeDaemon.cpp
@@ -19,17 +19,16 @@
 #include "TapeDaemon.hpp"
 #include "common/exception/Errnum.hpp"
 #include "common/utils/utils.hpp"
+#include "tapeserver/daemon/CommandLineParams.hpp"
 #include <google/protobuf/service.h>
 
 namespace cta { namespace tape { namespace daemon {
 
-TapeDaemon::TapeDaemon(const int argc, char* * const argv, 
-    std::ostream& stdOut, std::ostream& stdErr, 
+TapeDaemon::TapeDaemon(const cta::daemon::CommandLineParams & commandLine, 
     log::Logger& log, 
     const GlobalConfiguration& globalConfig, 
     cta::server::ProcessCap& capUtils): 
-    cta::server::Daemon(stdOut, stdErr, log),
-    m_argc(argc), m_argv(argv),
+    cta::server::Daemon(log),
     m_globalConfiguration(globalConfig), m_capUtils(capUtils),
     m_programName("cta-taped"), m_hostName(getHostName()) { }
 
@@ -42,19 +41,12 @@ TapeDaemon::~TapeDaemon() {
 //------------------------------------------------------------------------------
 int TapeDaemon::main() {
   try {
-
-    exceptionThrowingMain(m_argc, m_argv);
-
+    exceptionThrowingMain();
   } catch (cta::exception::Exception &ex) {
-    // Write the error to standard error
-    m_stdErr << std::endl << "Aborting: " << ex.getMessage().str() << std::endl
-      << std::endl;
-
     // Log the error
     std::list<log::Param> params = {
       log::Param("Message", ex.getMessage().str())};
     m_log(log::ERR, "Aborting", params);
-
     return 1;
   }
 
@@ -74,12 +66,9 @@ std::string cta::tape::daemon::TapeDaemon::getHostName() const {
 //------------------------------------------------------------------------------
 // exceptionThrowingMain
 //------------------------------------------------------------------------------
-void  cta::tape::daemon::TapeDaemon::exceptionThrowingMain(
-  const int argc, char **const argv)  {
-  parseCommandLine(argc, argv);
-
+void  cta::tape::daemon::TapeDaemon::exceptionThrowingMain()  {
   if(m_globalConfiguration.driveConfigs.empty())
-    throw cta::exception::Exception("/etc/cta/TPCONFIG is empty");
+    throw cta::exception::Exception("No drive found in configuration");
 
   // Process must be able to change user now and should be permitted to perform
   // raw IO in the future
diff --git a/tapeserver/daemon/TapeDaemon.hpp b/tapeserver/daemon/TapeDaemon.hpp
index dea03694a3..0a9378b0d1 100644
--- a/tapeserver/daemon/TapeDaemon.hpp
+++ b/tapeserver/daemon/TapeDaemon.hpp
@@ -19,6 +19,7 @@
 #pragma once
 
 #include "common/threading/Daemon.hpp"
+#include "tapeserver/daemon/CommandLineParams.hpp"
 #include "tapeserver/daemon/GlobalConfiguration.hpp"
 #include "common/processCap/ProcessCap.hpp"
 #include <signal.h>
@@ -34,18 +35,12 @@ class TapeDaemon : public cta::server::Daemon {
 public:
 
   /** Constructor.
-   * @param argc The argc of main().
-   * @param argv The argv of main().
-   * @param stdOut Stream representing standard out.
-   * @param stdErr Stream representing standard error.
+   * @param commandLine The parameters extracted from the command line.
    * @param log The object representing the API of the CTA logging system.
    * @param globalConfig The configuration of the tape server.
    * @param capUtils Object providing utilities for working UNIX capabilities. */
   TapeDaemon(
-    const int argc,
-    char **const argv,
-    std::ostream &stdOut,
-    std::ostream &stdErr,
+    const cta::daemon::CommandLineParams & commandLine,
     log::Logger &log,
     const GlobalConfiguration &globalConfig,
     cta::server::ProcessCap &capUtils);
@@ -77,10 +72,8 @@ protected:
   /** Returns the name of the host on which the daemon is running. */
   std::string getHostName() const;
 
-  /** Exception throwing main() function.
-   * @param argc The number of command-line arguments.
-   * @param argv The array of command-line arguments. */
-  void exceptionThrowingMain(const int argc, char **const argv);
+  /** Exception throwing main() function. */
+  void exceptionThrowingMain();
 
   /** Sets the dumpable attribute of the current process to true. */
   void setDumpable();
@@ -232,16 +225,6 @@ protected:
   void logChildProcessTerminated(const pid_t pid, const int waitpidStat)
     throw();
   
-  /**
-   * The argc of main().
-   */
-  const int m_argc;
-
-  /**
-   * The argv of main().
-   */
-  char **const m_argv;
-  
   /** The tape server's configuration */
   const GlobalConfiguration& m_globalConfiguration;
 
diff --git a/version.hpp.in b/version.hpp.in
index d9e67f98f4..d33b955cd5 100644
--- a/version.hpp.in
+++ b/version.hpp.in
@@ -18,5 +18,5 @@
 
 #pragma once
 
-#define CTA_VERSION "@CTA_VERSION@-@CTA_REPLEASE@"
+#define CTA_VERSION "@CTA_VERSION@-@CTA_RELEASE@"
 
-- 
GitLab