cli: fix sorting of active connections

CMP() is a confusing pattern. Sure enough, the sort order was wrong, for
example, `nmcli connection` would show

    $ nmcli -f STATE,UUID,DEVICE c
    STATE       UUID                                  DEVICE
    activating  3098c902-c59c-45f4-9e5a-e4cdb79cfe1b  nm-bond
    activated   e4fc23ac-54ab-4b1a-932a-ebed12c96d9b  eth1

("activating" shown before "activated").

With `nmcli device`, we sort with compare_devices(). This first sorts by
device state (with "connected" being sorted first). Only when the device
state is equal, we fallback to nmc_active_connection_cmp().  So with
`nmcli device` we usually get "connected" devices first, and we don't
really notice that there is a problem with nmc_active_connection_cmp().

On the other hand, `nmcli connection` likes to sort first via
nmc_active_connection_cmp(), which gets it wrong. Profiles in
"activating" state are sorted first. That's inconsistent with `nmcli
device`, but it's also not what is intended.

Fix that.

Note the change in the test output. Both eth1 and eth0 are connected to
to the same profile, but one "eth0" the active-connection's state is
DEACTIVATING, while on "eth1" it's ACTIVATED (but both device's states
are "CONNECTED"). That's why "eth1" is now sorted first (as desired).

Fixes: a1b25a47b08c ('cli: rework printing of `nmcli connection` for multiple active connections')

2 files changed, 57 insertions, 43 deletions

diff --git a/src/nmcli/connections.c b/src/nmcli/connections.c
index c4f70bd1b2..df81a70712 100644
--- a/src/nmcli/connections.c
+++ b/src/nmcli/connections.c
@@ -314,42 +314,56 @@ active_connection_get_state_ord(NMActiveConnection *active)
 int
 nmc_active_connection_cmp(NMActiveConnection *ac_a, NMActiveConnection *ac_b)
 {
-    NMSettingIPConfig  *s_ip;
-    NMRemoteConnection *conn;
+    NMSettingIPConfig  *s_ip4_a;
+    NMSettingIPConfig  *s_ip4_b;
+    NMSettingIPConfig  *s_ip6_a;
+    NMSettingIPConfig  *s_ip6_b;
+    NMRemoteConnection *conn_a;
+    NMRemoteConnection *conn_b;
     NMIPConfig         *da_ip;
     NMIPConfig         *db_ip;
-    int                 da_num_addrs;
-    int                 db_num_addrs;
-    int                 cmp = 0;
+    gint64              da_num_addrs;
+    gint64              db_num_addrs;
+    gboolean            bool_a;
+    gboolean            bool_b;
+
+    /* nmc_active_connection_cmp() sorts more-important ACs later. That means,
+     * - NULL comes first
+     * - then sorting by state (active_connection_get_state_ord()), with "activated" sorted last.
+     * - various properties of the AC.
+     *
+     * This is basically the inverse order of `nmcli connection`.
+     */
 
-    /* Non-active sort last. */
+    /* Non-active (and NULL) sort first! */
     NM_CMP_SELF(ac_a, ac_b);
-    NM_CMP_DIRECT(active_connection_get_state_ord(ac_b), active_connection_get_state_ord(ac_a));
+    NM_CMP_DIRECT(active_connection_get_state_ord(ac_a), active_connection_get_state_ord(ac_b));
+
+    conn_a = nm_active_connection_get_connection(ac_a);
+    conn_b = nm_active_connection_get_connection(ac_b);
+
+    s_ip6_a = conn_a ? nm_connection_get_setting_ip6_config(NM_CONNECTION(conn_a)) : NULL;
+    s_ip6_b = conn_b ? nm_connection_get_setting_ip6_config(NM_CONNECTION(conn_b)) : NULL;
 
     /* Shared connections (likely hotspots) go on the top if possible */
-    conn = nm_active_connection_get_connection(ac_a);
-    s_ip = conn ? nm_connection_get_setting_ip6_config(NM_CONNECTION(conn)) : NULL;
-    if (s_ip
-        && nm_streq(nm_setting_ip_config_get_method(s_ip), NM_SETTING_IP6_CONFIG_METHOD_SHARED))
-        cmp++;
-    conn = nm_active_connection_get_connection(ac_b);
-    s_ip = conn ? nm_connection_get_setting_ip6_config(NM_CONNECTION(conn)) : NULL;
-    if (s_ip
-        && nm_streq(nm_setting_ip_config_get_method(s_ip), NM_SETTING_IP6_CONFIG_METHOD_SHARED))
-        cmp--;
-    NM_CMP_RETURN(cmp);
-
-    conn = nm_active_connection_get_connection(ac_a);
-    s_ip = conn ? nm_connection_get_setting_ip4_config(NM_CONNECTION(conn)) : NULL;
-    if (s_ip
-        && nm_streq(nm_setting_ip_config_get_method(s_ip), NM_SETTING_IP4_CONFIG_METHOD_SHARED))
-        cmp++;
-    conn = nm_active_connection_get_connection(ac_b);
-    s_ip = conn ? nm_connection_get_setting_ip4_config(NM_CONNECTION(conn)) : NULL;
-    if (s_ip
-        && nm_streq(nm_setting_ip_config_get_method(s_ip), NM_SETTING_IP4_CONFIG_METHOD_SHARED))
-        cmp--;
-    NM_CMP_RETURN(cmp);
+    bool_a = (s_ip6_a
+              && nm_streq(nm_setting_ip_config_get_method(s_ip6_a),
+                          NM_SETTING_IP6_CONFIG_METHOD_SHARED));
+    bool_b = (s_ip6_b
+              && nm_streq(nm_setting_ip_config_get_method(s_ip6_b),
+                          NM_SETTING_IP6_CONFIG_METHOD_SHARED));
+    NM_CMP_DIRECT(bool_a, bool_b);
+
+    s_ip4_a = conn_a ? nm_connection_get_setting_ip4_config(NM_CONNECTION(conn_a)) : NULL;
+    s_ip4_b = conn_b ? nm_connection_get_setting_ip4_config(NM_CONNECTION(conn_b)) : NULL;
+
+    bool_a = (s_ip4_a
+              && nm_streq(nm_setting_ip_config_get_method(s_ip4_a),
+                          NM_SETTING_IP4_CONFIG_METHOD_SHARED));
+    bool_b = (s_ip4_b
+              && nm_streq(nm_setting_ip_config_get_method(s_ip4_b),
+                          NM_SETTING_IP4_CONFIG_METHOD_SHARED));
+    NM_CMP_DIRECT(bool_a, bool_b);
 
     /* VPNs go next */
     NM_CMP_DIRECT(!!nm_active_connection_get_vpn(ac_a), !!nm_active_connection_get_vpn(ac_b));
@@ -367,9 +381,9 @@ nmc_active_connection_cmp(NMActiveConnection *ac_a, NMActiveConnection *ac_b)
     db_num_addrs = db_ip ? nm_ip_config_get_addresses(db_ip)->len : 0;
 
     da_ip = nm_active_connection_get_ip6_config(ac_a);
-    da_num_addrs += da_ip ? nm_ip_config_get_addresses(da_ip)->len : 0;
+    da_num_addrs += (gint64) (da_ip ? nm_ip_config_get_addresses(da_ip)->len : 0u);
     db_ip = nm_active_connection_get_ip6_config(ac_b);
-    db_num_addrs += db_ip ? nm_ip_config_get_addresses(db_ip)->len : 0;
+    db_num_addrs += (gint64) (db_ip ? nm_ip_config_get_addresses(db_ip)->len : 0u);
 
     NM_CMP_DIRECT(da_num_addrs, db_num_addrs);
 
@@ -1447,7 +1461,7 @@ get_ac_for_connection_cmp(gconstpointer pa, gconstpointer pb)
     NMActiveConnection *ac_a = *((NMActiveConnection *const *) pa);
     NMActiveConnection *ac_b = *((NMActiveConnection *const *) pb);
 
-    NM_CMP_RETURN(nmc_active_connection_cmp(ac_a, ac_b));
+    NM_CMP_RETURN(nmc_active_connection_cmp(ac_b, ac_a));
     NM_CMP_DIRECT_STRCMP0(nm_active_connection_get_id(ac_a), nm_active_connection_get_id(ac_b));
     NM_CMP_DIRECT_STRCMP0(nm_active_connection_get_connection_type(ac_a),
                           nm_active_connection_get_connection_type(ac_b));
diff --git a/src/tests/client/test-client.check-on-disk/test_003.expected b/src/tests/client/test-client.check-on-disk/test_003.expected
index 6d2bb62c35..228073fe31 100644
--- a/src/tests/client/test-client.check-on-disk/test_003.expected
+++ b/src/tests/client/test-client.check-on-disk/test_003.expected
@@ -2986,8 +2986,8 @@ returncode: 0
 stdout: 1272 bytes
 >>>
 DEVICE  TYPE      STATE        IP4-CONNECTIVITY  IP6-CONNECTIVITY  DBUS-PATH                                  CONNECTION  CON-UUID                              CON-PATH                                           
-eth0    ethernet  connected    unknown           unknown           /org/freedesktop/NetworkManager/Devices/1  ethernet    UUID-ethernet-REPLACED-REPLACED-REPL  /org/freedesktop/NetworkManager/ActiveConnection/1 
 eth1    ethernet  connected    unknown           unknown           /org/freedesktop/NetworkManager/Devices/2  ethernet    UUID-ethernet-REPLACED-REPLACED-REPL  /org/freedesktop/NetworkManager/ActiveConnection/2 
+eth0    ethernet  connected    unknown           unknown           /org/freedesktop/NetworkManager/Devices/1  ethernet    UUID-ethernet-REPLACED-REPLACED-REPL  /org/freedesktop/NetworkManager/ActiveConnection/1 
 wlan0   wifi      unavailable  unknown           unknown           /org/freedesktop/NetworkManager/Devices/3  --          --                                    --                                                 
 wlan1   wifi      unavailable  unknown           unknown           /org/freedesktop/NetworkManager/Devices/4  --          --                                    --                                                 
 wlan1   wifi      unavailable  unknown           unknown           /org/freedesktop/NetworkManager/Devices/5  --          --                                    --                                                 
@@ -3001,8 +3001,8 @@ returncode: 0
 stdout: 1279 bytes
 >>>
 DEVICE  TYPE      STATE        IP4-CONNECTIVITY  IP6-CONNECTIVITY  DBUS-PATH                                  CONNECTION  CON-UUID                              CON-PATH                                           
-eth0    ethernet  połączono    nieznane          nieznane          /org/freedesktop/NetworkManager/Devices/1  ethernet    UUID-ethernet-REPLACED-REPLACED-REPL  /org/freedesktop/NetworkManager/ActiveConnection/1 
 eth1    ethernet  połączono    nieznane          nieznane          /org/freedesktop/NetworkManager/Devices/2  ethernet    UUID-ethernet-REPLACED-REPLACED-REPL  /org/freedesktop/NetworkManager/ActiveConnection/2 
+eth0    ethernet  połączono    nieznane          nieznane          /org/freedesktop/NetworkManager/Devices/1  ethernet    UUID-ethernet-REPLACED-REPLACED-REPL  /org/freedesktop/NetworkManager/ActiveConnection/1 
 wlan0   wifi      niedostępne  nieznane          nieznane          /org/freedesktop/NetworkManager/Devices/3  --          --                                    --                                                 
 wlan1   wifi      niedostępne  nieznane          nieznane          /org/freedesktop/NetworkManager/Devices/4  --          --                                    --                                                 
 wlan1   wifi      niedostępne  nieznane          nieznane          /org/freedesktop/NetworkManager/Devices/5  --          --                                    --                                                 
@@ -3015,14 +3015,14 @@ lang: C
 returncode: 0
 stdout: 641 bytes
 >>>
-eth0: connected to ethernet
-	"eth0"
-	ethernet (virtual), C0:61:AE:26:4D:D7, hw
-
 eth1: connected to ethernet
 	"eth1"
 	ethernet (virtual), 33:9A:18:81:EB:DC, hw
 
+eth0: connected to ethernet
+	"eth0"
+	ethernet (virtual), C0:61:AE:26:4D:D7, hw
+
 wlan0: unavailable
 	"wlan0"
 	wifi (virtual), DC:39:87:BA:3E:5D, hw
@@ -3051,14 +3051,14 @@ lang: pl_PL.UTF-8
 returncode: 0
 stdout: 706 bytes
 >>>
-eth0: połączono do ethernet
-	"eth0"
-	ethernet (virtual), C0:61:AE:26:4D:D7, sprzęt
-
 eth1: połączono do ethernet
 	"eth1"
 	ethernet (virtual), 33:9A:18:81:EB:DC, sprzęt
 
+eth0: połączono do ethernet
+	"eth0"
+	ethernet (virtual), C0:61:AE:26:4D:D7, sprzęt
+
 wlan0: niedostępne
 	"wlan0"
 	wifi (virtual), DC:39:87:BA:3E:5D, sprzęt