From 93c38a764d73fad3fd3ac3a61dffce32e344d6ab Mon Sep 17 00:00:00 2001
From: Andrew Bettison <andrewb@zip.com.au>
Date: Tue, 4 Dec 2012 18:20:53 +1030
Subject: [PATCH] Improve servald 'config' 'set' and 'del' commands

Return exit status 2 if the new config is invalid in any way.
---
 commandline.c | 12 ++++++--
 conf.c        |  2 ++
 tests/config  | 81 ++++++++++++++++++++-------------------------------
 3 files changed, 44 insertions(+), 51 deletions(-)

diff --git a/commandline.c b/commandline.c
index ce351c7f..6a2e9dba 100644
--- a/commandline.c
+++ b/commandline.c
@@ -948,7 +948,11 @@ int app_config_set(int argc, const char *const *argv, const struct command_line_
   if (cf_om_reload() == -1)
     return -1;
   // </kludge>
-  return cf_om_set(&cf_om_root, var, val) == -1 ? -1 : cf_om_save();
+  if (cf_om_set(&cf_om_root, var, val) == -1 || cf_om_save() == -1)
+    return -1;
+  if (cf_reload() == -1) // logs an error if the new config is bad
+    return 2;
+  return 0;
 }
 
 int app_config_del(int argc, const char *const *argv, const struct command_line_option *o, void *context)
@@ -963,7 +967,11 @@ int app_config_del(int argc, const char *const *argv, const struct command_line_
   if (cf_om_reload() == -1)
     return -1;
   // </kludge>
-  return cf_om_set(&cf_om_root, var, NULL) == -1 ? -1 : cf_om_save();
+  if (cf_om_set(&cf_om_root, var, NULL) == -1 || cf_om_save() == -1)
+    return -1;
+  if (cf_reload() == -1) // logs an error if the new config is bad
+    return 2;
+  return 0;
 }
 
 int app_config_get(int argc, const char *const *argv, const struct command_line_option *o, void *context)
diff --git a/conf.c b/conf.c
index 7402be14..bfd04f33 100644
--- a/conf.c
+++ b/conf.c
@@ -74,6 +74,8 @@ static int load()
   else if (meta.size > CONFIG_FILE_MAX_SIZE) {
     WHYF("config file %s is too big (%ld bytes exceeds limit %ld)", path, meta.size, CONFIG_FILE_MAX_SIZE);
     return CFERROR;
+  } else if (meta.size <= 0) {
+    INFOF("config file %s is zero size", path);
   } else {
     FILE *f = fopen(path, "r");
     if (f == NULL) {
diff --git a/tests/config b/tests/config
index a96e3f3e..543d6149 100755
--- a/tests/config
+++ b/tests/config
@@ -41,100 +41,83 @@ setup_SetCreateInstanceDir() {
    assert ! [ -d "$SERVALINSTANCE_PATH" ]
 }
 test_SetCreateInstanceDir() {
-   executeOk_servald config set foo bar
+   executeOk_servald config set debug.verbose 0
    assert [ -d "$SERVALINSTANCE_PATH" ]
 }
 
 doc_GetNull="Get an unset config item"
 test_GetNull() {
-   executeOk_servald config get foo
+   executeOk_servald config get debug.verbose
    assertStdoutLineCount '==' 0
 }
 
 doc_SetGet="Set and get a single config item"
 test_SetGet() {
-   executeOk_servald config set foo bar
-   executeOk_servald config get foo
+   executeOk_servald config set debug.verbose yes
+   executeOk_servald config get debug.verbose
    assertStdoutLineCount '==' 1
-   assertStdoutGrep --stdout --stderr --matches=1 '^foo=bar$'
+   assertStdoutGrep --stdout --stderr --matches=1 '^debug\.verbose=yes$'
 }
 
 doc_GetAll="Get all config items"
 test_GetAll() {
-   executeOk_servald config set foo bar
-   executeOk_servald config set hello world
+   executeOk_servald config set debug.verbose true
+   executeOk_servald config set log.show_pid true
+   executeOk_servald config set server.chdir /tmp/nothing
+   executeOk_servald config set rhizome.enable no
    executeOk_servald config get
-   assertStdoutLineCount '==' 2
-   assertStdoutGrep --stdout --matches=1 '^foo=bar$'
-   assertStdoutGrep --stdout --matches=1 '^hello=world$'
+   assertStdoutLineCount '==' 4
+   assertStdoutGrep --stdout --matches=1 '^debug\.verbose=true$'
+   assertStdoutGrep --stdout --matches=1 '^log\.show_pid=true$'
+   assertStdoutGrep --stdout --matches=1 '^server\.chdir=/tmp/nothing$'
+   assertStdoutGrep --stdout --matches=1 '^rhizome\.enable=no$'
 }
 
 doc_SetTwice="Set a single config item twice"
 test_SetTwice() {
-   executeOk_servald config set foo bar
-   executeOk_servald config get foo
+   executeOk_servald config set debug.verbose yes
+   executeOk_servald config get debug.verbose
    assertStdoutLineCount '==' 1
-   assertStdoutGrep --stdout --stderr --matches=1 '^foo=bar$'
-   executeOk_servald config set foo wah
-   executeOk_servald config get foo
+   assertStdoutGrep --stdout --stderr --matches=1 '^debug\.verbose=yes$'
+   executeOk_servald config set debug.verbose false
+   executeOk_servald config get debug.verbose
    assertStdoutLineCount '==' 1
-   assertStdoutGrep --stdout --stderr --matches=1 '^foo=wah$'
+   assertStdoutGrep --stdout --stderr --matches=1 '^debug\.verbose=false$'
 }
 
 doc_DelNull="Delete an unset config item"
 test_DelNull() {
-   executeOk_servald config del foo
+   executeOk_servald config del debug.verbose
    assertStdoutLineCount '==' 0
 }
 
 doc_Del="Delete single config item"
 test_Del() {
-   executeOk_servald config set foo bar
-   executeOk_servald config set hello world
+   executeOk_servald config set debug.verbose yes
+   executeOk_servald config set log.show_pid true
    executeOk_servald config get
    assertStdoutLineCount '==' 2
-   executeOk_servald config del foo
+   executeOk_servald config del debug.verbose
    executeOk_servald config get
    assertStdoutLineCount '==' 1
-   executeOk_servald config get foo
+   executeOk_servald config del log.show_pid
    assertStdoutLineCount '==' 0
 }
 
 doc_CaseSensitive="Config item names are case sensitive"
 test_CaseSensitive() {
-   executeOk_servald config set foo bar
-   executeOk_servald config set Foo xxx
-   executeOk_servald config get foo
-   assertStdoutLineCount '==' 1
-   assertStdoutGrep --stdout --stderr --matches=1 '^foo=bar$'
-   executeOk_servald config get Foo
-   assertStdoutLineCount '==' 1
-   assertStdoutGrep --stdout --stderr --matches=1 '^Foo=xxx$'
-   executeOk_servald config set FOO wah
-   executeOk_servald config get foo
-   assertStdoutLineCount '==' 1
-   assertStdoutGrep --stdout --stderr --matches=1 '^foo=bar$'
-   executeOk_servald config get FOO
-   assertStdoutLineCount '==' 1
-   assertStdoutGrep --stdout --stderr --matches=1 '^FOO=wah$'
+   execute $servald config set Debug.verbose yes
+   assertExitStatus --stderr '!=' 0
 }
 
-doc_DotsInNames="Config item names can have internal dots"
-test_DotsInNames() {
-   executeOk_servald config set foo.bar yes
-   executeOk_servald config get foo.bar
-   assertStdoutLineCount '==' 1
-   assertStdoutGrep --stdout --stderr --matches=1 '^foo\.bar=yes$'
-   execute $servald config set foo. yes
+doc_OptionNames="Config item names must be well formed"
+test_OptionNames() {
+   execute $servald config set debug. yes
    assertExitStatus --stderr '!=' 0
-   execute $servald config set .foo yes
+   execute $servald config set .verbose yes
    assertExitStatus --stderr '!=' 0
-   execute $servald config set foo..bar yes
+   execute $servald config set debug..verbose yes
    assertExitStatus --stderr '!=' 0
-   executeOk_servald config set foo.x.bar yes
-   executeOk_servald config get foo.x.bar
-   assertStdoutLineCount --stderr '==' 1
-   assertStdoutGrep --stdout --stderr --matches=1 '^foo\.x\.bar=yes$'
 }
 
 doc_DebugFlags="Debug config options affect verbosity"