Add error handling in ConfigFile parser.

3 files changed, 202 insertions, 148 deletions

diff --git a/src/configfile.cc b/src/configfile.cc
index e659a51..6b0d14f 100644
--- a/src/configfile.cc
+++ b/src/configfile.cc
@@ -108,10 +108,12 @@ ConfigFile::~ConfigFile()
 {
 }
 
-void ConfigFile::load()
+bool ConfigFile::load()
 {
   DEBUG(configfile, "Loading config file...\n");
-  if(!open("r")) return;
+  if(!open("r")) {
+    return false;
+  }
 
   values.clear();
 
@@ -120,150 +122,26 @@ void ConfigFile::load()
     line = readLine();
 
     if(line == "") break;
-
-    if(line[line.size() - 1] == '\n') {
-      line = line.substr(0, line.size() - 1); // strip ending newline.
-    }
-
-    std::string key;
-    std::string value;
-    enum {
-      before_key,
-      in_key,
-      after_key,
-      before_value,
-      in_value,
-      in_value_single_quoted,
-      in_value_double_quoted,
-      after_value,
-    } state = before_key;
-
-    for(std::size_t p = 0; p < line.size(); ++p) {
-      switch(state) {
-      case before_key:
-        if(line[p] == '#') {
-          // Comment: Ignore line.
-          p = line.size();
-          continue;
-        }
-        if(std::isspace(line[p])) {
-          continue;
-        }
-        key += line[p];
-        state = in_key;
-        break;
-
-      case in_key:
-        if(std::isspace(line[p])) {
-          state = after_key;
-          continue;
-        }
-        if(line[p] == ':' || line[p] == '=') {
-          state = before_value;
-          continue;
-        }
-        key += line[p];
-        break;
-
-      case after_key:
-        if(std::isspace(line[p])) {
-          continue;
-        }
-        if(line[p] == ':' || line[p] == '=') {
-          state = before_value;
-          continue;
-        }
-        // ERROR: Bad symbol, expecting only whitespace or key/value seperator
-        break;
-
-      case before_value:
-        if(std::isspace(line[p])) {
-          continue;
-        }
-        if(line[p] == '\'') {
-          state = in_value_single_quoted;
-          continue;
-        }
-        if(line[p] == '"') {
-          state = in_value_double_quoted;
-          continue;
-        }
-        value += line[p];
-        state = in_value;
-        break;
-
-      case in_value:
-        if(std::isspace(line[p])) {
-          state = after_value;
-          continue;
-        }
-        if(line[p] == '#') {
-          // Comment: Ignore the rest of the line.
-          p = line.size();
-          state = after_value;
-          continue;
-        }
-        value += line[p];
-        break;
-
-      case in_value_single_quoted:
-        if(line[p] == '\'') {
-          state = after_value;
-          continue;
-        }
-        value += line[p];
-        break;
-
-      case in_value_double_quoted:
-        if(line[p] == '"') {
-          state = after_value;
-          continue;
-        }
-        value += line[p];
-        break;
-
-      case after_value:
-        if(std::isspace(line[p])) {
-          continue;
-        }
-        if(line[p] == '#') {
-          // Comment: Ignore the rest of the line.
-          p = line.size();
-          continue;
-        }
-        // ERROR: Bad symbol, expecting only whitespace or key/value seperator
-        break;
-      }
-    }
-
-    if(state == before_key) {
-      // Line did not contain any data (empty or comment)
-      continue;
-    }
-
-    // If state == in_value_XXX_quoted here, the string was not terminated.
-    if(state != after_value && state != in_value) {
-      // ERROR: Malformed line.
-      break;
-    }
-
-    DEBUG(configfile, "key['%s'] value['%s']\n", key.c_str(), value.c_str());
-
-    if(key != "") {
-      values[key] = value;
+    
+    if(!parseLine(line)) {
+      return false;
     }
   }
 
   close();
+
+  return true;
 }
 
-void ConfigFile::save()
+bool ConfigFile::save()
 {
   DEBUG(configfile, "Saving configuration...\n");
 
   createConfigPath();
 
-  if(!open("w")) return;
+  if(!open("w")) {
+    return false;
+  }
 
   std::map<std::string, std::string>::iterator v = values.begin();
   for(; v != values.end(); ++v) {
@@ -271,6 +149,8 @@ void ConfigFile::save()
   }
 
   close();
+
+  return true;
 }
 
 std::string ConfigFile::getValue(const std::string& key)
@@ -328,3 +208,140 @@ std::string ConfigFile::readLine()
 
   return line;
 }
+
+bool ConfigFile::parseLine(const std::string& line)
+{
+  std::string key;
+  std::string value;
+  enum {
+    before_key,
+    in_key,
+    after_key,
+    before_value,
+    in_value,
+    in_value_single_quoted,
+    in_value_double_quoted,
+    after_value,
+  } state = before_key;
+
+  for(std::size_t p = 0; p < line.size(); ++p) {
+    switch(state) {
+    case before_key:
+      if(line[p] == '#') {
+        // Comment: Ignore line.
+        p = line.size();
+        continue;
+      }
+      if(std::isspace(line[p])) {
+        continue;
+      }
+      key += line[p];
+      state = in_key;
+      break;
+
+    case in_key:
+      if(std::isspace(line[p])) {
+        state = after_key;
+        continue;
+      }
+      if(line[p] == ':' || line[p] == '=') {
+        state = before_value;
+        continue;
+      }
+      key += line[p];
+      break;
+
+    case after_key:
+      if(std::isspace(line[p])) {
+        continue;
+      }
+      if(line[p] == ':' || line[p] == '=') {
+        state = before_value;
+        continue;
+      }
+      ERR(configfile, "Bad symbol."
+            " Expecting only whitespace or key/value seperator: '%s'",
+            line.c_str());
+      return false;
+
+    case before_value:
+      if(std::isspace(line[p])) {
+        continue;
+      }
+      if(line[p] == '\'') {
+        state = in_value_single_quoted;
+        continue;
+      }
+      if(line[p] == '"') {
+        state = in_value_double_quoted;
+        continue;
+      }
+      value += line[p];
+      state = in_value;
+      break;
+
+    case in_value:
+      if(std::isspace(line[p])) {
+        state = after_value;
+        continue;
+      }
+      if(line[p] == '#') {
+        // Comment: Ignore the rest of the line.
+        p = line.size();
+        state = after_value;
+        continue;
+      }
+      value += line[p];
+      break;
+
+    case in_value_single_quoted:
+      if(line[p] == '\'') {
+        state = after_value;
+        continue;
+      }
+      value += line[p];
+      break;
+
+    case in_value_double_quoted:
+      if(line[p] == '"') {
+        state = after_value;
+        continue;
+      }
+      value += line[p];
+      break;
+
+    case after_value:
+      if(std::isspace(line[p])) {
+        continue;
+      }
+      if(line[p] == '#') {
+        // Comment: Ignore the rest of the line.
+        p = line.size();
+        continue;
+      }
+      ERR(configfile, "Bad symbol."
+            " Expecting only whitespace or key/value seperator: '%s'",
+            line.c_str());
+      return false;
+    }
+  }
+
+  if(state == before_key) {
+    // Line did not contain any data (empty or comment)
+    return true;
+  }
+
+  // If state == in_value_XXX_quoted here, the string was not terminated.
+  if(state != after_value && state != in_value) {
+    ERR(configfile,"Malformed line: '%s'", line.c_str());
+    return false;
+  }
+
+  DEBUG(configfile, "key['%s'] value['%s']\n", key.c_str(), value.c_str());
+
+  if(key != "") {
+    values[key] = value;
+  }
+
+  return true;
+}
diff --git a/src/configfile.h b/src/configfile.h
index a3fd588..a6c50bd 100644
--- a/src/configfile.h
+++ b/src/configfile.h
@@ -36,8 +36,8 @@ public:
   ConfigFile(std::string filename);
   virtual ~ConfigFile();
 
-  virtual void load();
-  virtual void save();
+  virtual bool load();
+  virtual bool save();
 
   virtual std::string getValue(const std::string& key);
   virtual void setValue(const std::string& key, const std::string& value);
@@ -49,6 +49,7 @@ protected:
   virtual bool open(std::string mode);
   void close();
   std::string readLine();
+  bool parseLine(const std::string& line);
 
   FILE* fp;
 };
diff --git a/test/configtest.cc b/test/configtest.cc
index 6adcd39..24a7048 100644
--- a/test/configtest.cc
+++ b/test/configtest.cc
@@ -58,6 +58,10 @@ class test_configtest : public CppUnit::TestFixture
 	CPPUNIT_TEST(loading_inline_comment);
 	CPPUNIT_TEST(loading_single_quoted_string);
 	CPPUNIT_TEST(loading_double_quoted_string);
+	CPPUNIT_TEST(loading_error_no_key);
+	CPPUNIT_TEST(loading_error_no_value);
+	CPPUNIT_TEST(loading_error_string_not_terminated_single);
+	CPPUNIT_TEST(loading_error_string_not_terminated_double);
 	CPPUNIT_TEST_SUITE_END();
 
 public:
@@ -82,7 +86,7 @@ public:
     writeFile("a:b");
 
     TestConfigFile cf;
-    cf.load();
+    CPPUNIT_ASSERT_EQUAL(true, cf.load());
     CPPUNIT_ASSERT_EQUAL(std::string("b"), cf.getValue("a"));
 	}
 
@@ -91,7 +95,7 @@ public:
     writeFile(" a =\tb\t\n");
 
     TestConfigFile cf;
-    cf.load();
+    CPPUNIT_ASSERT_EQUAL(true, cf.load());
     CPPUNIT_ASSERT_EQUAL(std::string("b"), cf.getValue("a"));
 	}
 
@@ -100,7 +104,7 @@ public:
     writeFile("a:b\n");
 
     TestConfigFile cf;
-    cf.load();
+    CPPUNIT_ASSERT_EQUAL(true, cf.load());
     CPPUNIT_ASSERT_EQUAL(std::string("b"), cf.getValue("a"));
 	}
 
@@ -109,7 +113,7 @@ public:
     writeFile(" a : b ");
 
     TestConfigFile cf;
-    cf.load();
+    CPPUNIT_ASSERT_EQUAL(true, cf.load());
     CPPUNIT_ASSERT_EQUAL(std::string("b"), cf.getValue("a"));
 	}
 
@@ -118,7 +122,7 @@ public:
     writeFile("\ta\t:\tb\t");
 
     TestConfigFile cf;
-    cf.load();
+    CPPUNIT_ASSERT_EQUAL(true, cf.load());
     CPPUNIT_ASSERT_EQUAL(std::string("b"), cf.getValue("a"));
 	}
 
@@ -127,7 +131,7 @@ public:
     writeFile(" a : b \n");
 
     TestConfigFile cf;
-    cf.load();
+    CPPUNIT_ASSERT_EQUAL(true, cf.load());
     CPPUNIT_ASSERT_EQUAL(std::string("b"), cf.getValue("a"));
 	}
 
@@ -136,7 +140,7 @@ public:
     writeFile("\ta\t:\tb\t\n");
 
     TestConfigFile cf;
-    cf.load();
+    CPPUNIT_ASSERT_EQUAL(true, cf.load());
     CPPUNIT_ASSERT_EQUAL(std::string("b"), cf.getValue("a"));
 	}
 
@@ -145,7 +149,7 @@ public:
     writeFile("# comment\na:b\n");
 
     TestConfigFile cf;
-    cf.load();
+    CPPUNIT_ASSERT_EQUAL(true, cf.load());
     CPPUNIT_ASSERT_EQUAL(std::string("b"), cf.getValue("a"));
 	}
 
@@ -154,7 +158,7 @@ public:
     writeFile("a:b #comment\n");
 
     TestConfigFile cf;
-    cf.load();
+    CPPUNIT_ASSERT_EQUAL(true, cf.load());
     CPPUNIT_ASSERT_EQUAL(std::string("b"), cf.getValue("a"));
 	}
 
@@ -163,7 +167,7 @@ public:
     writeFile("a: '#\"b\" ' \n");
 
     TestConfigFile cf;
-    cf.load();
+    CPPUNIT_ASSERT_EQUAL(true, cf.load());
     CPPUNIT_ASSERT_EQUAL(std::string("#\"b\" "), cf.getValue("a"));
 	}
 
@@ -172,9 +176,41 @@ public:
     writeFile("a: \"#'b' \" \n");
 
     TestConfigFile cf;
-    cf.load();
+    CPPUNIT_ASSERT_EQUAL(true, cf.load());
     CPPUNIT_ASSERT_EQUAL(std::string("#'b' "), cf.getValue("a"));
 	}
+
+  void loading_error_no_key()
+  {
+    writeFile(":foo");
+
+    TestConfigFile cf;
+    CPPUNIT_ASSERT_EQUAL(false, cf.load());
+	}
+
+  void loading_error_no_value()
+  {
+    writeFile("key");
+
+    TestConfigFile cf;
+    CPPUNIT_ASSERT_EQUAL(false, cf.load());
+	}
+
+  void loading_error_string_not_terminated_single()
+  {
+    writeFile("a:'b\n");
+
+    TestConfigFile cf;
+    CPPUNIT_ASSERT_EQUAL(false, cf.load());
+	}
+
+  void loading_error_string_not_terminated_double()
+  {
+    writeFile("a:\"b\n");
+
+    TestConfigFile cf;
+    CPPUNIT_ASSERT_EQUAL(false, cf.load());
+	}
 };
 
 // Registers the fixture into the 'registry'