dhcpcd-discuss

Re: dhcpcd kills all connections on Wi-Fi roaming betweenaccesspoints

Roy Marples

Thu Dec 24 06:29:52 2020

On 24/12/2020 03:43, Boris Krasnovskiy wrote:
HI Roy,

sorry, my mistake, I used a config file with disabled DNS options.

The code partially works, the problem is:  after roaming DNS interfaces get reordered, and this is incorrect.

DNS interfaces should be ordered according to their metric always.

I disagree.
The rules are these: Precedence list, fallthrough on equality:

Carrier UP
Roaming
Not IPv4LL
Interface Metric
Oldest

In previous emails you've indicated that in the roaming state it never actually goes down, just back up again which could take time to find an AP. As such, you'll have a hard job convincing me that the working ethernet you just plugged into should not take precedence by default.

Now this could be an issue where the route metric == the interface metric (ie Linux, not NetBSD where my testing is) so we need to adjust the code to change the route metric to reflect roaming vs carrier up.

Your quote:
DNS interfaces should be ordered according to their metric always.
becomes:
DNS interfaces should be ordered according to their route metric always.

Then we are both happy if the route metric reflects the above ordering
Does this sound OK?

This means I need to test on my Linux laptop where I broke the OS - it's a pinebook and the upgrade using KDE neon failed hard. A reflash should fix, but as always that takes time. I've attached a new patch which should work. Be aware that existing route offset values have been updated to start from 1000 rather than 100. All the offsets are now set in route.h rather than magic numbers in the code to make things clearer. The existing route code *should* already change the route metric on before running a hook, but that's not been tested in years.

Hopefully you can test while I refresh my laptop.

Roy
diff --git a/BUILDING.md b/BUILDING.md
index f672724a..2856736c 100644
--- a/BUILDING.md
+++ b/BUILDING.md
@@ -148,3 +148,9 @@ copied to `$(libexecdir)/dhcpcd-hooks` for use.
 The configure program attempts to find hooks for systems you have installed.
 To add more simply
 `./configure -with-hook=ntp.conf`
+
+If using resolvconf, the `20-resolv.conf` hook now requires a version with the
+`-C` and `-c` options to deprecate and activate interfaces to support wireless
+roaming (Linux) or carrier just drops (NetBSD).
+If your resolvconf does not support this then you will see a warning
+about an illegal option when the carrier changes, but things should still work.
diff --git a/hooks/20-resolv.conf b/hooks/20-resolv.conf
index 73d33386..86f4873d 100644
--- a/hooks/20-resolv.conf
+++ b/hooks/20-resolv.conf
@@ -10,6 +10,11 @@ resolv_conf_dir="$state_dir/resolv.conf"
 NL="
 "
 : ${resolvconf:=resolvconf}
+if type "$resolvconf" >/dev/null 2>&1; then
+	have_resolvconf=true
+else
+	have_resolvconf=false
+fi
 
 build_resolv_conf()
 {
@@ -164,7 +169,7 @@ add_resolv_conf()
 	for x in ${new_domain_name_servers}; do
 		conf="${conf}nameserver $x$NL"
 	done
-	if type "$resolvconf" >/dev/null 2>&1; then
+	if $have_resolvconf; then
 		[ -n "$ifmetric" ] && export IF_METRIC="$ifmetric"
 		printf %s "$conf" | "$resolvconf" -a "$ifname"
 		return $?
@@ -180,7 +185,7 @@ add_resolv_conf()
 
 remove_resolv_conf()
 {
-	if type "$resolvconf" >/dev/null 2>&1; then
+	if $have_resolvconf; then
 		"$resolvconf" -d "$ifname" -f
 	else
 		if [ -e "$resolv_conf_dir/$ifname" ]; then
@@ -199,7 +204,11 @@ BOUND6|RENEW6|REBIND6|REBOOT6|INFORM6)
 esac
 
 if $if_configured; then
-	if $if_up || [ "$reason" = ROUTERADVERT ]; then
+	if $have_resolvconf && [ "$reason" = NOCARRIER_ROAMING ]; then
+		"$resolvconf" -C "$interface.*"
+	elif $have_resolvconf && [ "$reason" = CARRIER ]; then
+		"$resolvconf" -c "$interface.*"
+	elif $if_up || [ "$reason" = ROUTERADVERT ]; then
 		add_resolv_conf
 	elif $if_down; then
 		remove_resolv_conf
diff --git a/src/dhcpcd.c b/src/dhcpcd.c
index 3acf72c3..7799277a 100644
--- a/src/dhcpcd.c
+++ b/src/dhcpcd.c
@@ -700,26 +700,7 @@ dhcpcd_nocarrier_roaming(struct interface *ifp)
 {
 
 	loginfox("%s: carrier lost - roaming", ifp->name);
-
-	/*
-	 * XXX We should pass something like NOCARRIER_ROAMING
-	 * and set if_up=true; ifdown=false; so that the hook scripts
-	 * can make a decision to keep or discard the interface information.
-	 *
-	 * Currently they discard it (no carrier after all) which is
-	 * generally fine as new connections won't work and current
-	 * connections try to chug along as best as.
-	 * dhcpcd has been doing this since NetBSD-7 at least.
-	 *
-	 * However, for slow roaming this is poor for say web browsing
-	 * as new lookups will fail quickly giving a poor user experience.
-	 * We should improve this, but the hooks will require some work first
-	 * as we need to introduce a mechanism to sort interfaces by
-	 * carrier > roaming > nocarrier. Then the hooks know in which
-	 * order to apply their data, if at all.
-	 * This probably should be a user toggle.
-	 */
-	script_runreason(ifp, "NOCARRIER");
+	script_runreason(ifp, "NOCARRIER_ROAMING");
 
 #ifdef ARP
 	arp_drop(ifp);
diff --git a/src/if.c b/src/if.c
index deb5280b..d3852f3d 100644
--- a/src/if.c
+++ b/src/if.c
@@ -697,12 +697,11 @@ if_discover(struct dhcpcd_ctx *ctx, struct ifaddrs **ifaddrs,
 			ifp->metric = (unsigned int)ifr.ifr_metric;
 		if_getssid(ifp);
 #else
-		/* We reserve the 100 range for virtual interfaces, if and when
-		 * we can work them out. */
-		ifp->metric = 200 + ifp->index;
+		/* Leave a low portion for user config */
+		ifp->metric = RTMETRIC_BASE + ifp->index;
 		if (if_getssid(ifp) != -1) {
 			ifp->wireless = true;
-			ifp->metric += 100;
+			ifp->metric += RTMETRIC_WIRELESS;
 		}
 #endif
 
diff --git a/src/ipv4ll.c b/src/ipv4ll.c
index 93898109..d5b27c53 100644
--- a/src/ipv4ll.c
+++ b/src/ipv4ll.c
@@ -137,7 +137,7 @@ ipv4ll_defaultroute(rb_tree_t *routes, struct interface *ifp)
 	sa_in_init(&rt->rt_ifa, &state->addr->addr);
 	rt->rt_dflags |= RTDF_IPV4LL;
 #ifdef HAVE_ROUTE_METRIC
-	rt->rt_metric += 10000;
+	rt->rt_metric += RTMETRIC_IPV4LL_OFF;
 #endif
 	return rt_proto_add(routes, rt) ? 1 : 0;
 }
diff --git a/src/route.c b/src/route.c
index 47fd6cf2..71523c14 100644
--- a/src/route.c
+++ b/src/route.c
@@ -168,6 +168,15 @@ rt_compare_proto(void *context, const void *node1, const void *node2)
 	if (c != 0)
 		return -c;
 
+	/* Prefer roaming over non roaming if both carriers are down. */
+	if (ifp1->carrier == LINK_DOWN && ifp2->carrier == LINK_DOWN) {
+		bool roam1 = if_roaming(ifp1);
+		bool roam2 = if_roaming(ifp2);
+
+		if (roam1 != roam2)
+			return roam1 ? 1 : -1;
+	}
+
 #ifdef INET
 	/* IPv4LL routes always come last */
 	if (rt1->rt_dflags & RTDF_IPV4LL && !(rt2->rt_dflags & RTDF_IPV4LL))
@@ -374,6 +383,8 @@ rt_setif(struct rt *rt, struct interface *ifp)
 	rt->rt_ifp = ifp;
 #ifdef HAVE_ROUTE_METRIC
 	rt->rt_metric = ifp->metric;
+	if (if_roaming(ifp))
+		rt->rt_metric += RTMETRIC_ROAM;
 #endif
 }
 
diff --git a/src/route.h b/src/route.h
index e9ac121a..cb935c83 100644
--- a/src/route.h
+++ b/src/route.h
@@ -93,6 +93,15 @@ struct rt {
 #ifdef HAVE_ROUTE_METRIC
 	unsigned int		rt_metric;
 #endif
+/* Maximum interface index is generally USHORT_MAX or 65535.
+ * Add some padding for other stuff and we get offsets for the
+ * below that should work automatically.
+ * This is only an issue if the user defines higher metrics in
+ * their configuration, but then they might wish to override also. */
+#define	RTMETRIC_BASE		   1000U
+#define	RTMETRIC_WIRELESS	   2000U
+#define	RTMETRIC_IPV4LL		1000000U
+#define	RTMETRIC_ROAM		2000000U
 #ifdef HAVE_ROUTE_PREF
 	int			rt_pref;
 #endif
diff --git a/src/script.c b/src/script.c
index 0260845e..bf1e5152 100644
--- a/src/script.c
+++ b/src/script.c
@@ -74,6 +74,9 @@ static const char * const if_params[] = {
 	NULL
 };
 
+static const char * true_str = "true";
+static const char * false_str = "false";
+
 void
 if_printoptions(void)
 {
@@ -228,6 +231,10 @@ make_env(struct dhcpcd_ctx *ctx, const struct interface *ifp,
 	const struct if_options *ifo;
 	const struct interface *ifp2;
 	int af;
+	bool is_stdin = ifp->name[0] == '\0';
+	const char *if_up, *if_down;
+	rb_tree_t ifaces;
+	struct rt *rt;
 #ifdef INET
 	const struct dhcp_state *state;
 #ifdef IPV4LL
@@ -237,7 +244,6 @@ make_env(struct dhcpcd_ctx *ctx, const struct interface *ifp,
 #ifdef DHCP6
 	const struct dhcp6_state *d6_state;
 #endif
-	bool is_stdin = ifp->name[0] == '\0';
 
 #ifdef HAVE_OPEN_MEMSTREAM
 	if (ctx->script_fp == NULL) {
@@ -276,6 +282,7 @@ make_env(struct dhcpcd_ctx *ctx, const struct interface *ifp,
 		if (efprintf(fp, "pid=%d", getpid()) == -1)
 			goto eexit;
 	}
+
 	if (!is_stdin) {
 		if (efprintf(fp, "reason=%s", reason) == -1)
 			goto eexit;
@@ -326,6 +333,7 @@ make_env(struct dhcpcd_ctx *ctx, const struct interface *ifp,
 	else if (strcmp(reason, "PREINIT") == 0 ||
 	    strcmp(reason, "CARRIER") == 0 ||
 	    strcmp(reason, "NOCARRIER") == 0 ||
+	    strcmp(reason, "NOCARRIER_ROAMING") == 0 ||
 	    strcmp(reason, "UNKNOWN") == 0 ||
 	    strcmp(reason, "DEPARTED") == 0 ||
 	    strcmp(reason, "STOPPED") == 0)
@@ -382,34 +390,43 @@ make_env(struct dhcpcd_ctx *ctx, const struct interface *ifp,
 	if (ifp->ctx->options & DHCPCD_DUMPLEASE)
 		goto dumplease;
 
+	rb_tree_init(&ifaces, &rt_compare_proto_ops);
+	TAILQ_FOREACH(ifp2, ifp->ctx->ifaces, next) {
+		rt = rt_new(UNCONST(ifp2));
+		if (rt == NULL)
+			goto eexit;
+		if (rb_tree_insert_node(&ifaces, rt) != rt)
+			goto eexit;
+	}
 	if (fprintf(fp, "interface_order=") == -1)
 		goto eexit;
-	TAILQ_FOREACH(ifp2, ifp->ctx->ifaces, next) {
-		if (ifp2 != TAILQ_FIRST(ifp->ctx->ifaces)) {
-			if (fputc(' ', fp) == EOF)
-				return -1;
-		}
-		if (fprintf(fp, "%s", ifp2->name) == -1)
-			return -1;
+	RB_TREE_FOREACH(rt, &ifaces) {
+		if (rt != RB_TREE_MIN(&ifaces) &&
+		    fprintf(fp, "%s", " ") == -1)
+			goto eexit;
+		if (fprintf(fp, "%s", rt->rt_ifp->name) == -1)
+			goto eexit;
 	}
+	rt_headclear(&ifaces, AF_UNSPEC);
 	if (fputc('\0', fp) == EOF)
-		return -1;
+		goto eexit;
 
 	if (strcmp(reason, "STOPPED") == 0) {
-		if (efprintf(fp, "if_up=false") == -1)
-			goto eexit;
-		if (efprintf(fp, "if_down=%s",
-		    ifo->options & DHCPCD_RELEASE ? "true" : "false") == -1)
-			goto eexit;
+		if_up = false_str;
+		if_down = ifo->options & DHCPCD_RELEASE ? true_str : false_str;
 	} else if (strcmp(reason, "TEST") == 0 ||
 	    strcmp(reason, "PREINIT") == 0 ||
 	    strcmp(reason, "CARRIER") == 0 ||
 	    strcmp(reason, "UNKNOWN") == 0)
 	{
-		if (efprintf(fp, "if_up=false") == -1)
-			goto eexit;
-		if (efprintf(fp, "if_down=false") == -1)
-			goto eexit;
+		if_up = false_str;
+		if_down = false_str;
+	} else if (strcmp(reason, "NOCARRIER") == 0) {
+		if_up = false_str;
+		if_down = true_str;
+	} else if (strcmp(reason, "NOCARRIER_ROAMING") == 0) {
+		if_up = true_str;
+		if_down = false_str;
 	} else if (1 == 2 /* appease ifdefs */
 #ifdef INET
 	    || (protocol == PROTO_DHCP && state && state->new)
@@ -426,16 +443,17 @@ make_env(struct dhcpcd_ctx *ctx, const struct interface *ifp,
 #endif
 	    )
 	{
-		if (efprintf(fp, "if_up=true") == -1)
-			goto eexit;
-		if (efprintf(fp, "if_down=false") == -1)
-			goto eexit;
+		if_up = true_str;
+		if_down = false_str;
 	} else {
-		if (efprintf(fp, "if_up=false") == -1)
-			goto eexit;
-		if (efprintf(fp, "if_down=true") == -1)
-			goto eexit;
+		if_up = false_str;
+		if_down = true_str;
 	}
+	if (efprintf(fp, "if_up=%s", if_up) == -1)
+		goto eexit;
+	if (efprintf(fp, "if_down=%s", if_down) == -1)
+		goto eexit;
+
 	if ((af = dhcpcd_ifafwaiting(ifp)) != AF_MAX) {
 		if (efprintf(fp, "if_afwaiting=%d", af) == -1)
 			goto eexit;

Follow-Ups:
RE: dhcpcd kills all connections on Wi-Fi roaming betweenaccesspointsBoris Krasnovskiy
References:
dhcpcd kills all connections on Wi-Fi roaming between access pointsBoris Krasnovskiy
Re: dhcpcd kills all connections on Wi-Fi roaming between access pointsRoy Marples
Re: dhcpcd kills all connections on Wi-Fi roaming between access pointsBoris Krasnovskiy
Re: dhcpcd kills all connections on Wi-Fi roaming between access pointsRoy Marples
Re: dhcpcd kills all connections on Wi-Fi roaming between access pointsBoris Krasnovskiy
Re: dhcpcd kills all connections on Wi-Fi roaming between access pointsRoy Marples
Re: dhcpcd kills all connections on Wi-Fi roaming between access pointsRoy Marples
RE: dhcpcd kills all connections on Wi-Fi roaming between access pointsbls s
RE: dhcpcd kills all connections on Wi-Fi roaming between accesspointsBoris Krasnovskiy
RE: dhcpcd kills all connections on Wi-Fi roaming between accesspointsbls s
RE: dhcpcd kills all connections on Wi-Fi roaming betweenaccesspointsBoris Krasnovskiy
RE: dhcpcd kills all connections on Wi-Fi roaming betweenaccesspointsbls s
Re: dhcpcd kills all connections on Wi-Fi roaming betweenaccesspointsRoy Marples
Re: dhcpcd kills all connections on Wi-Fi roaming betweenaccesspointsBoris Krasnovskiy
Re: dhcpcd kills all connections on Wi-Fi roaming betweenaccesspointsBoris Krasnovskiy
Archive administrator: postmaster@marples.name