timesync: fix PropertiesChanges signals for NTP properties

As in their current form they didn't work at all:

systemd-timesyncd[190115]: Assertion 's' failed at src/libsystemd/sd-event/sd-event.c:3058, function sd_event_source_set_enabled(). Ignoring.
systemd-timesyncd[190115]: Failed to reenable system ntp server change event source!
systemd-timesyncd[190115]: Failed to enable ntp server defer event, ignoring: Invalid argument

This was also pointed out in the post-merge review [0].

Let's address this together with the rest of the comments, and add
some tests to make sure everything works as it should.

Resolves: #28770
Follow-up to: 8f1c446

[0] https://github.com/systemd/systemd/commit/8f1c4469793f2f0281fdfbc20ba4085e20cdd16f#r124147466

diff --git a/src/timesync/timesyncd-manager.c b/src/timesync/timesyncd-manager.c
index 21825f1165cbbc50fd5db25fb4acf2d1867bd63e..82fe668bf9cd47bd62d0f23e2ee184f596fc97cd 100644 (file)
--- a/src/timesync/timesyncd-manager.c
+++ b/src/timesync/timesyncd-manager.c
@@ -1227,8 +1227,8 @@ static int manager_save_time_and_rearm(Manager *m, usec_t t) {
 static const char* ntp_server_property_name[_SERVER_TYPE_MAX] = {
         [SERVER_SYSTEM]   = "SystemNTPServers",
         [SERVER_FALLBACK] = "FallbackNTPServers",
-        [SERVER_LINK] = "LinkNTPServers",
-        [SERVER_RUNTIME] = "RuntimeNTPServers",
+        [SERVER_LINK]     = "LinkNTPServers",
+        [SERVER_RUNTIME]  = "RuntimeNTPServers",
 };
 
 static int ntp_server_emit_changed_strv(Manager *manager, char **properties) {

diff --git a/src/timesync/timesyncd-server.c b/src/timesync/timesyncd-server.c
index fc9bca8c5a854fafb1edcbd3c3afaa9fa0212d72..0f68203e031de14ef3b524ffecea6cc64b9c19e9 100644 (file)
--- a/src/timesync/timesyncd-server.c
+++ b/src/timesync/timesyncd-server.c
@@ -66,16 +66,12 @@ static int enable_ntp_server_defer_event(Manager *m, ServerType type) {
         assert(m);
         assert((type >= 0) && (type < _SERVER_TYPE_MAX));
 
-        r = sd_event_source_set_enabled(m->deferred_ntp_server_event_source, SD_EVENT_ONESHOT);
-        if (r < 0)
-                return log_debug_errno(r, "Failed to reenable system ntp server change event source!");
+        m->ntp_server_change_mask |= 1U << type;
 
         r = bus_manager_emit_ntp_server_changed(m);
         if (r < 0)
                 return r;
 
-        m->ntp_server_change_mask |= 1U << type;
-
         return 1;
 }
 

diff --git a/test/units/testsuite-45.sh b/test/units/testsuite-45.sh
index d6a5f1da42aafcdaffa34ae1e6b07a76b192a494..d93e33f22c0a06f986877f727f9842da85ff23f9 100755 (executable)
--- a/test/units/testsuite-45.sh
+++ b/test/units/testsuite-45.sh
@@ -281,6 +281,117 @@ EOF
     assert_rc 3 systemctl is-active --quiet systemd-timesyncd
 
     systemctl stop busctl-monitor.service
+    rm -rf /run/systemd/system/systemd-timesyncd.service.d/
+    systemctl daemon-reload
+}
+
+assert_timesyncd_signal() {
+    local timestamp="${1:?}"
+    local property="${2:?}"
+    local value="${3:?}"
+    local args=(-q --since="$timestamp" -p info _SYSTEMD_UNIT="busctl-monitor.service")
+
+    journalctl --sync
+
+    for _ in {0..9}; do
+        if journalctl "${args[@]}" --grep .; then
+            [[ "$(journalctl "${args[@]}" -o cat | tr -d '\n' | jq -r ".payload.data[1].$property.data | join(\" \")")" == "$value" ]];
+            return 0
+        fi
+
+        sleep .5
+    done
+
+    return 1
+}
+
+assert_networkd_ntp() {
+    local interface="${1:?}"
+    local value="${2:?}"
+    # Go through the array of NTP servers and for each entry do:
+    #   - if the entry is an IPv4 address, join the Address array into a dot separated string
+    #   - if the entry is a server address, select it unchanged
+    # These steps produce an array of strings, that is then joined into a space-separated string
+    # Note: this doesn't support IPv6 addresses, since converting them to a string is a bit more
+    # involved than a simple join(), but let's leave that to another time
+    local expr='[.NTP[] | (select(.Family == 2).Address | join(".")), select(has("Server")).Server] | join(" ")'
+
+    [[ "$(networkctl status "$interface" --json=short | jq -r "$expr")" == "$value" ]]
+}
+
+testcase_timesyncd() {
+    if systemd-detect-virt -cq; then
+        echo "This test case requires a VM, skipping..."
+        return 0
+    fi
+
+    if ! command -v networkctl >/dev/null; then
+        echo "This test requires systemd-networkd, skipping..."
+        return 0
+    fi
+
+    # Create a dummy interface managed by networkd, so we can configure link NTP servers
+    mkdir -p /run/systemd/network/
+    cat >/etc/systemd/network/ntp99.netdev <<EOF
+[NetDev]
+Name=ntp99
+Kind=dummy
+EOF
+    cat >/etc/systemd/network/ntp99.network <<EOF
+[Match]
+Name=ntp99
+
+[Network]
+Address=10.0.0.1/24
+EOF
+
+    systemctl unmask systemd-timesyncd systemd-networkd
+    systemctl restart systemd-timesyncd
+    systemctl restart systemd-networkd
+    networkctl status ntp99
+
+    systemd-run --unit busctl-monitor.service --service-type=exec \
+        busctl monitor --json=short --match="type='signal',sender=org.freedesktop.timesync1,member='PropertiesChanged',path=/org/freedesktop/timesync1"
+
+    # LinkNTPServers
+    #
+    # Single IP
+    ts="$(date +"%F %T.%6N")"
+    timedatectl ntp-servers ntp99 10.0.0.1
+    assert_networkd_ntp ntp99 10.0.0.1
+    assert_timesyncd_signal "$ts" LinkNTPServers 10.0.0.1
+    # Setting NTP servers to the same value shouldn't emit a PropertiesChanged signal
+    ts="$(date +"%F %T.%6N")"
+    timedatectl ntp-servers ntp99 10.0.0.1
+    assert_networkd_ntp ntp99 10.0.0.1
+    (! assert_timesyncd_signal "$ts" LinkNTPServers 10.0.0.1)
+    # Multiple IPs
+    ts="$(date +"%F %T.%6N")"
+    timedatectl ntp-servers ntp99 10.0.0.1 192.168.0.99
+    assert_networkd_ntp ntp99 "10.0.0.1 192.168.0.99"
+    assert_timesyncd_signal "$ts" LinkNTPServers "10.0.0.1 192.168.0.99"
+    # Multiple IPs + servers
+    ts="$(date +"%F %T.%6N")"
+    timedatectl ntp-servers ntp99 10.0.0.1 192.168.0.99 foo.localhost foo 10.11.12.13
+    assert_networkd_ntp ntp99 "10.0.0.1 192.168.0.99 foo.localhost foo 10.11.12.13"
+    assert_timesyncd_signal "$ts" LinkNTPServers "10.0.0.1 192.168.0.99 foo.localhost foo 10.11.12.13"
+
+    # RuntimeNTPServers
+    #
+    # There's no user-facing API that allows changing this propery (afaik), so let's
+    # call SetRuntimeNTPServers() directly to test things out. The inner workings should
+    # be exactly the same as in the previous case, so do just one test to make sure
+    # things work
+    ts="$(date +"%F %T.%6N")"
+    busctl call org.freedesktop.timesync1 /org/freedesktop/timesync1 org.freedesktop.timesync1.Manager \
+        SetRuntimeNTPServers as 4 "10.0.0.1" foo "192.168.99.1" bar
+    servers="$(busctl get-property org.freedesktop.timesync1 /org/freedesktop/timesync1 org.freedesktop.timesync1.Manager RuntimeNTPServers)"
+    [[ "$servers" == 'as 4 "10.0.0.1" "foo" "192.168.99.1" "bar"' ]]
+    assert_timesyncd_signal "$ts" RuntimeNTPServers "10.0.0.1 foo 192.168.99.1 bar"
+
+    # Cleanup
+    systemctl stop systemd-networkd systemd-timesyncd
+    rm -f /run/systemd/network/ntp99.*
 }
 
 run_testcases