]> git.apps.os.sepia.ceph.com Git - ceph.git/commitdiff
ConfUtils: new parser
authorColin Patrick McCabe <cmccabe@alumni.cmu.edu>
Mon, 4 Apr 2011 20:06:35 +0000 (13:06 -0700)
committerColin Patrick McCabe <cmccabe@alumni.cmu.edu>
Tue, 5 Apr 2011 21:12:27 +0000 (14:12 -0700)
A new parser for ConfUtils that outputs a list of syntax errors when
files are wrong.

It also checks that configuration files are valid utf-8 and don't contain
embedded nulls.

Also added a few more unit tests.

Signed-off-by: Colin McCabe <colin.mccabe@dreamhost.com>
src/auth/KeyRing.cc
src/common/ConfUtils.cc
src/common/ConfUtils.h
src/common/config.cc
src/test/confutils.cc

index 25579b938eb2838a9a9c13ea22a53b583d107ffb..c904b6da787f9a59c919774a1b615ab6f175c2f5 100644 (file)
@@ -32,15 +32,6 @@ using namespace std;
 
 KeyRing g_keyring;
 
-static string remove_quotes(string& s)
-{
-  int n = s.size();
-  /* remove quotes from string */
-  if (s[0] == '"' && s[n - 1] == '"')
-    return s.substr(1, n-2);
-  return s;
-}
-
 int KeyRing::set_modifier(const char *type, const char *val, EntityName& name, map<string, bufferlist>& caps)
 {
   if (!val)
@@ -49,7 +40,6 @@ int KeyRing::set_modifier(const char *type, const char *val, EntityName& name, m
   if (strcmp(type, "key") == 0) {
     CryptoKey key;
     string l(val);
-    l = remove_quotes(l);
     try {
       key.decode_base64(l);
     } catch (const buffer::error& err) {
@@ -61,7 +51,6 @@ int KeyRing::set_modifier(const char *type, const char *val, EntityName& name, m
     if (!*caps_entity)
       return -EINVAL;
       string l(val);
-      l = remove_quotes(l);
       bufferlist bl;
       ::encode(l, bl);
       caps[caps_entity] = bl;
@@ -111,10 +100,9 @@ void KeyRing::decode_plaintext(bufferlist::iterator& bli)
     goto done_err;
   }
 
-  for (std::list<ConfSection*>::const_iterator p =
-           cf.get_section_list().begin();
-       p != cf.get_section_list().end(); ++p) {
-    string name = (*p)->get_name();
+  for (ConfFile::const_section_iter_t s = cf.sections_begin();
+           s != cf.sections_end(); ++s) {
+    string name = s->first;
     if (name == "global")
       continue;
 
@@ -125,17 +113,14 @@ void KeyRing::decode_plaintext(bufferlist::iterator& bli)
       goto done_err;
     }
 
-    ConfList& cl = (*p)->get_list();
-    ConfList::iterator cli;
-    for (cli = cl.begin(); cli != cl.end(); ++ cli) {
-      ConfLine *line = *cli;
-      const char *type = line->get_var();
-      if (!type)
+    for (ConfSection::const_line_iter_t l = s->second.lines.begin();
+        l != s->second.lines.end(); ++l) {
+      if (l->key.empty())
         continue;
-
-      ret = set_modifier(type, line->get_val(), ename, caps);
+      ret = set_modifier(l->key.c_str(), l->val.c_str(), ename, caps);
       if (ret < 0) {
-        derr << "error setting modifier for [" << name << "] type=" << type << " val=" << line->get_val() << dendl;
+        derr << "error setting modifier for [" << name << "] type=" << l->key
+            << " val=" << l->val << dendl;
         goto done_err;
       }
     }
index 0b221c8a04300c6f5df8d7a95462cc9222035931..a0878dfeb50e91e88b983da9b112c58f57c010b0 100644 (file)
+// -*- mode:C++; tab-width:8; c-basic-offset:2; indent-tabs-mode:t -*-
+// vim: ts=8 sw=2 smarttab
+/*
+ * Ceph - scalable distributed file system
+ *
+ * Copyright (C) 2011 New Dream Network
+ *
+ * This is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public
+ * License version 2.1, as published by the Free Software
+ * Foundation.  See file COPYING.
+ *
+ */
+
+#include <errno.h>
+#include <list>
+#include <map>
+#include <sstream>
+#include <stdio.h>
 #include <stdlib.h>
 #include <string.h>
-#include <stdio.h>
-#include <unistd.h>
-#include <sys/types.h>
+#include <string>
 #include <sys/stat.h>
-#include <fcntl.h>
-#include <errno.h>
-#include <wctype.h>
+#include <sys/types.h>
+#include <unistd.h>
 
-#include <map>
-#include <list>
-#include <string>
+#include "include/buffer.h"
+#include "common/errno.h"
+#include "common/utf8.h"
+#include "common/ConfUtils.h"
 
-#include "common/safe_io.h"
-#include "ConfUtils.h"
-#include "dyn_snprintf.h"
+using std::cerr;
+using std::ostringstream;
+using std::pair;
+using std::string;
 
-using namespace std;
+#define MAX_CONFIG_FILE_SZ 0x40000000
 
-struct ltstr
+////////////////////////////// ConfLine //////////////////////////////
+ConfLine::
+ConfLine(const std::string &key_, const std::string val_,
+      const std::string newsection_, const std::string comment_, int line_no_)
+  : key(key_), val(val_), newsection(newsection_)
 {
-  bool operator()(const char* s1, const char* s2) const
-  {
-    return strcmp(s1, s2) < 0;
-  }
-};
-
-static const char *_def_delim=" \t\n\r";
-static const char *_eq_delim="= \t\n\r";
-static const char *_eq_nospace_delim="=\t\n\r";
-static const char *_eol_delim="\n\r";
-/* static const char *_pr_delim="[] \t\n\r"; */
-
+  // If you want to implement writable ConfFile support, you'll need to save
+  // the comment and line_no arguments here.
+}
 
-static int is_delim(char c, const char *delim)
+bool ConfLine::
+operator<(const ConfLine &rhs) const
 {
-       while (*delim) {
-               if (c==*delim)
-                       return 1;
-               delim++;
-       }
-
-       return 0;
+  // We only compare keys.
+  // If you have more than one line with the same key in a given section, the
+  // last one wins.
+  if (key < rhs.key)
+    return true;
+  else
+    return false;
 }
 
-static wchar_t get_ucs2(unsigned char **pstr)
+std::ostream &operator<<(std::ostream& oss, const ConfLine &l)
 {
-       unsigned char *s = *pstr;
-       wchar_t wc = 0;
-       int b, i;
-       int num_bytes = 0;
-
-       if (*s <= 0x7f) {
-               wc = *s;
-               *pstr = ++s;
-               return wc;
-       }
-
-       b=0x40;
-       while (b & *s) {
-               num_bytes++;
-               b >>= 1;
-       }
-       if (!num_bytes)
-               return *s; /* this is not a correctly encoded utf8 */
-
-       wc = *s & (b-1);
-       wc <<= (num_bytes * 6);
-
-       s++;
-
-       for (i=0; i<num_bytes; i++, s++) {
-               int shift;
-               wchar_t w;
-               if ((*s & 0xc0) != 0x80) {
-                       *pstr = s;
-                       return 0; /* not a valid utf8 */
-               }
-               shift = (num_bytes - i - 1) * 6;
-               w = *s & 0x3f;
-               w <<= shift;
-               wc |= w;
-       }
-       *pstr = s;
-       return wc;
+  oss << "ConfLine(key = '" << l.key << "', val='"
+      << l.val << "', newsection='" << l.newsection << "')";
+  return oss;
 }
-
-static void skip_whitespace(char **pstr, const char *delim)
+///////////////////////// ConfFile //////////////////////////
+ConfFile::
+ConfFile()
 {
-       unsigned char *str = (unsigned char *)*pstr;
-       wchar_t wc;
-
-       do {
-               while (*str && is_delim((char)*str, delim)) {
-                       str++;
-               }
-               if (!*str)
-                       return;
-               *pstr = (char *)str;
-               wc = get_ucs2(&str);
-       } while (!iswprint(wc));
 }
 
-static char *get_next_tok(char *str, const char *delim, int alloc, char **p)
+ConfFile::
+~ConfFile()
 {
-       int i=0;
-       char *out;
-
-        skip_whitespace(&str, delim);
-
-       if (*str == '"') {
-               while (str[i] && !is_delim(str[i], _eol_delim)) {
-                       i++;
-                       if (str[i] == '"') {
-                               i++;
-                               break;
-                       }
-               }               
-       } else {
-               while (str[i] && !is_delim(str[i], delim)) {
-                       i++;
-               }
-       }
-
-       if (alloc) {
-               out = (char *)malloc(i+1);
-               memcpy(out, str, i);
-               out[i] = '\0';
-       } else {
-               out = str;
-       }
-
-       if (p)
-               *p = &str[i];
-
-       return out;
 }
 
-static char *get_next_name(char *str, int alloc, char **p)
+void ConfFile::
+clear()
 {
-       int i=0;
-       char *out;
-
-       while (*str && is_delim(*str, _def_delim)) {
-               str++;
-       }
-
-       if (*str == '"') {
-               while (str[i] && !is_delim(str[i], _eol_delim)) {
-                       i++;
-                       if (str[i] == '"') {
-                               i++;
-                               break;
-                       }
-               }
-       } else {
-               while (str[i] && !is_delim(str[i], _eq_nospace_delim)) {
-                       i++;
-               }
-
-               i--;
-
-               while (str[i] && is_delim(str[i], _def_delim)) {
-                       i--;
-               }
-
-               i++;
-       }
-
-       if (alloc) {
-               out = (char *)malloc(i+1);
-               memcpy(out, str, i);
-               out[i] = '\0';
-       } else {
-               out = str;
-       }
-
-       if (p)
-               *p = &str[i];
-
-       return out;
+  sections.clear();
 }
 
-/*
- * normalizes a var name, removes extra spaces; e.g., 'foo  bar' -> 'foo bar'
+/* We load the whole file into memory and then parse it.  Although this is not
+ * the optimal approach, it does mean that most of this code can be shared with
+ * the bufferlist loading function. Since bufferlists are always in-memory, the
+ * load_from_buffer interface works well for them.
+ * In general, configuration files should be a few kilobytes at maximum, so
+ * loading the whole configuration into memory shouldn't be a problem.
  */
-static char *normalize_name(const char *name)
-{
-       char *newname, *p;
-       int i, len;
-       int last_delim = 0, had_delim = 0, had_non_delim = 0;
-
-       if (!name)
-               return NULL;
-
-       len = strlen(name);
-       newname = (char *)malloc(len + 1);
-
-       p = newname;
-
-       for (i=0; i < len; i++) {
-               int now_delim = is_delim(name[i], _def_delim);
-
-               if (!now_delim) {
+int ConfFile::
+parse_file(const std::string &fname, std::deque<std::string> *errors)
+{
+  int ret = 0;
+  size_t sz;
+  char *buf = NULL;
+  FILE *fp = fopen(fname.c_str(), "r");
+  if (!fp) {
+    ret = errno;
+    ostringstream oss;
+    oss << "read_conf: failed to open '" << fname << "': " << cpp_strerror(ret);
+    errors->push_back(oss.str());
+    return ret;
+  }
 
-                       if (had_delim && had_non_delim)
-                               *p++ = ' ';
+  struct stat st_buf;
+  if (fstat(fileno(fp), &st_buf)) {
+    ret = errno;
+    ostringstream oss;
+    oss << "read_conf: failed to fstat '" << fname << "': " << cpp_strerror(ret);
+    errors->push_back(oss.str());
+    goto done;
+  }
 
-                       *p++ = name[i];
+  if (st_buf.st_size > MAX_CONFIG_FILE_SZ) {
+    ostringstream oss;
+    oss << "read_conf: config file '" << fname << "' is " << st_buf.st_size
+       << " bytes, but the maximum is " << MAX_CONFIG_FILE_SZ;
+    errors->push_back(oss.str());
+    ret = -EINVAL;
+    goto done;
+  }
 
-                       had_non_delim = 1;
-                       had_delim = 0;
-               } else {
-                       had_delim = 1;
-               }
+  sz = (size_t)st_buf.st_size;
+  buf = (char*)malloc(sz);
+
+  if (fread(buf, 1, sz, fp) != sz) {
+    if (ferror(fp)) {
+      ret = errno;
+      ostringstream oss;
+      oss << "read_conf: fread error while reading '" << fname << "': "
+         << cpp_strerror(ret);
+      errors->push_back(oss.str());
+      goto done;
+    }
+    else {
+      ostringstream oss;
+      oss << "read_conf: unexpected EOF while reading '" << fname << "': "
+         << "possible concurrent modification?";
+      errors->push_back(oss.str());
+      ret = -EIO;
+      goto done;
+    }
+  }
 
-               last_delim = now_delim;
-       }
-       *p = '\0';
+  load_from_buffer(buf, sz, errors);
+  ret = 0;
 
-       return newname;
+done:
+  free(buf);
+  fclose(fp);
+  return ret;
 }
 
-#define MAX_LINE 256
-
-static char *get_next_delim(char *str, const char *delim, int alloc, char **p)
+int ConfFile::
+parse_bufferlist(ceph::bufferlist *bl, std::deque<std::string> *errors)
 {
-       int i=0;
-       char *out;
-
-       while (str[i] && is_delim(str[i], delim)) {
-               i++;
-       }
-
-       if (alloc) {
-               out = (char *)malloc(i+1);
-               memcpy(out, str, i);
-               out[i] = '\0';
-       } else {
-               out = str;
-       }
-
-       *p = &str[i];
-
-       return out;
+  load_from_buffer(bl->c_str(), bl->length(), errors);
+  return 0;
 }
 
-static int _parse_section(char *str, ConfLine *parsed)
+int ConfFile::
+read(const std::string &section, const std::string &key, std::string &val) const
 {
-       char *name = NULL;
-       char *p;
-       int ret = 0;
-       char *line;
-       size_t max_line = MAX_LINE;
-
-       char *start, *end;
-
-       start = index(str, '[');
-       end = strchr(str, ']'); 
-
-       if (!start || !end)
-               return 0;
-
-       start++;
-       *end = '\0';
-
-       if (end <= start)
-               return 0;
-       
-
-       p = start;
-       line = (char *)malloc(max_line);
-       line[0] ='\0';
-
-       do {
-               if (name)
-                       free(name);
-               name = get_next_tok(p, _def_delim, 1, &p);
-
-               if (*name) {
-                       if (*line)
-                               dyn_snprintf(&line, &max_line, 2, "%s %s", line, name);
-                       else
-                               dyn_snprintf(&line, &max_line, 1, "%s", name);
-               }
-       } while (*name);
-       if (name)
-               free(name);
-       
-       if (*line)      
-               parsed->set_section(line);
-
-       free(line);
-
-       return ret;
+  const_section_iter_t s = sections.find(section);
+  if (s == sections.end())
+    return -ENOENT;
+  ConfLine exemplar(key, "", "", "", 0);
+  ConfSection::const_line_iter_t l = s->second.lines.find(exemplar);
+  if (l == s->second.lines.end())
+    return -ENOENT;
+  val = l->val;
+  return 0;
 }
 
-int parse_line(char *line, ConfLine *parsed)
+ConfFile::const_section_iter_t ConfFile::
+sections_begin() const
 {
-       char *dup=strdup(line);
-       char *p = NULL;
-       char *eq;
-       int ret = 0;
-
-       memset(parsed, 0, sizeof(ConfLine));
-
-       parsed->set_prefix(get_next_delim(dup, _def_delim, 1, &p), false);
-
-       if (!*p)
-               goto out;
-
-       switch (*p) {
-               case ';':
-               case '#':
-                       parsed->set_suffix(p);
-                       goto out;
-               case '[':
-                       parsed->set_suffix(p);
-                       ret = _parse_section(p, parsed);
-                       free(dup);
-                       return ret;
-       }
-
-       parsed->set_var(get_next_name(p, 1, &p), false);
-       if (!*p)
-               goto out;
-
-       parsed->set_mid(get_next_delim(p, _eq_delim, 1, &p), false);
-       if (!*p)
-               goto out;
-
-       eq = get_next_tok(parsed->get_mid(), _def_delim, 0, NULL);
-       if (*eq != '=') {
-               goto out;
-       }
-
-       parsed->set_val(get_next_tok(p, _def_delim, 1, &p), false);
-       if (!*p)
-               goto out;
-
-       ret = 1;
-
-       parsed->set_suffix(p);
-out:
-       free(dup);
-       return ret;
+  return sections.begin();
 }
 
-static int _str_cat(char *str1, int max, char *str2)
+ConfFile::const_section_iter_t ConfFile::
+sections_end() const
 {
-       int len = 0;
-
-       if (max)
-               len = snprintf(str1, max, "%s", str2);
-
-       if (len < 0)
-               len = 0;
-
-       return len;
+  return sections.end();
 }
 
-ConfSection::~ConfSection()
+void ConfFile::
+trim_whitespace(std::string &str, bool strip_internal)
 {
-       ConfList::iterator conf_iter, conf_end;
-       ConfLine *cl;
-
-       conf_end = conf_list.end();
+  // strip preceding
+  const char *in = str.c_str();
+  while (true) {
+    char c = *in;
+    if ((!c) || (!isspace(c)))
+      break;
+    ++in;
+  }
+  char output[strlen(in) + 1];
+  strcpy(output, in);
+
+  // strip trailing
+  char *o = output + strlen(output);
+  while (true) {
+    if (o == output)
+      break;
+    --o;
+    if (!isspace(*o)) {
+      ++o;
+      *o = '\0';
+      break;
+    }
+  }
 
-       for (conf_iter = conf_list.begin(); conf_iter != conf_end; ++conf_iter) {
-               cl = *conf_iter;
+  if (!strip_internal) {
+    str.assign(output);
+    return;
+  }
 
-               delete cl;
-       }
+  // strip internal
+  char output2[strlen(output) + 1];
+  char *out2 = output2;
+  bool prev_was_space = false;
+  for (char *u = output; *u; ++u) {
+    char c = *u;
+    if (isspace(c)) {
+      if (!prev_was_space)
+       *out2++ = c;
+      prev_was_space = true;
+    }
+    else {
+      *out2++ = c;
+      prev_was_space = false;
+    }
+  }
+  *out2++ = '\0';
+  str.assign(output2);
 }
 
-void ConfLine::_set(char **dst, const char *val, bool alloc)
+std::ostream &operator<<(std::ostream &oss, const ConfFile &cf)
 {
-       if (*dst)
-               free((void *)*dst);
-
-       if (!val) {
-               *dst = NULL;
-       } else {
-               if (alloc)
-                       *dst = strdup(val);
-               else
-                       *dst = (char *)val;
-       }
-}
+  for (ConfFile::const_section_iter_t s = cf.sections_begin();
+       s != cf.sections_end(); ++s) {
+    oss << "[" << s->first << "]\n";
+    for (ConfSection::const_line_iter_t l = s->second.lines.begin();
+        l != s->second.lines.end(); ++l) {
+      if (!l->key.empty()) {
+       oss << "\t" << l->key << " = \"" << l->val << "\"\n";
+      }
+    }
+  }
+  return oss;
+}
+
+void ConfFile::
+load_from_buffer(const char *buf, size_t sz, std::deque<std::string> *errors)
+{
+  errors->clear();
+
+  section_iter_t::value_type vt("global", ConfSection());
+  pair < section_iter_t, bool > vr(sections.insert(vt));
+  assert(vr.second);
+  section_iter_t cur_section = vr.first;
+  std::string acc;
+
+  const char *b = buf;
+  int line_no = 0;
+  size_t line_len = -1;
+  size_t rem = sz;
+  while (1) {
+    b += line_len + 1;
+    rem -= line_len + 1;
+    if (rem == 0)
+      break;
+    line_no++;
+
+    // look for the next newline
+    const char *end = (const char*)memchr(b, '\n', rem);
+    if (!end) {
+      ostringstream oss;
+      oss << "read_conf: ignoring line " << line_no << " because it doesn't "
+         << "end with a newline! Please end the config file with a newline.";
+      errors->push_back(oss.str());
+      break;
+    }
+
+    // find length of line, and search for NULLs
+    line_len = 0;
+    bool found_null = false;
+    for (const char *tmp = b; tmp != end; ++tmp) {
+      line_len++;
+      if (*tmp == '\0') {
+       found_null = true;
+      }
+    }
+
+    if (found_null) {
+      ostringstream oss;
+      oss << "read_conf: ignoring line " << line_no << " because it has "
+         << "an embedded null.";
+      errors->push_back(oss.str());
+      acc.clear();
+      continue;
+    }
+
+    if (check_utf8(b, line_len)) {
+      ostringstream oss;
+      oss << "read_conf: ignoring line " << line_no << " because it is not "
+         << "valid UTF8.";
+      errors->push_back(oss.str());
+      acc.clear();
+      continue;
+    }
+
+    if ((line_len >= 1) && (b[line_len-1] == '\\')) {
+      // A backslash at the end of a line serves as a line continuation marker.
+      // Combine the next line with this one.
+      // Remove the backslash itself from the text.
+      acc.append(b, line_len - 1);
+      continue;
+    }
+
+    acc.append(b, line_len);
+
+    //cerr << "acc = '" << acc << "'" << std::endl;
+    ConfLine *cline = process_line(line_no, acc.c_str(), errors);
+    acc.clear();
+    if (!cline)
+      continue;
+    const std::string &csection(cline->newsection);
+    if (!csection.empty()) {
+      std::map <std::string, ConfSection>::value_type nt(csection, ConfSection());
+      pair < section_iter_t, bool > nr(sections.insert(nt));
+      cur_section = nr.first;
+    }
+    else if (!cline->key.empty()) {
+      // add line to current section
+      //std::cerr << "cur_section = " << cur_section->first << ", " << *cline << std::endl;
+      cur_section->second.lines.insert(*cline);
+    }
+    delete cline;
+  }
 
-void ConfLine::set_prefix(const char *val, bool alloc)
-{
-       _set(&prefix, val, alloc);
+  if (!acc.empty()) {
+    ostringstream oss;
+    oss << "read_conf: don't end with lines that end in backslashes!";
+    errors->push_back(oss.str());
+  }
 }
 
-void ConfLine::set_var(const char *val, bool alloc)
-{
-       _set(&var, val, alloc);
-
-       if (norm_var) {
-               free(norm_var);
-               norm_var = NULL;
+/*
+ * A simple state-machine based parser.
+ * This probably could/should be rewritten with something like boost::spirit
+ * or yacc if the grammar ever gets more complex.
+ */
+ConfLine* ConfFile::
+process_line(int line_no, const char *line, std::deque<std::string> *errors)
+{
+  enum acceptor_state_t {
+    ACCEPT_INIT,
+    ACCEPT_SECTION_NAME,
+    ACCEPT_KEY,
+    ACCEPT_VAL_START,
+    ACCEPT_UNQUOTED_VAL,
+    ACCEPT_QUOTED_VAL,
+    ACCEPT_COMMENT_START,
+    ACCEPT_COMMENT_TEXT,
+  };
+  const char *l = line;
+  acceptor_state_t state = ACCEPT_INIT;
+  string key, val, newsection, comment;
+  while (true) {
+    char c = *l++;
+    switch (state) {
+      case ACCEPT_INIT:
+       if (c == '\0')
+         return NULL; // blank line. Not an error, but not interesting either.
+       else if (c == '[')
+         state = ACCEPT_SECTION_NAME;
+       else if ((c == '#') || (c == ';'))
+         state = ACCEPT_COMMENT_TEXT;
+       else if (c == ']') {
+         ostringstream oss;
+         oss << "unexpected right bracket at char " << (l - line)
+             << ", line " << line_no;
+         errors->push_back(oss.str());
+         return NULL;
        }
-}
-
-void ConfLine::set_mid(const char *val, bool alloc)
-{
-       _set(&mid, val, alloc);
-}
-
-void ConfLine::set_val(const char *val, bool alloc)
-{
-       _set(&this->val, val, alloc);
-}
-
-void ConfLine::set_suffix(const char *val, bool alloc)
-{
-       _set(&suffix, val, alloc);
-}
-
-void ConfLine::set_section(const char *val, bool alloc)
-{
-       _set(&section, val, alloc);
-}
-
-char *ConfLine::get_norm_var()
-{
-       if (!norm_var)
-               norm_var = normalize_name(var);
-
-       return norm_var;
-}
-
-
-ConfLine::~ConfLine()
-{
-       if (prefix)
-               free(prefix);
-       if(var)
-               free(var);
-       if (mid)
-               free(mid);
-       if (val)
-               free(val);
-       if (suffix)
-               free(suffix);
-       if (section)
-               free(section);
-}
-
-void ConfFile::common_init()
-{
-  filename = NULL;
-  pbl = NULL;
-  buf_pos = 0;
-  post_process_func = NULL;
-  default_global = true;
-}
-
-ConfFile::ConfFile()
-  : parse_lock("ConfFile::parse_lock", false, false, false)
-{
-}
-
-ConfFile::~ConfFile()
-{
-       SectionList::iterator sec_iter, sec_end;
-       ConfSection *sec;
-
-       free(filename);
-
-       sec_end = sections_list.end();
-
-       for (sec_iter = sections_list.begin(); sec_iter != sec_end; ++sec_iter) {
-               sec = *sec_iter;
-
-               delete sec;
+       else if (isspace(c)) {
+         // ignore whitespace here
        }
-}
-
-int ConfLine::output(char *line, int max_len)
-{
-       int len = 0;
-
-       if (!max_len)
-               return 0;
-
-       line[0] = '\0';
-       if (prefix)
-               len += _str_cat(&line[len], max_len-len, prefix);
-       if (var)
-               len += _str_cat(&line[len], max_len-len, var);
-       if (mid)
-               len += _str_cat(&line[len], max_len-len, mid);
-       if (val)
-               len += _str_cat(&line[len], max_len-len, val);
-       if (suffix)
-               len += _str_cat(&line[len], max_len-len, suffix);
-
-       return len;
-}
-
-
-void ConfFile::_dump(int fd)
-{
-       SectionList::iterator sec_iter, sec_end;
-       ConfLine *cl;
-       char *line;
-       size_t max_line = MAX_LINE;
-       size_t len;
-       char *p;
-       int r;
-
-       line = (char *)malloc(max_line);
-
-       sec_end=sections_list.end();
-
-       for (sec_iter=sections_list.begin(); sec_iter != sec_end; ++sec_iter) {
-               ConfList::iterator iter, end;
-               ConfSection *sec;
-               sec = *sec_iter;
-
-               end = sec->conf_list.end();
-
-               for (iter = sec->conf_list.begin(); iter != end; ++iter) {
-                       cl = *iter;
-                       p = line;
-                       len = 0;
-
-                       if (cl) {
-                               line[0] = '\0';
-                               do {
-                                       if (len >= max_line) {
-                                               max_line *= 2;
-                                               free(line);
-                                               line = (char *)malloc(max_line);
-                                       }
-
-                                       len = cl->output(line, max_line);
-                               } while (len == max_line);
-                               r = safe_write(fd, line, strlen(line));
-                               if (r < 0)
-                                       return;
-                               r = safe_write(fd, "\n", 1);
-                               if (r < 0)
-                                       return;
-                       }
-               }
+       else {
+         // try to accept this character as a key
+         state = ACCEPT_KEY;
+         --l;
        }
-
-       free(line);
-}
-
-void ConfFile::dump()
-{
-       SectionList::iterator sec_iter, sec_end;
-
-       sec_end=sections_list.end();
-
-       _dump(STDOUT_FILENO);
-}
-
-ConfSection *ConfFile::_add_section(const char *section, ConfLine *cl)
-{
-       ConfSection *sec;
-#define BUF_SIZE 4096
-       char *buf;
-
-       buf = (char *)malloc(BUF_SIZE);
-
-       sec = new ConfSection(section);
-       sections[section] = sec;
-       sections_list.push_back(sec);
-
-       if (!cl) {
-               cl = new ConfLine();
-               snprintf(buf, BUF_SIZE, "[%s]", section);
-               cl->set_prefix(buf);
+       break;
+      case ACCEPT_SECTION_NAME:
+       if (c == '\0') {
+         ostringstream oss;
+         oss << "error parsing new section name: expected right bracket "
+             << "at char " << (l - line) << ", line " << line_no;
+         errors->push_back(oss.str());
+         return NULL;
        }
-       sec->conf_list.push_back(cl);
-
-       free(buf);
-
-       return sec;
-}
-
-int ConfFile::_read(int fd, char *buf, size_t size)
-{
-       if (filename)
-               return safe_read(fd, buf, size);
-
-       if (!pbl)
-               return -EINVAL;
-
-       size_t left = min(pbl->length() - buf_pos, size);
-
-       if (left) {
-               memcpy(buf, pbl->c_str() + buf_pos, left);
-               buf_pos += left;
+       else if (c == ']') {
+         trim_whitespace(newsection, true);
+         if (newsection.empty()) {
+           ostringstream oss;
+           oss << "error parsing new section name: no section name found? "
+               << "at char " << (l - line) << ", line " << line_no;
+           errors->push_back(oss.str());
+           return NULL;
+         }
+         state = ACCEPT_COMMENT_START;
        }
-
-       return left;
-}
-
-int ConfFile::_close(int fd)
-{
-       if (filename)
-               return close(fd);
-
-       if (!pbl)
-               return -EINVAL;
-
-       return 0;
-}
-
-int ConfFile::_parse(const char *filename, ConfSection **psection)
-{
-       char *buf;
-       int len, i, l;
-       char *line;
-       ConfLine *cl;
-       ConfSection *section = *psection;
-       int fd = -1;
-       int max_line = MAX_LINE;
-       int eof = 0;
-       bool ret = 0;
-
-       if (filename) {
-               fd = open(filename, O_RDONLY);
-               if (fd < 0)
-                       return -errno;
+       else if ((c == '#') || (c == ';')) {
+         ostringstream oss;
+         oss << "unexpected comment marker while parsing new section name, at "
+             << "char " << (l - line) << ", line " << line_no;
+         errors->push_back(oss.str());
+         return NULL;
+       }
+       else
+         newsection += c;
+       break;
+      case ACCEPT_KEY:
+       if ((c == '#') || (c == ';') || (c == '\0')) {
+         ostringstream oss;
+         oss << "unexpected character while parsing putative key value, "
+             << "at char " << (l - line) << ", line " << line_no;
+         errors->push_back(oss.str());
+         return NULL;
+       }
+       else if (c == '=') {
+         trim_whitespace(key, true);
+         if (key.empty()) {
+           ostringstream oss;
+           oss << "error parsing key name: no key name found? "
+               << "at char " << (l - line) << ", line " << line_no;
+           errors->push_back(oss.str());
+           return NULL;
+         }
+         state = ACCEPT_VAL_START;
+       }
+       else
+         key += c;
+       break;
+      case ACCEPT_VAL_START:
+       if (c == '\0')
+         return new ConfLine(key, val, newsection, comment, line_no);
+       else if ((c == '#') || (c == ';'))
+         state = ACCEPT_COMMENT_TEXT;
+       else if (c == '"')
+         state = ACCEPT_QUOTED_VAL;
+       else if (isspace(c)) {
+         // ignore whitespace
        }
        else {
-               if (!pbl)
-                       return -EINVAL;
-               buf_pos = 0;
+         // try to accept character as a val
+         state = ACCEPT_UNQUOTED_VAL;
+         --l;
        }
-
-       line = (char *)malloc(max_line);
-       l = 0;
-
-       buf = (char *)malloc(BUF_SIZE);
-       do {
-               len = _read(fd, buf, BUF_SIZE);
-               if (len < 0) {
-                       ret = -EDOM;
-                       goto done;
-               }
-
-               if (len < BUF_SIZE) {
-                       eof = 1;
-                       buf[len] = '\0';
-                       len++;
-               }
-
-               for (i=0; i<len; i++) {
-                       switch (buf[i]) {
-                       case '\r' :
-                               continue;
-                       case '\0' :
-                       case '\n' :
-                               if (l > 0 && line[l-1] == '\\') {
-                                       l--;
-                                       continue;
-                               }
-                               line[l] = '\0';
-                               cl = new ConfLine();
-                               parse_line(line, cl);
-                               if (cl->get_var()) {
-                                       if (strcmp(cl->get_var(), "include") == 0) {
-                                               if (!filename) { // don't allow including if we're not using files
-                                                       ret = -EDOM;
-                                                       goto done;
-                                               }
-                                               if (!_parse(cl->get_val(), &section)) {
-                                                       printf("error parsing %s\n", cl->get_val());
-                                               }
-                                       } else {
-                                               char *norm_var = cl->get_norm_var();
-                                               if (!section) {
-                                                       ret = -EDOM;
-                                                       goto done;
-                                               }
-                                               section->conf_map[norm_var] = cl;
-                                               free(norm_var);
-                                               global_list.push_back(cl);
-                                               section->conf_list.push_back(cl);
-                                       }
-                               } else if (cl->get_section()) {
-                                       section = _add_section(cl->get_section(), cl);
-                               } else {
-                                       delete cl;
-                               }
-                               l = 0;
-                               break;
-                       default:
-                               line[l++] = buf[i];
-
-                               if (l == max_line-1) {
-                                       max_line *= 2;
-                                       line = (char *)realloc(line, max_line);
-                               }
-                       }
-               }
-       } while (!eof);
-done:
-       free(buf);
-
-       *psection = section;
-       _close(fd);
-       free(line);
-
-       return ret;
-}
-
-int ConfFile::parse_file(const char *fname, std::deque<std::string> *parse_errors)
-{
-  common_init();
-
-  if (fname)
-    filename = strdup(fname);
-  else
-    filename = NULL;
-
-  return parse();
-}
-
-int ConfFile::parse_bufferlist(ceph::bufferlist *pbl_,
-                              std::deque<std::string> *parse_errors)
-{
-  common_init();
-  pbl =  pbl_;
-
-  return parse();
-}
-
-int ConfFile::parse()
-{
-       ConfSection *section = NULL;
-
-       parse_lock.Lock();
-
-       if (default_global) {
-               section = new ConfSection("global");
-               sections["global"] = section;
-               sections_list.push_back(section);
+       break;
+      case ACCEPT_UNQUOTED_VAL:
+       if (c == '\0') {
+         trim_whitespace(val, false);
+         return new ConfLine(key, val, newsection, comment, line_no);
        }
-
-       int res = _parse(filename, &section);
-       parse_lock.Unlock();
-       return res;
-}
-
-int ConfFile::flush()
-{
-       int rc;
-       int fd;
-
-       fd = open(filename, O_RDWR | O_CREAT, 0644);
-
-       if (fd < 0) {
-               printf("error opening file %s errno=%d\n", filename, errno);
-               return 0;
+       else if ((c == '#') || (c == ';')) {
+         trim_whitespace(val, false);
+         state = ACCEPT_COMMENT_TEXT;
        }
-
-       _dump(fd);
-       rc = close(fd);
-
-       if (rc < 0)
-               return 0;
-
-       return 1;
-}
-
-ConfLine *ConfFile::_find_var(const char *section, const char* var)
-{
-       SectionMap::iterator iter = sections.find(section);
-       ConfSection *sec;
-       ConfMap::iterator cm_iter;
-       ConfLine *cl;
-       char *norm_var;
-
-       if (iter == sections.end() )
-               goto notfound;
-
-       sec = iter->second;
-       norm_var = normalize_name(var);
-
-       cm_iter = sec->conf_map.find(norm_var);
-       free(norm_var);
-
-       if (cm_iter == sec->conf_map.end())
-               goto notfound;
-
-       cl = cm_iter->second;
-
-       return cl;
-notfound:
-       return 0;
-}
-
-ConfLine *ConfFile::_add_var(const char *section, const char* var)
-{
-       SectionMap::iterator iter = sections.find(section);
-       ConfSection *sec;
-       ConfMap::iterator cm_iter;
-       ConfLine *cl;
-
-       if (iter == sections.end() ) {
-               sec = _add_section(section, NULL);
-       } else {
-               sec = iter->second;
+       else
+         val += c;
+       break;
+      case ACCEPT_QUOTED_VAL:
+       if (c == '\0') {
+         ostringstream oss;
+         oss << "found opening quote for value, but not the closing quote. "
+             << "line " << line_no;
+         errors->push_back(oss.str());
+         return NULL;
        }
-
-       cl = new ConfLine();
-
-       cl->set_prefix("\t");
-       cl->set_var(var);
-       cl->set_mid(" = ");
-
-       sec->conf_map[var] = cl;
-       sec->conf_list.push_back(cl);
-
-       return cl;
-}
-
-int ConfFile::read(const char *section, const char *var, std::string &out)
-{
-       ConfLine *cl = _find_var(section, var);
-       if (!cl || !cl->get_val()) {
-               return -ENOENT;
+       else if (c == '"') {
+         state = ACCEPT_COMMENT_START;
+       }
+       else {
+         // Add anything, including whitespace.
+         val += c;
        }
-       out = cl->get_val();
-       return 0;
+       break;
+      case ACCEPT_COMMENT_START:
+       if (c == '\0') {
+         return new ConfLine(key, val, newsection, comment, line_no);
+       }
+       else if ((c == '#') || (c == ';')) {
+         state = ACCEPT_COMMENT_TEXT;
+       }
+       else if (isspace(c)) {
+         // ignore whitespace
+       }
+       else {
+         ostringstream oss;
+         oss << "unexpected character at char " << (l - line) << " of line "
+             << line_no;
+         errors->push_back(oss.str());
+         return NULL;
+       }
+       break;
+      case ACCEPT_COMMENT_TEXT:
+       if (c == '\0')
+         return new ConfLine(key, val, newsection, comment, line_no);
+       else
+         comment += c;
+       break;
+      default:
+       assert(0);
+       break;
+    }
+    assert(c != '\0'); // We better not go past the end of the input string.
+  }
 }
index 790051308b93c332128e84e8ba453bae3e92c2df..a21a3d778de4a77c860a8b5b9b551739ceb094fd 100644 (file)
+// -*- mode:C++; tab-width:8; c-basic-offset:2; indent-tabs-mode:t -*-
+// vim: ts=8 sw=2 smarttab
+/*
+ * Ceph - scalable distributed file system
+ *
+ * Copyright (C) 2011 New Dream Network
+ *
+ * This is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public
+ * License version 2.1, as published by the Free Software
+ * Foundation.  See file COPYING.
+ *
+ */
+
 #ifndef CEPH_CONFUTILS_H
 #define CEPH_CONFUTILS_H
 
-
 #include <deque>
-#include <string.h>
 #include <map>
+#include <set>
 #include <string>
-#include <list>
 
 #include "include/buffer.h"
-#include "common/Mutex.h"
 
+/*
+ * Ceph configuration file support.
+ *
+ * This class loads an INI-style configuration from a file or bufferlist, and
+ * holds it in memory. In general, an INI configuration file is composed of
+ * sections, which contain key/value pairs. You can put comments on the end of
+ * lines by using either a hash mark (#) or the semicolon (;).
+ *
+ * You can get information out of ConfFile by calling get_key or by examining
+ * individual sections.
+ *
+ * This class could be extended to support modifying configuration files and
+ * writing them back out without too much difficulty. Currently, this is not
+ * implemented, and the file is read-only.
+ */
 class ConfLine {
-       char *prefix;
-       char *var;
-       char *mid;
-       char *val;
-       char *suffix;
-       char *section;
-       char *norm_var;
-
-       void _set(char **dst, const char *val, bool alloc);
 public:
-       ConfLine() : prefix(NULL), var(NULL), mid(NULL), val(NULL),
-                  suffix(NULL), section(NULL), norm_var(NULL) {}
-       ~ConfLine();
-
-       void set_prefix(const char *val, bool alloc = true);
-       void set_var(const char *val, bool alloc = true);
-       void set_mid(const char *val, bool alloc = true);
-       void set_val(const char *val, bool alloc = true);
-       void set_suffix(const char *val, bool alloc = true);
-       void set_section(const char *val, bool alloc = true);
-
-       char *get_prefix() { return prefix; }
-       char *get_var() { return var; }
-       char *get_mid() { return mid; }
-       char *get_val() { return val; }
-       char *get_suffix() { return suffix; }
-       char *get_section() { return section; }
-       char *get_norm_var();
+  ConfLine(const std::string &key_, const std::string val_,
+          const std::string newsection_, const std::string comment_, int line_no_);
+  bool operator<(const ConfLine &rhs) const;
+  friend std::ostream &operator<<(std::ostream& oss, const ConfLine &l);
 
-       int output(char *line, int max_len);
+  std::string key, val, newsection;
 };
 
-typedef std::map<std::string, ConfLine *> ConfMap;
-typedef std::list<ConfLine *> ConfList;
-
-class ConfFile;
-
-class ConfSection
-{
-       friend class ConfFile;
-
-       std::string name;
-       ConfList conf_list;
-       ConfMap conf_map;
+class ConfSection {
 public:
-       ~ConfSection();
-       ConfSection(std::string sec_name) : name(sec_name) { }
+  typedef std::set <ConfLine>::const_iterator const_line_iter_t;
 
-       const std::string& get_name() { return name; }
-        ConfList& get_list() { return conf_list; }
+  std::set <ConfLine> lines;
 };
 
-typedef std::map<std::string, ConfSection *> SectionMap;
-typedef std::list<ConfSection *> SectionList;
-
 class ConfFile {
-       char *filename;
-
-       ceph::bufferlist *pbl;
-       size_t buf_pos;
-       Mutex parse_lock;
-       bool default_global;
-
-       void (*post_process_func)(std::string &);
-
-       SectionMap sections;
-       SectionList sections_list;
-       ConfList global_list;
-
-       ConfLine *_add_var(const char *section, const char* var);
-
-       ConfSection *_add_section(const char *section, ConfLine *cl);
-       void _dump(int fd);
-       int _parse(const char *filename, ConfSection **psection);
-       void common_init();
-
-       int _read(int fd, char *buf, size_t size);
-       int _close(int fd);
-       int parse();
 public:
-        ConfFile();
-       ~ConfFile();
-
-       const SectionList& get_section_list() { return sections_list; }
-       const char *get_filename() { return filename; }
-
-       ConfLine *_find_var(const char *section, const char* var);
-
-       int parse_file(const char *fname, std::deque<std::string> *parse_errors);
-       int parse_bufferlist(ceph::bufferlist *bl,
-                            std::deque<std::string> *parse_errors);
-
-       int read(const char *section, const char *var, std::string &val);
-
-       void dump();
-       int flush();
+  typedef std::map <std::string, ConfSection>::iterator section_iter_t;
+  typedef std::map <std::string, ConfSection>::const_iterator const_section_iter_t;
+
+  ConfFile();
+  ~ConfFile();
+  void clear();
+  int parse_file(const std::string &fname, std::deque<std::string> *errors);
+  int parse_bufferlist(ceph::bufferlist *bl, std::deque<std::string> *errors);
+  int read(const std::string &section, const std::string &key,
+             std::string &val) const;
+
+  const_section_iter_t sections_begin() const;
+  const_section_iter_t sections_end() const;
+
+  static void trim_whitespace(std::string &str, bool strip_internal);
+  friend std::ostream &operator<<(std::ostream &oss, const ConfFile &cf);
+
+private:
+  void load_from_buffer(const char *buf, size_t sz,
+                      std::deque<std::string> *errors);
+  static ConfLine* process_line(int line_no, const char *line,
+                               std::deque<std::string> *errors);
+
+  std::map <std::string, ConfSection> sections;
 };
 
 #endif
index aa896ef05006d69f41f463b479ac89df960bb24b..8f51826d01de69b141bc0bb8c68443983e0c2429 100644 (file)
@@ -564,7 +564,12 @@ parse_argv(std::vector<const char*>& args)
   std::string val;
   for (std::vector<const char*>::iterator i = args.begin(); i != args.end(); ) {
     if (ceph_argparse_flag(args, i, "--show_conf", (char*)NULL)) {
-      cf->dump();
+      if (cf) {
+       cerr << *cf << std::endl;
+      }
+      else {
+       cerr << "(no conf loaded)" << std::endl;
+      }
       _exit(0);
     }
     else if (ceph_argparse_flag(args, i, "--foreground", "-f", (char*)NULL)) {
@@ -728,11 +733,9 @@ get_all_sections(std::vector <std::string> &sections)
 {
   if (!cf)
     return -EDOM;
-  for (std::list<ConfSection*>::const_iterator p =
-           cf->get_section_list().begin();
-       p != cf->get_section_list().end(); ++p)
-  {
-    sections.push_back((*p)->get_name());
+  for (ConfFile::const_section_iter_t s = cf->sections_begin();
+       s != cf->sections_end(); ++s) {
+    sections.push_back(s->first);
   }
   return 0;
 }
index 84a0092e1665c002a54ccca814129df9be405936..513013c3118330a6f7466c380a7fe6a4ce85427e 100644 (file)
@@ -14,6 +14,7 @@
 #include "common/ConfUtils.h"
 #include "common/errno.h"
 #include "gtest/gtest.h"
+#include "include/buffer.h"
 
 #include <errno.h>
 #include <iostream>
@@ -23,6 +24,7 @@
 #include <sys/stat.h>
 #include <sys/types.h>
 
+using ceph::bufferlist;
 using std::cerr;
 using std::ostringstream;
 
@@ -107,6 +109,14 @@ static std::string next_tempfile(const char *text)
   return oss.str();
 }
 
+const char * const trivial_conf_1 = "";
+
+const char * const trivial_conf_2 = "log dir = foobar";
+
+const char * const trivial_conf_3 = "log dir = barfoo\n";
+
+const char * const trivial_conf_4 = "log dir = \"barbaz\"\n";
+
 const char * const simple_conf_1 = "\
 ; here's a comment\n\
 [global]\n\
@@ -150,6 +160,7 @@ const char * const simple_conf_2 = "\
 [osd0]\n\
         keyring   =       osd_keyring          ; osd's keyring\n\
 \n\
+           \n\
 [global]\n\
        # I like pound signs as comment markers.\n\
        ; Do you like pound signs as comment markers?\n\
@@ -196,6 +207,14 @@ const char illegal_conf3[] = "\
         keyring = osd_keyring          ; osd's keyring\n\
 ";
 
+// illegal because it has unterminated quotes
+const char illegal_conf4[] = "\
+[global]\n\
+        keyring = \"unterminated quoted string\n\
+[osd0]\n\
+        keyring = osd_keyring          ; osd's keyring\n\
+";
+
 // unicode config file
 const char unicode_config_1[] = "\
 [global]\n\
@@ -204,6 +223,74 @@ const char unicode_config_1[] = "\
 [osd0]\n\
 ";
 
+TEST(Whitespace, ConfUtils) {
+  std::string test0("");
+  ConfFile::trim_whitespace(test0, false);
+  ASSERT_EQ(test0, "");
+
+  std::string test0a("");
+  ConfFile::trim_whitespace(test0a, true);
+  ASSERT_EQ(test0a, "");
+
+  std::string test0b("          ");
+  ConfFile::trim_whitespace(test0b, false);
+  ASSERT_EQ(test0b, "");
+
+  std::string test0c("          ");
+  ConfFile::trim_whitespace(test0c, true);
+  ASSERT_EQ(test0c, "");
+
+  std::string test1(" abc             ");
+  ConfFile::trim_whitespace(test1, false);
+  ASSERT_EQ(test1, "abc");
+
+  std::string test2(" abc        d     ");
+  ConfFile::trim_whitespace(test2, true);
+  ASSERT_EQ(test2, "abc d");
+
+  std::string test3(" abc        d     ");
+  ConfFile::trim_whitespace(test3, false);
+  ASSERT_EQ(test3, "abc        d");
+
+  std::string test4("abcd");
+  ConfFile::trim_whitespace(test4, false);
+  ASSERT_EQ(test4, "abcd");
+
+  std::string test5("abcd");
+  ConfFile::trim_whitespace(test5, true);
+  ASSERT_EQ(test5, "abcd");
+}
+
+TEST(ParseFiles0, ConfUtils) {
+  std::deque<std::string> err;
+  std::string val;
+
+  std::string trivial_conf_1_f(next_tempfile(trivial_conf_1));
+  ConfFile cf1;
+  ASSERT_EQ(cf1.parse_file(trivial_conf_1_f.c_str(), &err), 0);
+  ASSERT_EQ(err.size(), 0U);
+
+  std::string trivial_conf_2_f(next_tempfile(trivial_conf_2));
+  ConfFile cf2;
+  ASSERT_EQ(cf2.parse_file(trivial_conf_2_f.c_str(), &err), 0);
+  ASSERT_EQ(err.size(), 1U);
+
+  bufferlist bl3;
+  bl3.append(trivial_conf_3, strlen(trivial_conf_3));
+  ConfFile cf3;
+  ASSERT_EQ(cf3.parse_bufferlist(&bl3, &err), 0);
+  ASSERT_EQ(err.size(), 0U);
+  ASSERT_EQ(cf3.read("global", "log dir", val), 0);
+  ASSERT_EQ(val, "barfoo");
+
+  std::string trivial_conf_4_f(next_tempfile(trivial_conf_4));
+  ConfFile cf4;
+  ASSERT_EQ(cf4.parse_file(trivial_conf_4_f.c_str(), &err), 0);
+  ASSERT_EQ(err.size(), 0U);
+  ASSERT_EQ(cf4.read("global", "log dir", val), 0);
+  ASSERT_EQ(val, "barbaz");
+}
+
 TEST(ParseFiles1, ConfUtils) {
   std::deque<std::string> err;
   std::string simple_conf_1_f(next_tempfile(simple_conf_1));
@@ -281,23 +368,27 @@ TEST(ReadFiles2, ConfUtils) {
   ASSERT_EQ(val, "\x66\xd1\x86\xd1\x9d\xd3\xad\xd3\xae");
 }
 
-// FIXME: illegal configuration files don't return a parse error currently.
 TEST(IllegalFiles, ConfUtils) {
   std::deque<std::string> err;
   std::string illegal_conf1_f(next_tempfile(illegal_conf1));
   ConfFile cf1;
   std::string val;
   ASSERT_EQ(cf1.parse_file(illegal_conf1_f.c_str(), &err), 0);
-  ASSERT_EQ(err.size(), 0U); // FIXME
+  ASSERT_EQ(err.size(), 1U);
 
   bufferlist bl2;
   bl2.append(illegal_conf2, strlen(illegal_conf2));
   ConfFile cf2;
   ASSERT_EQ(cf2.parse_bufferlist(&bl2, &err), 0);
-  ASSERT_EQ(err.size(), 0U); // FIXME
+  ASSERT_EQ(err.size(), 1U);
 
   std::string illegal_conf3_f(next_tempfile(illegal_conf3));
   ConfFile cf3;
   ASSERT_EQ(cf3.parse_file(illegal_conf3_f.c_str(), &err), 0);
-  ASSERT_EQ(err.size(), 0U); // FIXME
+  ASSERT_EQ(err.size(), 1U);
+
+  std::string illegal_conf4_f(next_tempfile(illegal_conf4));
+  ConfFile cf4;
+  ASSERT_EQ(cf4.parse_file(illegal_conf4_f.c_str(), &err), 0);
+  ASSERT_EQ(err.size(), 1U);
 }