ctdb-common: Correctly handle conf->reload()

Configuration reload should reset the values of configuration options
missing from the config file to default.

Signed-off-by: Amitay Isaacs <amitay@gmail.com>
Reviewed-by: Martin Schwenke <martin@meltin.net>
Autobuild-User(master): Martin Schwenke <martins@samba.org>
Autobuild-Date(master): Fri Jun 29 15:12:37 CEST 2018 on sn-devel-144

diff --git a/ctdb/common/conf.c b/ctdb/common/conf.c
index dccf6613f48729877b23b1f3056b10139e7fe6de..3c1369e138d270c3f4a957c2fd61f756c23086d8 100644 (file)
--- a/ctdb/common/conf.c
+++ b/ctdb/common/conf.c
@@ -161,6 +161,44 @@ static int conf_value_from_string(TALLOC_CTX *mem_ctx,
        return ret;
 }
 
+static bool conf_value_compare(struct conf_value *old, struct conf_value *new)
+{
+       if (old == NULL || new == NULL) {
+               return false;
+       }
+
+       if (old->type != new->type) {
+               return false;
+       }
+
+       switch (old->type) {
+       case CONF_STRING:
+               if (old->data.string == NULL && new->data.string == NULL) {
+                       return true;
+               }
+               if (old->data.string != NULL && new->data.string != NULL) {
+                       if (strcmp(old->data.string, new->data.string) == 0) {
+                               return true;
+                       }
+               }
+               break;
+
+       case CONF_INTEGER:
+               if (old->data.integer == new->data.integer) {
+                       return true;
+               }
+               break;
+
+       case CONF_BOOLEAN:
+               if (old->data.boolean == new->data.boolean) {
+                       return true;
+               }
+               break;
+       }
+
+       return false;
+}
+
 static int conf_value_copy(TALLOC_CTX *mem_ctx,
                           struct conf_value *src,
                           struct conf_value *dst)
@@ -409,6 +447,12 @@ static bool conf_option_validate(struct conf_option *opt,
        return ret;
 }
 
+static bool conf_option_same_value(struct conf_option *opt,
+                                  struct conf_value *new_value)
+{
+       return conf_value_compare(opt->value, new_value);
+}
+
 static int conf_option_new_value(struct conf_option *opt,
                                 struct conf_value *new_value,
                                 enum conf_update_mode mode)
@@ -416,36 +460,56 @@ static int conf_option_new_value(struct conf_option *opt,
        int ret;
        bool ok;
 
-       ok = conf_option_validate(opt, new_value, mode);
-       if (!ok) {
-               D_ERR("conf: validation for option \"%s\" failed\n",
-                     opt->name);
-               return EINVAL;
+       if (opt->new_value != &opt->default_value) {
+               TALLOC_FREE(opt->new_value);
        }
 
-       TALLOC_FREE(opt->new_value);
-       opt->new_value = talloc_zero(opt, struct conf_value);
-       if (opt->new_value == NULL) {
-               return ENOMEM;
-       }
+       if (new_value == &opt->default_value) {
+               /*
+                * This happens only during load/reload. Set the value to
+                * default value, so if the config option is dropped from
+                * config file, then it get's reset to default.
+                */
+               opt->new_value = &opt->default_value;
+       } else {
+               ok = conf_option_validate(opt, new_value, mode);
+               if (!ok) {
+                       D_ERR("conf: validation for option \"%s\" failed\n",
+                             opt->name);
+                       return EINVAL;
+               }
 
-       opt->new_value->type = opt->value->type;
-       ret = conf_value_copy(opt, new_value, opt->new_value);
-       if (ret != 0) {
-               return ret;
+               opt->new_value = talloc_zero(opt, struct conf_value);
+               if (opt->new_value == NULL) {
+                       return ENOMEM;
+               }
+
+               opt->new_value->type = opt->value->type;
+               ret = conf_value_copy(opt, new_value, opt->new_value);
+               if (ret != 0) {
+                       return ret;
+               }
        }
 
        conf_option_set_ptr_value(opt);
 
-       if (mode == CONF_MODE_API) {
-               opt->temporary_modified = true;
-       } else {
-               opt->temporary_modified = false;
+       if (new_value != &opt->default_value) {
+               if (mode == CONF_MODE_API) {
+                       opt->temporary_modified = true;
+               } else {
+                       opt->temporary_modified = false;
+               }
        }
 
        return 0;
 }
 
+static int conf_option_new_default_value(struct conf_option *opt,
+                                        enum conf_update_mode mode)
+{
+       return conf_option_new_value(opt, &opt->default_value, mode);
+}
+
 static void conf_option_default(struct conf_option *opt)
 {
        if (! opt->default_set) {
@@ -462,7 +526,9 @@ static void conf_option_default(struct conf_option *opt)
 
 static void conf_option_reset(struct conf_option *opt)
 {
-       TALLOC_FREE(opt->new_value);
+       if (opt->new_value != &opt->default_value) {
+               TALLOC_FREE(opt->new_value);
+       }
 
        conf_option_set_ptr_value(opt);
 }
@@ -483,6 +549,11 @@ static void conf_option_update(struct conf_option *opt)
        conf_option_set_ptr_value(opt);
 }
 
+static void conf_option_reset_temporary(struct conf_option *opt)
+{
+       opt->temporary_modified = false;
+}
+
 static bool conf_option_is_default(struct conf_option *opt)
 {
        return (opt->value == &opt->default_value);
@@ -586,6 +657,25 @@ static void conf_all_default(struct conf_context *conf)
        }
 }
 
+static int conf_all_temporary_default(struct conf_context *conf,
+                                     enum conf_update_mode mode)
+{
+       struct conf_section *s;
+       struct conf_option *opt;
+       int ret;
+
+       for (s = conf->section; s != NULL; s = s->next) {
+               for (opt = s->option; opt != NULL; opt = opt->next) {
+                       ret = conf_option_new_default_value(opt, mode);
+                       if (ret != 0) {
+                               return ret;
+                       }
+               }
+       }
+
+       return 0;
+}
+
 static void conf_all_reset(struct conf_context *conf)
 {
        struct conf_section *s;
@@ -606,6 +696,7 @@ static void conf_all_update(struct conf_context *conf)
        for (s = conf->section; s != NULL; s = s->next) {
                for (opt = s->option; opt != NULL; opt = opt->next) {
                        conf_option_update(opt);
+                       conf_option_reset_temporary(opt);
                }
        }
 }
@@ -920,6 +1011,7 @@ static int conf_load_internal(struct conf_context *conf)
 {
        struct conf_load_state state;
        FILE *fp;
+       int ret;
        bool ok;
 
        fp = fopen(conf->filename, "r");
@@ -932,6 +1024,11 @@ static int conf_load_internal(struct conf_context *conf)
                .mode = (conf->reload ? CONF_MODE_RELOAD : CONF_MODE_LOAD),
        };
 
+       ret = conf_all_temporary_default(conf, state.mode);
+       if (ret != 0) {
+               return ret;
+       }
+
        ok = tini_parse(fp,
                        false,
                        conf_load_section,
@@ -998,6 +1095,7 @@ static bool conf_load_option(const char *name,
        TALLOC_CTX *tmp_ctx;
        struct conf_value value;
        int ret;
+       bool ok;
 
        if (state->s == NULL) {
                if (state->conf->ignore_unknown) {
@@ -1035,6 +1133,11 @@ static bool conf_load_option(const char *name,
                return false;
        }
 
+       ok = conf_option_same_value(opt, &value);
+       if (ok) {
+               goto done;
+       }
+
        ret = conf_option_new_value(opt, &value, state->mode);
        if (ret != 0) {
                talloc_free(tmp_ctx);
@@ -1042,6 +1145,7 @@ static bool conf_load_option(const char *name,
                return false;
        }
 
+done:
        talloc_free(tmp_ctx);
        return true;
 
@@ -1104,6 +1208,11 @@ static int conf_set(struct conf_context *conf,
                return ENOENT;
        }
 
+       ok = conf_option_same_value(opt, value);
+       if (ok) {
+               return 0;
+       }
+
        ret = conf_option_new_value(opt, value, CONF_MODE_API);
        if (ret != 0) {
                conf_option_reset(opt);

diff --git a/ctdb/tests/cunit/conf_test_001.sh b/ctdb/tests/cunit/conf_test_001.sh
index e83e04c78b7fad6815e08ecffe64f8fc9d8eda18..08a51b00d9b30723d14c1754eb87231d1ef11629 100755 (executable)
--- a/ctdb/tests/cunit/conf_test_001.sh
+++ b/ctdb/tests/cunit/conf_test_001.sh
@@ -32,7 +32,12 @@ unit_test conf_test 4
 ok_null
 unit_test conf_test 5
 
-ok_null
+ok <<EOF
+[section1]
+       key1 = foobar # temporary
+       key2 = 20 # temporary
+       key3 = false # temporary
+EOF
 unit_test conf_test 6
 
 ok <<EOF
@@ -77,7 +82,7 @@ ok <<EOF
 [section1]
        key1 = value2
        key2 = 20
-       key3 = false
+       # key3 = true
 EOF
 unit_test conf_test 9 "$conffile"
 
@@ -90,7 +95,7 @@ ok <<EOF
 [section1]
        key1 = value2
        # key2 = 10
-       key3 = false # temporary
+       # key3 = true
 EOF
 unit_test conf_test 9 "$conffile"
 
@@ -122,3 +127,40 @@ required_result 2 <<EOF
        key3 = false # temporary
 EOF
 unit_test conf_test 10 "$conffile"
+
+cat > "$conffile" <<EOF
+[section1]
+    key1 = value2
+    key2 = 20
+    key3 = false
+EOF
+
+touch "${conffile}.reload"
+
+ok <<EOF
+[section1]
+       # key1 = value1
+       # key2 = 10
+       # key3 = true
+EOF
+unit_test conf_test 11 "$conffile"
+
+cat > "$conffile" <<EOF
+[section1]
+    key1 = value2
+    key2 = 20
+    key3 = false
+EOF
+
+cat > "${conffile}.reload" <<EOF
+[section1]
+    key1 = value3
+EOF
+
+ok <<EOF
+[section1]
+       key1 = value3
+       # key2 = 10
+       # key3 = true
+EOF
+unit_test conf_test 11 "$conffile"

diff --git a/ctdb/tests/src/conf_test.c b/ctdb/tests/src/conf_test.c
index a1bca6910fab57c698f504a7c1e3d335eed95c88..b727cf34406e10dc68937f82ffc86d3a48635eca 100644 (file)
--- a/ctdb/tests/src/conf_test.c
+++ b/ctdb/tests/src/conf_test.c
@@ -248,6 +248,8 @@ static void test6(void)
        assert(b2_val == false);
        assert(is_default == false);
 
+       conf_dump(conf, stdout);
+
        conf_set_defaults(conf);
 
        assert(strcmp(s_val, "default") == 0);
@@ -310,12 +312,21 @@ static void test7(void)
        status = conf_valid(conf);
        assert(status == true);
 
+       ret = conf_set_string(conf, "section1", "key1", "default");
+       assert(ret == 0);
+
        ret = conf_set_string(conf, "section1", "key1", "foobar");
        assert(ret == EINVAL);
 
+       ret = conf_set_integer(conf, "section1", "key2", 10);
+       assert(ret == 0);
+
        ret = conf_set_integer(conf, "section1", "key2", 20);
        assert(ret == EINVAL);
 
+       ret = conf_set_boolean(conf, "section1", "key3", true);
+       assert(ret == 0);
+
        ret = conf_set_boolean(conf, "section1", "key3", false);
        assert(ret == EINVAL);
 
@@ -398,6 +409,45 @@ static void test9(const char *filename, bool ignore_unknown)
        exit(ret);
 }
 
+static void test11(const char *filename)
+{
+       TALLOC_CTX *mem_ctx = talloc_new(NULL);
+       char reload[PATH_MAX];
+       struct conf_context *conf;
+       int ret;
+       bool status;
+
+       ret = snprintf(reload, sizeof(reload), "%s.reload", filename);
+       assert(ret < sizeof(reload));
+
+       ret = conf_init(mem_ctx, &conf);
+       assert(ret == 0);
+       assert(conf != NULL);
+
+       conf_define_section(conf, "section1", NULL);
+
+       conf_define_string(conf, "section1", "key1", "value1", NULL);
+       conf_define_integer(conf, "section1", "key2", 10, NULL);
+       conf_define_boolean(conf, "section1", "key3", true, NULL);
+
+       status = conf_valid(conf);
+       assert(status == true);
+
+       ret = conf_load(conf, filename, false);
+       assert(ret == 0);
+
+       ret = rename(reload, filename);
+       assert(ret == 0);
+
+       ret = conf_reload(conf);
+       assert(ret == 0);
+
+       conf_dump(conf, stdout);
+
+       talloc_free(mem_ctx);
+       exit(ret);
+}
+
 int main(int argc, const char **argv)
 {
        int num;
@@ -454,6 +504,9 @@ int main(int argc, const char **argv)
                test9(argv[2], false);
                break;
 
+       case 11:
+               test11(argv[2]);
+               break;
        }
 
        return 0;