BUG#11331 "Warning: could not add log destination" message does not provide enough info

Implement error reporting to Logger and associated classes.


ndb/include/logger/LogHandler.hpp:
  Add error string to LogHandler along with error code
ndb/include/logger/Logger.hpp:
  Add reporting back of error to addHandler(BaseString).
ndb/src/common/logger/FileLogHandler.cpp:
  Implement error reporting
ndb/src/common/logger/LogHandler.cpp:
  Implement error reporting.
  
  Trim spaces and tabs out of parameter names. This allows "p=v, param=value"
ndb/src/common/logger/Logger.cpp:
  Report back errors when adding a handler via BaseString
ndb/src/common/logger/SysLogHandler.cpp:
  Report an error on invalid syslog facility
ndb/src/mgmsrv/MgmtSrvr.cpp:
  Report error when adding log handler

ndb/include/logger/LogHandler.hpp

@@ -125,6 +125,18 @@ public:
    */
   void setErrorCode(int code);
 
+  /**
+   * Returns the error string.
+   */
+  char* getErrorStr();
+
+  /**
+   * Sets the error string.
+   *
+   * @param str the error string.
+   */
+  void setErrorStr(char* str);
+
   /**
    * Parse logstring parameters
    *
@@ -195,6 +207,7 @@ private:
 
   const char* m_pDateTimeFormat;
   int m_errorCode;
+  char* m_errorStr;
 
   // for handling repeated messages
   unsigned m_count_repeated_messages;

ndb/include/logger/Logger.hpp

@@ -178,8 +178,11 @@ public:
    * Add a new handler
    *
    * @param logstring string describing the handler to add
+   * @param err OS errno in event of error
+   * @param len max length of errStr buffer
+   * @param errStr logger error string in event of error
    */
-  bool addHandler(const BaseString &logstring);
+  bool addHandler(const BaseString &logstring, int *err, int len, char* errStr);
 
   /**
    * Remove a log handler.

ndb/src/common/logger/FileLogHandler.cpp

@@ -187,6 +187,7 @@ FileLogHandler::setParam(const BaseString &param, const BaseString &value){
     return setMaxSize(value);
   if(param == "maxfiles")
     return setMaxFiles(value);
+  setErrorStr("Invalid parameter");
   return false;
 }
 
@@ -196,16 +197,18 @@ FileLogHandler::setFilename(const BaseString &filename) {
   if(m_pLogFile)
     delete m_pLogFile;
   m_pLogFile = new File_class(filename.c_str(), "a+");
-  open();
-  return true;
+  return open();
 }
 
 bool
 FileLogHandler::setMaxSize(const BaseString &size) {
   char *end;
   long val = strtol(size.c_str(), &end, 0); /* XXX */
-  if(size.c_str() == end)
+  if(size.c_str() == end || val < 0)
+  {
+    setErrorStr("Invalid file size");
     return false;
+  }
   if(end[0] == 'M')
     val *= 1024*1024;
   if(end[0] == 'k')
@@ -220,8 +223,11 @@ bool
 FileLogHandler::setMaxFiles(const BaseString &files) {
   char *end;
   long val = strtol(files.c_str(), &end, 0);
-  if(files.c_str() == end)
+  if(files.c_str() == end || val < 1)
+  {
+    setErrorStr("Invalid maximum number of files");
     return false;
+  }
   m_maxNoFiles = val;
 
   return true;
@@ -230,6 +236,9 @@ FileLogHandler::setMaxFiles(const BaseString &files) {
 bool
 FileLogHandler::checkParams() {
   if(m_pLogFile == NULL)
+  {
+    setErrorStr("Log file cannot be null.");
     return false;
+  }
   return true;
 }

ndb/src/common/logger/LogHandler.cpp

@@ -23,7 +23,8 @@
 //
 LogHandler::LogHandler() : 
   m_pDateTimeFormat("%d-%.2d-%.2d %.2d:%.2d:%.2d"),
-  m_errorCode(0)
+  m_errorCode(0),
+  m_errorStr(NULL)
 {
   m_max_repeat_frequency= 3; // repeat messages maximum every 3 seconds
   m_count_repeated_messages= 0;
@@ -155,6 +156,19 @@ LogHandler::setErrorCode(int code)
   m_errorCode = code;
 }
 
+
+char*
+LogHandler::getErrorStr()
+{
+  return m_errorStr;
+}
+
+void
+LogHandler::setErrorStr(char* str)
+{
+  m_errorStr= str;
+}
+
 bool
 LogHandler::parseParams(const BaseString &_params) {
   Vector<BaseString> v_args;
@@ -165,10 +179,19 @@ LogHandler::parseParams(const BaseString &_params) {
   for(size_t i=0; i < v_args.size(); i++) {
     Vector<BaseString> v_param_value;
     if(v_args[i].split(v_param_value, "=", 2) != 2)
+    {
       ret = false;
-    else if (!setParam(v_param_value[0], v_param_value[1]))
+      setErrorStr("Can't find key=value pair.");
+    }
+    else
+    {
+      v_param_value[0].trim(" \t");
+      if (!setParam(v_param_value[0], v_param_value[1]))
+      {
         ret = false;
       }
+    }
+  }
 
   if(!checkParams())
     ret = false;

ndb/src/common/logger/Logger.cpp

@@ -167,7 +167,7 @@ Logger::addHandler(LogHandler* pHandler)
 }
 
 bool
-Logger::addHandler(const BaseString &logstring) {
+Logger::addHandler(const BaseString &logstring, int *err, int len, char* errStr) {
   size_t i;
   Vector<BaseString> logdest;
   Vector<LogHandler *>loghandlers;
@@ -200,9 +200,18 @@ Logger::addHandler(const BaseString &logstring) {
       handler = new ConsoleLogHandler();
     
     if(handler == NULL)
+    {
+      snprintf(errStr,len,"Could not create log destination: %s",
+               logdest[i].c_str());
       DBUG_RETURN(false);
+    }
     if(!handler->parseParams(params))
+    {
+      *err= handler->getErrorCode();
+      if(handler->getErrorStr())
+        strncpy(errStr, handler->getErrorStr(), len);
       DBUG_RETURN(false);
+    }
     loghandlers.push_back(handler);
   }
   

ndb/src/common/logger/SysLogHandler.cpp

@@ -154,5 +154,6 @@ SysLogHandler::setFacility(const BaseString &facility) {
       return true;
     }
   }
+  setErrorStr("Invalid syslog facility name");
   return false;
 }

ndb/src/mgmsrv/MgmtSrvr.cpp

@@ -179,6 +179,8 @@ MgmtSrvr::startEventLog()
   }
   
   const char * tmp;
+  char errStr[100];
+  int err= 0;
   BaseString logdest;
   char *clusterLog= NdbConfig_ClusterLogFileName(_ownNodeId);
   NdbAutoPtr<char> tmp_aptr(clusterLog);
@@ -192,9 +194,17 @@ MgmtSrvr::startEventLog()
     logdest.assfmt("FILE:filename=%s,maxsize=1000000,maxfiles=6", 
 		   clusterLog);
   }
-  if(!g_eventLogger.addHandler(logdest)) {
+  errStr[0]='\0';
+  if(!g_eventLogger.addHandler(logdest, &err, sizeof(errStr), errStr)) {
     ndbout << "Warning: could not add log destination \""
-	   << logdest.c_str() << "\"" << endl;
+           << logdest.c_str() << "\". Reason: ";
+    if(err)
+      ndbout << strerror(err);
+    if(err && errStr[0]!='\0')
+      ndbout << ", ";
+    if(errStr[0]!='\0')
+      ndbout << errStr;
+    ndbout << endl;
   }
 }