colin/NetworkManager
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

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

Browse Source 
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.
This commit is contained in:
Thomas Haller
2017-08-21 15:33:57 +02:00
parent fa7fafe8fd
commit 774c8a811e
7 changed files with 60 additions and 51 deletions
Download Patch File Download Diff File Expand all files Collapse all files

4
src/nm-default-route-manager.c
View File

@@ -304,7 +304,7 @@ _platform_route_sync_add (const VTableIP *vtable, NMDefaultRouteManager *self, g
rt.plen = 0; rt.plen = 0;
rt.metric = entry->effective_metric; 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 { } else {
NMPlatformIP6Route rt = entry->route.r6; NMPlatformIP6Route rt = entry->route.r6;
@@ -312,7 +312,7 @@ _platform_route_sync_add (const VTableIP *vtable, NMDefaultRouteManager *self, g
rt.plen = 0; rt.plen = 0;
rt.metric = entry->effective_metric; 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) { if (!success) {
_LOGW (vtable->vt->addr_family, "failed to add default route %s with effective metric %u", _LOGW (vtable->vt->addr_family, "failed to add default route %s with effective metric %u",

6
src/platform/nm-fake-platform.c
View File

@@ -1199,7 +1199,7 @@ ip_route_delete (NMPlatform *platform, const NMPObject *obj)
return ipx_route_delete (platform, AF_UNSPEC, -1, obj); return ipx_route_delete (platform, AF_UNSPEC, -1, obj);
} }
static gboolean static NMPlatformError
ip_route_add (NMPlatform *platform, ip_route_add (NMPlatform *platform,
NMPNlmFlags flags, NMPNlmFlags flags,
int addr_family, 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", 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); 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;
} }
/*****************************************************************************/ /*****************************************************************************/

40
src/platform/nm-linux-platform.c
View File

@@ -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 * static const char *
wait_for_nl_response_to_string (WaitForNlResponseResult seq_result, char *buf, gsize buf_size) 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; return !!obj;
} }
static gboolean static NMPlatformError
do_add_addrroute (NMPlatform *platform, const NMPObject *obj_id, struct nl_msg *nlmsg) do_add_addrroute (NMPlatform *platform, const NMPObject *obj_id, struct nl_msg *nlmsg)
{ {
WaitForNlResponseResult seq_result = WAIT_FOR_NL_RESPONSE_RESULT_UNKNOWN; WaitForNlResponseResult seq_result = WAIT_FOR_NL_RESPONSE_RESULT_UNKNOWN;
int nle; int nle;
char s_buf[256]; 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), nm_assert (NM_IN_SET (NMP_OBJECT_GET_TYPE (obj_id),
NMP_OBJECT_TYPE_IP4_ADDRESS, NMP_OBJECT_TYPE_IP6_ADDRESS, 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_GET_CLASS (obj_id)->obj_type_name,
nmp_object_to_string (obj_id, NMP_OBJECT_TO_STRING_ID, NULL, 0), nmp_object_to_string (obj_id, NMP_OBJECT_TO_STRING_ID, NULL, 0),
nl_geterror (nle), -nle); nl_geterror (nle), -nle);
return FALSE; return NM_PLATFORM_ERROR_NETLINK;
} }
delayed_action_handle_all (platform, FALSE); 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 _NMLOG (seq_result == WAIT_FOR_NL_RESPONSE_RESULT_RESPONSE_OK
? LOGL_DEBUG ? LOGL_DEBUG
: LOGL_ERR, : LOGL_WARN,
"do-add-%s[%s]: %s", "do-add-%s[%s]: %s",
NMP_OBJECT_GET_CLASS (obj_id)->obj_type_name, NMP_OBJECT_GET_CLASS (obj_id)->obj_type_name,
nmp_object_to_string (obj_id, NMP_OBJECT_TO_STRING_ID, NULL, 0), 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))); wait_for_nl_response_to_string (seq_result, s_buf, sizeof (s_buf)));
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 /* In rare cases, the object is not yet ready as we received the ACK from
* kernel. Need to refetch. * kernel. Need to refetch.
* *
* We want to safe the expensive refetch, thus we look first into the cache * We want to safe the expensive refetch, thus we look first into the cache
* whether the object exists. * whether the object exists.
* *
* FIXME: if the object already existed previously, we might not notice a * rh#1484434 */
* missing update. It's not clear how to fix that reliably without refechting if (!nmp_cache_lookup_obj (nm_platform_get_cache (platform),
* all the time. */ obj_id))
obj = nmp_cache_lookup_obj (cache, obj_id);
if (!obj) {
do_request_one_type (platform, NMP_OBJECT_GET_TYPE (obj_id)); do_request_one_type (platform, NMP_OBJECT_GET_TYPE (obj_id));
obj = nmp_cache_lookup_obj (cache, obj_id);
} }
/* Adding is only successful, if kernel reported success *and* we have the return wait_for_nl_response_to_plerr (seq_result);
* expected object in cache afterwards. */
return obj && seq_result == WAIT_FOR_NL_RESPONSE_RESULT_RESPONSE_OK;
} }
static gboolean static gboolean
@@ -5910,7 +5914,7 @@ ip4_address_add (NMPlatform *platform,
label); label);
nmp_object_stackinit_id_ip4_address (&obj_id, ifindex, addr, plen, peer_addr); 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 static gboolean
@@ -5940,7 +5944,7 @@ ip6_address_add (NMPlatform *platform,
NULL); NULL);
nmp_object_stackinit_id_ip6_address (&obj_id, ifindex, &addr); 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 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, ip_route_add (NMPlatform *platform,
NMPNlmFlags flags, NMPNlmFlags flags,
int addr_family, int addr_family,
@@ -6019,7 +6023,7 @@ ip_route_add (NMPlatform *platform,
nlmsg = _nl_msg_new_route (RTM_NEWROUTE, flags, &obj); nlmsg = _nl_msg_new_route (RTM_NEWROUTE, flags, &obj);
if (!nlmsg) if (!nlmsg)
g_return_val_if_reached (FALSE); g_return_val_if_reached (NM_PLATFORM_ERROR_BUG);
return do_add_addrroute (platform, &obj, nlmsg); return do_add_addrroute (platform, &obj, nlmsg);
} }

16
src/platform/nm-platform.c
View File

@@ -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_NOT_SLAVE, "not-slave"),
NM_UTILS_LOOKUP_STR_ITEM (NM_PLATFORM_ERROR_NO_FIRMWARE, "no-firmware"), 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_OPNOTSUPP, "not-supported"),
NM_UTILS_LOOKUP_STR_ITEM (NM_PLATFORM_ERROR_NETLINK, "netlink"),
NM_UTILS_LOOKUP_ITEM_IGNORE (_NM_PLATFORM_ERROR_MININT), 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_type = 0; i_type < 2; i_type++) {
for (i = 0; i < routes->len; i++) { for (i = 0; i < routes->len; i++) {
NMPlatformError plerr;
conf_o = routes->pdata[i]; conf_o = routes->pdata[i];
#define VTABLE_IS_DEVICE_ROUTE(vt, o) (vt->is_ip4 \ #define VTABLE_IS_DEVICE_ROUTE(vt, o) (vt->is_ip4 \
@@ -3621,9 +3624,10 @@ nm_platform_ip_route_sync (NMPlatform *self,
continue; continue;
} }
if (!nm_platform_ip_route_add (self, plerr = nm_platform_ip_route_add (self,
NMP_NLM_FLAG_APPEND, NMP_NLM_FLAG_APPEND,
conf_o)) { conf_o);
if (plerr != NM_PLATFORM_ERROR_SUCCESS) {
/* ignore error adding route. */ /* 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, _ip_route_add (NMPlatform *self,
NMPNlmFlags flags, NMPNlmFlags flags,
int addr_family, int addr_family,
@@ -3733,7 +3737,7 @@ _ip_route_add (NMPlatform *self,
return klass->ip_route_add (self, flags, addr_family, route); return klass->ip_route_add (self, flags, addr_family, route);
} }
gboolean NMPlatformError
nm_platform_ip_route_add (NMPlatform *self, nm_platform_ip_route_add (NMPlatform *self,
NMPNlmFlags flags, NMPNlmFlags flags,
const NMPObject *route) 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)); return _ip_route_add (self, flags, addr_family, NMP_OBJECT_CAST_IP_ROUTE (route));
} }
gboolean NMPlatformError
nm_platform_ip4_route_add (NMPlatform *self, nm_platform_ip4_route_add (NMPlatform *self,
NMPNlmFlags flags, NMPNlmFlags flags,
const NMPlatformIP4Route *route) const NMPlatformIP4Route *route)
@@ -3762,7 +3766,7 @@ nm_platform_ip4_route_add (NMPlatform *self,
return _ip_route_add (self, flags, AF_INET, route); return _ip_route_add (self, flags, AF_INET, route);
} }
gboolean NMPlatformError
nm_platform_ip6_route_add (NMPlatform *self, nm_platform_ip6_route_add (NMPlatform *self,
NMPNlmFlags flags, NMPNlmFlags flags,
const NMPlatformIP6Route *route) const NMPlatformIP6Route *route)

9
src/platform/nm-platform.h
View File

@@ -158,6 +158,7 @@ typedef enum { /*< skip >*/
NM_PLATFORM_ERROR_NOT_SLAVE, NM_PLATFORM_ERROR_NOT_SLAVE,
NM_PLATFORM_ERROR_NO_FIRMWARE, NM_PLATFORM_ERROR_NO_FIRMWARE,
NM_PLATFORM_ERROR_OPNOTSUPP, NM_PLATFORM_ERROR_OPNOTSUPP,
NM_PLATFORM_ERROR_NETLINK,
} NMPlatformError; } NMPlatformError;
#define NM_PLATFORM_LINK_OTHER_NETNS (-1) #define NM_PLATFORM_LINK_OTHER_NETNS (-1)
@@ -797,7 +798,7 @@ typedef struct {
gboolean (*ip4_address_delete) (NMPlatform *, int ifindex, in_addr_t address, guint8 plen, in_addr_t peer_address); 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 (*ip6_address_delete) (NMPlatform *, int ifindex, struct in6_addr address, guint8 plen);
gboolean (*ip_route_add) (NMPlatform *, NMPlatformError (*ip_route_add) (NMPlatform *,
NMPNlmFlags flags, NMPNlmFlags flags,
int addr_family, int addr_family,
const NMPlatformIPRoute *route); const NMPlatformIPRoute *route);
@@ -1122,11 +1123,11 @@ gboolean nm_platform_ip_address_flush (NMPlatform *self,
void nm_platform_ip_route_normalize (int addr_family, void nm_platform_ip_route_normalize (int addr_family,
NMPlatformIPRoute *route); NMPlatformIPRoute *route);
gboolean nm_platform_ip_route_add (NMPlatform *self, NMPlatformError nm_platform_ip_route_add (NMPlatform *self,
NMPNlmFlags flags, NMPNlmFlags flags,
const NMPObject *route); const NMPObject *route);
gboolean nm_platform_ip4_route_add (NMPlatform *self, NMPNlmFlags flags, const NMPlatformIP4Route *route); NMPlatformError 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_ip6_route_add (NMPlatform *self, NMPNlmFlags flags, const NMPlatformIP6Route *route);
gboolean nm_platform_ip_route_delete (NMPlatform *self, const NMPObject *route); gboolean nm_platform_ip_route_delete (NMPlatform *self, const NMPObject *route);

4
src/platform/tests/test-common.c
View File

@@ -1009,7 +1009,7 @@ void nmtstp_ip4_route_add (NMPlatform *platform,
route.metric = metric; route.metric = metric;
route.mss = mss; 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, void nmtstp_ip6_route_add (NMPlatform *platform,
@@ -1033,7 +1033,7 @@ void nmtstp_ip6_route_add (NMPlatform *platform,
route.metric = metric; route.metric = metric;
route.mss = mss; 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);
} }
/*****************************************************************************/ /*****************************************************************************/

6
src/platform/tests/test-route.c
View File

@@ -469,7 +469,7 @@ test_ip4_route_options (void)
route.mtu = 1350; route.mtu = 1350;
route.lock_cwnd = TRUE; 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 */ /* Test route listing */
routes = nmtstp_ip4_route_get_all (NM_PLATFORM_GET, ifindex); 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); _wait_for_ipv6_addr_non_tentative (NM_PLATFORM_GET, 400, IFINDEX, addr_n, addr_in6);
for (i = 0; i < rts_n; i++) 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); routes = nmtstp_ip6_route_get_all (NM_PLATFORM_GET, IFINDEX);
switch (TEST_IDX) { switch (TEST_IDX) {
@@ -703,7 +703,7 @@ again_find_idx:
order_idx[order_len++] = idx; order_idx[order_len++] = idx;
r->ifindex = iface_data[idx].ifindex; 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 { } else {
i = nmtst_get_rand_int () % order_len; i = nmtst_get_rand_int () % order_len;
idx = order_idx[i]; idx = order_idx[i];