conf: fix reloading modified sources specified by IP address

When reloading a modified source from sourcedir which is ordered before
the original source (e.g. maxpoll was decreased), the new source is
added before the original one is removed. If the source is specified by
IP address, the addition fails due to the conflict with the original
source. Sources specified by hostname don't conflict. They are resolved
later (repeatedly if the resolver provides only conflicting addresses).

Split the processing of sorted source lists into two phases, so all
modified sources are removed before they are added again to avoid the
conflict.

Reported-by: Thomas Lange <thomas@corelatus.se>

diff --git a/conf.c b/conf.c
index f98406098b5b261d8b219fa80a8a4b9d560c1184..4c1d2e904781bd1652d363699c3bfb0bd0b860d1 100644 (file)
--- a/conf.c
+++ b/conf.c
@@ -1679,7 +1679,7 @@ reload_source_dirs(void)
   uint32_t *prev_ids, *new_ids;
   char buf[MAX_LINE_LENGTH];
   NSR_Status s;
-  int d;
+  int d, pass;
 
   prev_size = ARR_GetSize(ntp_source_ids);
   if (prev_size > 0 && ARR_GetSize(ntp_sources) != prev_size)
@@ -1713,36 +1713,37 @@ reload_source_dirs(void)
 
   qsort(new_sources, new_size, sizeof (new_sources[0]), compare_sources);
 
-  for (i = j = 0; i < prev_size || j < new_size; ) {
-    if (i < prev_size && j < new_size)
-      d = compare_sources(&prev_sources[i], &new_sources[j]);
-    else
-      d = i < prev_size ? -1 : 1;
+  for (pass = 0; pass < 2; pass++) {
+    for (i = j = 0; i < prev_size || j < new_size; i += d <= 0, j += d >= 0) {
+      if (i < prev_size && j < new_size)
+        d = compare_sources(&prev_sources[i], &new_sources[j]);
+      else
+        d = i < prev_size ? -1 : 1;
 
-    if (d < 0) {
-      /* Remove the missing source */
-      if (prev_sources[i].params.name[0] != '\0')
+      /* Remove missing sources before adding others to avoid conflicts */
+      if (pass == 0 && d < 0 && prev_sources[i].params.name[0] != '\0') {
         NSR_RemoveSourcesById(prev_ids[i]);
-      i++;
-    } else if (d > 0) {
-      /* Add a newly configured source */
-      source = &new_sources[j];
-      s = NSR_AddSourceByName(source->params.name, source->params.port, source->pool,
-                              source->type, &source->params.params, &new_ids[j]);
-
-      if (s == NSR_UnresolvedName) {
-        unresolved++;
-      } else if (s != NSR_Success) {
-        LOG(LOGS_ERR, "Could not add source %s", source->params.name);
-
-        /* Mark the source as not present */
-        source->params.name[0] = '\0';
       }
-      j++;
-    } else {
-      /* Keep the existing source */
-      new_ids[j] = prev_ids[i];
-      i++, j++;
+
+      /* Add new sources */
+      if (pass == 1 && d > 0) {
+        source = &new_sources[j];
+        s = NSR_AddSourceByName(source->params.name, source->params.port, source->pool,
+                                source->type, &source->params.params, &new_ids[j]);
+
+        if (s == NSR_UnresolvedName) {
+          unresolved++;
+        } else if (s != NSR_Success) {
+          LOG(LOGS_ERR, "Could not add source %s", source->params.name);
+
+          /* Mark the source as not present */
+          source->params.name[0] = '\0';
+        }
+      }
+
+      /* Keep unchanged sources */
+      if (pass == 1 && d == 0)
+        new_ids[j] = prev_ids[i];
     }
   }
 

diff --git a/test/system/008-confload b/test/system/008-confload
index a7fc1d52ace99bce4896330767569f7883ba47f2..7e806988b71eebf2d899115c6068631e4d7475c1 100755 (executable)
--- a/test/system/008-confload
+++ b/test/system/008-confload
@@ -30,6 +30,8 @@ echo "server 127.123.4.4" > $TEST_DIR/conf4.d/4.conf
 echo "server 127.123.5.1" > $TEST_DIR/conf5.d/1.sources
 echo "server 127.123.5.2" > $TEST_DIR/conf5.d/2.sources
 echo "server 127.123.5.3" > $TEST_DIR/conf5.d/3.sources
+echo "server 127.123.5.4" > $TEST_DIR/conf5.d/4.sources
+echo "server 127.123.5.5" > $TEST_DIR/conf5.d/5.sources
 
 start_chronyd || test_fail
 
@@ -46,12 +48,16 @@ check_chronyc_output "^[^=]*
 .. 127\.123\.1\.2 [^^]*
 .. 127\.123\.5\.1 [^^]*
 .. 127\.123\.5\.2 [^^]*
-.. 127\.123\.5\.3 [^^]*$" || test_fail
+.. 127\.123\.5\.3 [^^]*
+.. 127\.123\.5\.4 [^^]*
+.. 127\.123\.5\.5 [^^]*$" || test_fail
 
 rm $TEST_DIR/conf5.d/1.sources
-echo "server 127.123.5.2 minpoll 7" > $TEST_DIR/conf5.d/2.sources
-echo > $TEST_DIR/conf5.d/3.sources
-echo "server 127.123.5.4" > $TEST_DIR/conf5.d/4.sources
+echo "server 127.123.5.2 minpoll 5" > $TEST_DIR/conf5.d/2.sources
+echo "server 127.123.5.3 minpoll 7" > $TEST_DIR/conf5.d/3.sources
+echo > $TEST_DIR/conf5.d/4.sources
+echo "server 127.123.5.5" >> $TEST_DIR/conf5.d/5.sources
+echo "server 127.123.5.6" > $TEST_DIR/conf5.d/6.sources
 
 run_chronyc "reload sources" || test_fail
 
@@ -66,9 +72,12 @@ check_chronyc_output "^[^=]*
 .. 127\.123\.2\.3 [^^]*
 .. 127\.123\.4\.4 [^^]*
 .. 127\.123\.1\.2 *[05]   6 [^^]*
-.. 127\.123\.5\.2 *[05]   7 [^^]*
-.. 127\.123\.5\.4 [^^]*$" || test_fail
+.. 127\.123\.5\.5 [^^]*
+.. 127\.123\.5\.2 *[05]   5 [^^]*
+.. 127\.123\.5\.3 *[05]   7 [^^]*
+.. 127\.123\.5\.6 [^^]*$" || test_fail
 
 stop_chronyd || test_fail
+check_chronyd_message_count "Could not add source" 1 1 || test_fail
 
 test_pass