platform: return failure reason from route-add and return only netlink response

Let nm_platform_ip_route_add() and friends return an NMPlatformError
failure reason.

Also, do_add_addrroute() did not return the response from kernel.
Instead, it determined success/failure based on the presence of the
object in the cache. That is racy and does not allow to give a failure
reason from kernel.

Instead, determine success solely based on the netlink reply from
kernel. The received errno shall be authorative, there is no need
to second guess the response.

There is a problem that netlink is not a reliable protocol. In case
of receive buffer overflow, the response is lost and we don't know
whether the command succeeded (it likely did). It's unclear how to fix
that, but for now just return "unspecified" error. We probably avoid
that already by having a huge buffer size.

Also, downgrade the error message to <warn> level. <error> is really
for bugs only.

7 files changed, 60 insertions, 51 deletions

diff --git a/src/nm-default-route-manager.c b/src/nm-default-route-manager.c
index 66a94a05a4..1f6342ccce 100644
--- a/src/nm-default-route-manager.c
+++ b/src/nm-default-route-manager.c
@@ -304,7 +304,7 @@ _platform_route_sync_add (const VTableIP *vtable, NMDefaultRouteManager *self, g
 		rt.plen = 0;
 		rt.metric = entry->effective_metric;
 
-		success = nm_platform_ip4_route_add (priv->platform, NMP_NLM_FLAG_REPLACE, &rt);
+		success = (nm_platform_ip4_route_add (priv->platform, NMP_NLM_FLAG_REPLACE, &rt) == NM_PLATFORM_ERROR_SUCCESS);
 	} else {
 		NMPlatformIP6Route rt = entry->route.r6;
 
@@ -312,7 +312,7 @@ _platform_route_sync_add (const VTableIP *vtable, NMDefaultRouteManager *self, g
 		rt.plen = 0;
 		rt.metric = entry->effective_metric;
 
-		success = nm_platform_ip6_route_add (priv->platform, NMP_NLM_FLAG_REPLACE, &rt);
+		success = (nm_platform_ip6_route_add (priv->platform, NMP_NLM_FLAG_REPLACE, &rt) == NM_PLATFORM_ERROR_SUCCESS);
 	}
 	if (!success) {
 		_LOGW (vtable->vt->addr_family, "failed to add default route %s with effective metric %u",
diff --git a/src/platform/nm-fake-platform.c b/src/platform/nm-fake-platform.c
index d51acce791..40877b321a 100644
--- a/src/platform/nm-fake-platform.c
+++ b/src/platform/nm-fake-platform.c
@@ -1199,7 +1199,7 @@ ip_route_delete (NMPlatform *platform, const NMPObject *obj)
 	return ipx_route_delete (platform, AF_UNSPEC, -1, obj);
 }
 
-static gboolean
+static NMPlatformError
 ip_route_add (NMPlatform *platform,
               NMPNlmFlags flags,
               int addr_family,
@@ -1284,7 +1284,7 @@ ip_route_add (NMPlatform *platform,
 				nm_log_warn (LOGD_PLATFORM, "Fake platform: failure adding ip6-route '%d: %s/%d %d': Network Unreachable",
 				             r->ifindex, nm_utils_inet6_ntop (&r6->network, NULL), r->plen, r->metric);
 			}
-			return FALSE;
+			return NM_PLATFORM_ERROR_UNSPECIFIED;
 		}
 	}
 
@@ -1346,7 +1346,7 @@ ip_route_add (NMPlatform *platform,
 		}
 	}
 
-	return TRUE;
+	return NM_PLATFORM_ERROR_SUCCESS;
 }
 
 /*****************************************************************************/
diff --git a/src/platform/nm-linux-platform.c b/src/platform/nm-linux-platform.c
index 6c4b34a03c..7c874df23f 100644
--- a/src/platform/nm-linux-platform.c
+++ b/src/platform/nm-linux-platform.c
@@ -265,6 +265,16 @@ static gboolean event_handler_read_netlink (NMPlatform *platform, gboolean wait_
 
 /*****************************************************************************/
 
+static NMPlatformError
+wait_for_nl_response_to_plerr (WaitForNlResponseResult seq_result)
+{
+	if (seq_result == WAIT_FOR_NL_RESPONSE_RESULT_RESPONSE_OK)
+		return NM_PLATFORM_ERROR_SUCCESS;
+	if (seq_result < 0)
+		return (NMPlatformError) seq_result;
+	return NM_PLATFORM_ERROR_NETLINK;
+}
+
 static const char *
 wait_for_nl_response_to_string (WaitForNlResponseResult seq_result, char *buf, gsize buf_size)
 {
@@ -4223,14 +4233,12 @@ do_add_link_with_lookup (NMPlatform *platform,
 	return !!obj;
 }
 
-static gboolean
+static NMPlatformError
 do_add_addrroute (NMPlatform *platform, const NMPObject *obj_id, struct nl_msg *nlmsg)
 {
 	WaitForNlResponseResult seq_result = WAIT_FOR_NL_RESPONSE_RESULT_UNKNOWN;
 	int nle;
 	char s_buf[256];
-	const NMPObject *obj;
-	NMPCache *cache = nm_platform_get_cache (platform);
 
 	nm_assert (NM_IN_SET (NMP_OBJECT_GET_TYPE (obj_id),
 	                      NMP_OBJECT_TYPE_IP4_ADDRESS, NMP_OBJECT_TYPE_IP6_ADDRESS,
@@ -4244,7 +4252,7 @@ do_add_addrroute (NMPlatform *platform, const NMPObject *obj_id, struct nl_msg *
 		       NMP_OBJECT_GET_CLASS (obj_id)->obj_type_name,
 		       nmp_object_to_string (obj_id, NMP_OBJECT_TO_STRING_ID, NULL, 0),
 		       nl_geterror (nle), -nle);
-		return FALSE;
+		return NM_PLATFORM_ERROR_NETLINK;
 	}
 
 	delayed_action_handle_all (platform, FALSE);
@@ -4253,30 +4261,26 @@ do_add_addrroute (NMPlatform *platform, const NMPObject *obj_id, struct nl_msg *
 
 	_NMLOG (seq_result == WAIT_FOR_NL_RESPONSE_RESULT_RESPONSE_OK
 	            ? LOGL_DEBUG
-	            : LOGL_ERR,
+	            : LOGL_WARN,
 	        "do-add-%s[%s]: %s",
 	        NMP_OBJECT_GET_CLASS (obj_id)->obj_type_name,
 	        nmp_object_to_string (obj_id, NMP_OBJECT_TO_STRING_ID, NULL, 0),
 	        wait_for_nl_response_to_string (seq_result, s_buf, sizeof (s_buf)));
 
-	/* In rare cases, the object is not yet ready as we received the ACK from
-	 * kernel. Need to refetch.
-	 *
-	 * We want to safe the expensive refetch, thus we look first into the cache
-	 * whether the object exists.
-	 *
-	 * FIXME: if the object already existed previously, we might not notice a
-	 * missing update. It's not clear how to fix that reliably without refechting
-	 * all the time. */
-	obj = nmp_cache_lookup_obj (cache, obj_id);
-	if (!obj) {
-		do_request_one_type (platform, NMP_OBJECT_GET_TYPE (obj_id));
-		obj = nmp_cache_lookup_obj (cache, obj_id);
+	if (NMP_OBJECT_GET_TYPE (obj_id) == NMP_OBJECT_TYPE_IP6_ADDRESS) {
+		/* In rare cases, the object is not yet ready as we received the ACK from
+		 * kernel. Need to refetch.
+		 *
+		 * We want to safe the expensive refetch, thus we look first into the cache
+		 * whether the object exists.
+		 *
+		 * rh#1484434 */
+		if (!nmp_cache_lookup_obj (nm_platform_get_cache (platform),
+		                           obj_id))
+			do_request_one_type (platform, NMP_OBJECT_GET_TYPE (obj_id));
 	}
 
-	/* Adding is only successful, if kernel reported success *and* we have the
-	 * expected object in cache afterwards. */
-	return obj && seq_result == WAIT_FOR_NL_RESPONSE_RESULT_RESPONSE_OK;
+	return wait_for_nl_response_to_plerr (seq_result);
 }
 
 static gboolean
@@ -5910,7 +5914,7 @@ ip4_address_add (NMPlatform *platform,
 	                             label);
 
 	nmp_object_stackinit_id_ip4_address (&obj_id, ifindex, addr, plen, peer_addr);
-	return do_add_addrroute (platform, &obj_id, nlmsg);
+	return do_add_addrroute (platform, &obj_id, nlmsg) == NM_PLATFORM_ERROR_SUCCESS;
 }
 
 static gboolean
@@ -5940,7 +5944,7 @@ ip6_address_add (NMPlatform *platform,
 	                             NULL);
 
 	nmp_object_stackinit_id_ip6_address (&obj_id, ifindex, &addr);
-	return do_add_addrroute (platform, &obj_id, nlmsg);
+	return do_add_addrroute (platform, &obj_id, nlmsg) == NM_PLATFORM_ERROR_SUCCESS;
 }
 
 static gboolean
@@ -5995,7 +5999,7 @@ ip6_address_delete (NMPlatform *platform, int ifindex, struct in6_addr addr, gui
 
 /*****************************************************************************/
 
-static gboolean
+static NMPlatformError
 ip_route_add (NMPlatform *platform,
               NMPNlmFlags flags,
               int addr_family,
@@ -6019,7 +6023,7 @@ ip_route_add (NMPlatform *platform,
 
 	nlmsg = _nl_msg_new_route (RTM_NEWROUTE, flags, &obj);
 	if (!nlmsg)
-		g_return_val_if_reached (FALSE);
+		g_return_val_if_reached (NM_PLATFORM_ERROR_BUG);
 	return do_add_addrroute (platform, &obj, nlmsg);
 }
 
diff --git a/src/platform/nm-platform.c b/src/platform/nm-platform.c
index 211a0abc9f..ae409b207b 100644
--- a/src/platform/nm-platform.c
+++ b/src/platform/nm-platform.c
@@ -248,6 +248,7 @@ NM_UTILS_LOOKUP_STR_DEFINE (_nm_platform_error_to_string, NMPlatformError,
 	NM_UTILS_LOOKUP_STR_ITEM (NM_PLATFORM_ERROR_NOT_SLAVE,   "not-slave"),
 	NM_UTILS_LOOKUP_STR_ITEM (NM_PLATFORM_ERROR_NO_FIRMWARE, "no-firmware"),
 	NM_UTILS_LOOKUP_STR_ITEM (NM_PLATFORM_ERROR_OPNOTSUPP,   "not-supported"),
+	NM_UTILS_LOOKUP_STR_ITEM (NM_PLATFORM_ERROR_NETLINK,     "netlink"),
 	NM_UTILS_LOOKUP_ITEM_IGNORE (_NM_PLATFORM_ERROR_MININT),
 );
 
@@ -3597,6 +3598,8 @@ nm_platform_ip_route_sync (NMPlatform *self,
 
 	for (i_type = 0; i_type < 2; i_type++) {
 		for (i = 0; i < routes->len; i++) {
+			NMPlatformError plerr;
+
 			conf_o = routes->pdata[i];
 
 #define VTABLE_IS_DEVICE_ROUTE(vt, o) (vt->is_ip4 \
@@ -3621,9 +3624,10 @@ nm_platform_ip_route_sync (NMPlatform *self,
 				continue;
 			}
 
-			if (!nm_platform_ip_route_add (self,
-			                               NMP_NLM_FLAG_APPEND,
-			                               conf_o)) {
+			plerr = nm_platform_ip_route_add (self,
+			                                  NMP_NLM_FLAG_APPEND,
+			                                  conf_o);
+			if (plerr != NM_PLATFORM_ERROR_SUCCESS) {
 				/* ignore error adding route. */
 			}
 		}
@@ -3710,7 +3714,7 @@ nm_platform_ip_route_normalize (int addr_family,
 	}
 }
 
-static gboolean
+static NMPlatformError
 _ip_route_add (NMPlatform *self,
                NMPNlmFlags flags,
                int addr_family,
@@ -3733,7 +3737,7 @@ _ip_route_add (NMPlatform *self,
 	return klass->ip_route_add (self, flags, addr_family, route);
 }
 
-gboolean
+NMPlatformError
 nm_platform_ip_route_add (NMPlatform *self,
                           NMPNlmFlags flags,
                           const NMPObject *route)
@@ -3754,7 +3758,7 @@ nm_platform_ip_route_add (NMPlatform *self,
 	return _ip_route_add (self, flags, addr_family, NMP_OBJECT_CAST_IP_ROUTE (route));
 }
 
-gboolean
+NMPlatformError
 nm_platform_ip4_route_add (NMPlatform *self,
                            NMPNlmFlags flags,
                            const NMPlatformIP4Route *route)
@@ -3762,7 +3766,7 @@ nm_platform_ip4_route_add (NMPlatform *self,
 	return _ip_route_add (self, flags, AF_INET, route);
 }
 
-gboolean
+NMPlatformError
 nm_platform_ip6_route_add (NMPlatform *self,
                            NMPNlmFlags flags,
                            const NMPlatformIP6Route *route)
diff --git a/src/platform/nm-platform.h b/src/platform/nm-platform.h
index 2bd6509c73..dbd55b4dd0 100644
--- a/src/platform/nm-platform.h
+++ b/src/platform/nm-platform.h
@@ -158,6 +158,7 @@ typedef enum { /*< skip >*/
 	NM_PLATFORM_ERROR_NOT_SLAVE,
 	NM_PLATFORM_ERROR_NO_FIRMWARE,
 	NM_PLATFORM_ERROR_OPNOTSUPP,
+	NM_PLATFORM_ERROR_NETLINK,
 } NMPlatformError;
 
 #define NM_PLATFORM_LINK_OTHER_NETNS    (-1)
@@ -797,10 +798,10 @@ typedef struct {
 	gboolean (*ip4_address_delete) (NMPlatform *, int ifindex, in_addr_t address, guint8 plen, in_addr_t peer_address);
 	gboolean (*ip6_address_delete) (NMPlatform *, int ifindex, struct in6_addr address, guint8 plen);
 
-	gboolean (*ip_route_add) (NMPlatform *,
-	                          NMPNlmFlags flags,
-	                          int addr_family,
-	                          const NMPlatformIPRoute *route);
+	NMPlatformError (*ip_route_add) (NMPlatform *,
+	                                 NMPNlmFlags flags,
+	                                 int addr_family,
+	                                 const NMPlatformIPRoute *route);
 	gboolean (*ip_route_delete) (NMPlatform *, const NMPObject *obj);
 
 	NMPlatformError (*ip_route_get) (NMPlatform *self,
@@ -1122,11 +1123,11 @@ gboolean nm_platform_ip_address_flush (NMPlatform *self,
 void nm_platform_ip_route_normalize (int addr_family,
                                      NMPlatformIPRoute *route);
 
-gboolean nm_platform_ip_route_add (NMPlatform *self,
-                                   NMPNlmFlags flags,
-                                   const NMPObject *route);
-gboolean nm_platform_ip4_route_add (NMPlatform *self, NMPNlmFlags flags, const NMPlatformIP4Route *route);
-gboolean nm_platform_ip6_route_add (NMPlatform *self, NMPNlmFlags flags, const NMPlatformIP6Route *route);
+NMPlatformError nm_platform_ip_route_add (NMPlatform *self,
+                                          NMPNlmFlags flags,
+                                          const NMPObject *route);
+NMPlatformError nm_platform_ip4_route_add (NMPlatform *self, NMPNlmFlags flags, const NMPlatformIP4Route *route);
+NMPlatformError nm_platform_ip6_route_add (NMPlatform *self, NMPNlmFlags flags, const NMPlatformIP6Route *route);
 
 gboolean nm_platform_ip_route_delete (NMPlatform *self, const NMPObject *route);
 
diff --git a/src/platform/tests/test-common.c b/src/platform/tests/test-common.c
index 9692c09050..c0e6cb96de 100644
--- a/src/platform/tests/test-common.c
+++ b/src/platform/tests/test-common.c
@@ -1009,7 +1009,7 @@ void nmtstp_ip4_route_add (NMPlatform *platform,
 	route.metric = metric;
 	route.mss = mss;
 
-	g_assert (nm_platform_ip4_route_add (platform, NMP_NLM_FLAG_REPLACE, &route));
+	g_assert_cmpint (nm_platform_ip4_route_add (platform, NMP_NLM_FLAG_REPLACE, &route), ==, NM_PLATFORM_ERROR_SUCCESS);
 }
 
 void nmtstp_ip6_route_add (NMPlatform *platform,
@@ -1033,7 +1033,7 @@ void nmtstp_ip6_route_add (NMPlatform *platform,
 	route.metric = metric;
 	route.mss = mss;
 
-	g_assert (nm_platform_ip6_route_add (platform, NMP_NLM_FLAG_REPLACE, &route));
+	g_assert_cmpint (nm_platform_ip6_route_add (platform, NMP_NLM_FLAG_REPLACE, &route), ==, NM_PLATFORM_ERROR_SUCCESS);
 }
 
 /*****************************************************************************/
diff --git a/src/platform/tests/test-route.c b/src/platform/tests/test-route.c
index a8ba384492..f749d7514c 100644
--- a/src/platform/tests/test-route.c
+++ b/src/platform/tests/test-route.c
@@ -469,7 +469,7 @@ test_ip4_route_options (void)
 	route.mtu = 1350;
 	route.lock_cwnd = TRUE;
 
-	g_assert (nm_platform_ip4_route_add (NM_PLATFORM_GET, NMP_NLM_FLAG_REPLACE, &route));
+	g_assert (nm_platform_ip4_route_add (NM_PLATFORM_GET, NMP_NLM_FLAG_REPLACE, &route) == NM_PLATFORM_ERROR_SUCCESS);
 
 	/* Test route listing */
 	routes = nmtstp_ip4_route_get_all (NM_PLATFORM_GET, ifindex);
@@ -597,7 +597,7 @@ test_ip6_route_options (gconstpointer test_data)
 	_wait_for_ipv6_addr_non_tentative (NM_PLATFORM_GET, 400, IFINDEX, addr_n, addr_in6);
 
 	for (i = 0; i < rts_n; i++)
-		g_assert (nm_platform_ip6_route_add (NM_PLATFORM_GET, NMP_NLM_FLAG_REPLACE, &rts_add[i]));
+		g_assert (nm_platform_ip6_route_add (NM_PLATFORM_GET, NMP_NLM_FLAG_REPLACE, &rts_add[i]) == NM_PLATFORM_ERROR_SUCCESS);
 
 	routes = nmtstp_ip6_route_get_all (NM_PLATFORM_GET, IFINDEX);
 	switch (TEST_IDX) {
@@ -703,7 +703,7 @@ again_find_idx:
 			order_idx[order_len++] = idx;
 
 			r->ifindex = iface_data[idx].ifindex;
-			g_assert (nm_platform_ip4_route_add (platform, NMP_NLM_FLAG_APPEND, r));
+			g_assert (nm_platform_ip4_route_add (platform, NMP_NLM_FLAG_APPEND, r) == NM_PLATFORM_ERROR_SUCCESS);
 		} else {
 			i = nmtst_get_rand_int () % order_len;
 			idx = order_idx[i];