From 9dac3076f741acbabee2e28c6740cb72a564a178 Mon Sep 17 00:00:00 2001
From: Thomas Haller <thaller@redhat.com>
Date: Sun, 6 Oct 2019 23:19:32 +0200
Subject: [PATCH] libnm: fix return value for
 nm_remote_settings_load_connections() to ignore server result

nm_remote_settings_load_connections() and nm_remote_settings_load_connections_async()
behave inconsistently.

It's unexpected, that a FALSE return value may leave @error unset.

Note that before commit 22e830f0469a ('settings/d-bus: fix boolean
return value of "LoadConnections"'), the server boolean response
would have been bogus anyway (at least for some versions).

Unify the behavior, and ignore the boolean return value.
---
 ...rg.freedesktop.NetworkManager.Settings.xml |  1 +
 libnm/nm-client.c                             | 24 ++++++++++++-------
 libnm/nm-remote-settings.c                    |  5 ++--
 3 files changed, 19 insertions(+), 11 deletions(-)

diff --git a/introspection/org.freedesktop.NetworkManager.Settings.xml b/introspection/org.freedesktop.NetworkManager.Settings.xml
index f7be2717b8..59e0d57549 100644
--- a/introspection/org.freedesktop.NetworkManager.Settings.xml
+++ b/introspection/org.freedesktop.NetworkManager.Settings.xml
@@ -102,6 +102,7 @@
         LoadConnections:
         @filenames: Array of paths to on-disk connection profiles in directories monitored by NetworkManager.
         @status: Success or failure of the operation as a whole. True if NetworkManager at least tried to load the indicated connections, even if it did not succeed. False if an error occurred before trying to load the connections (eg, permission denied).
+          Note that before 1.20, NetworkManager had a bug and this @status value was wrong. It is better to assume success if the method does not return with a D-Bus error. On top of that, you can look at @failures to know whether any of the requested files failed.
         @failures: Paths of connection files that could not be loaded.
 
         Loads or reloads the indicated connections from disk. You should call this
diff --git a/libnm/nm-client.c b/libnm/nm-client.c
index d5ae5d4ac3..7785628624 100644
--- a/libnm/nm-client.c
+++ b/libnm/nm-client.c
@@ -1896,8 +1896,16 @@ nm_client_add_connection2_finish (NMClient *client,
  * then @failures will be set to a %NULL-terminated array of the
  * filenames that failed to load.
  *
- * Returns: %TRUE if NetworkManager at least tried to load @filenames,
- * %FALSE if an error occurred (eg, permission denied).
+ * Returns: %TRUE on success.
+ *
+ * Warning: before libnm 1.22, the boolean return value was inconsistent.
+ *   That is made worse, because when running against certain server versions
+ *   before 1.20, the server would return wrong values for success/failure.
+ *   This means, if you use this function in libnm before 1.22, you are advised
+ *   to ignore the boolean return value and only look at @failures and @error.
+ *   With libnm >= 1.22, the boolean return value corresponds to whether @error was
+ *   set. Note that even in the success case, you might have individual @failures.
+ *   With 1.22, the return value is consistent with nm_client_load_connections_finish().
  *
  * Deprecated: 1.22, use nm_client_load_connections_async() or GDBusConnection
  **/
@@ -1988,8 +1996,8 @@ nm_client_load_connections_async (NMClient *client,
 
  * See nm_client_load_connections() for more details.
  *
- * Returns: %TRUE if NetworkManager at least tried to load @filenames,
- * %FALSE if an error occurred (eg, permission denied).
+ * Returns: %TRUE on success.
+ *   Note that even in the success case, you might have individual @failures.
  **/
 gboolean
 nm_client_load_connections_finish (NMClient *client,
@@ -2005,11 +2013,9 @@ nm_client_load_connections_finish (NMClient *client,
 	simple = G_SIMPLE_ASYNC_RESULT (result);
 	if (g_simple_async_result_propagate_error (simple, error))
 		return FALSE;
-	else {
-		if (failures)
-			*failures = g_strdupv (g_simple_async_result_get_op_res_gpointer (simple));
-		return TRUE;
-	}
+	if (failures)
+		*failures = g_strdupv (g_simple_async_result_get_op_res_gpointer (simple));
+	return TRUE;
 }
 
 /**
diff --git a/libnm/nm-remote-settings.c b/libnm/nm-remote-settings.c
index 24abcee397..3c6dc4ec93 100644
--- a/libnm/nm-remote-settings.c
+++ b/libnm/nm-remote-settings.c
@@ -416,9 +416,10 @@ nm_remote_settings_load_connections (NMRemoteSettings *settings,
 	                                                 cancellable, error)) {
 		if (error && *error)
 			g_dbus_error_strip_remote_error (*error);
-		success = FALSE;
+		return FALSE;
 	}
-	return success;
+
+	return TRUE;
 }
 
 static void