dhcpcd-discuss

On 26/04/2019 16:09, Matt Horan wrote:
> On Thu, Apr 25, 2019 at 11:24:11AM -0400, Matt Horan wrote:
> > On Thu, Apr 25, 2019 at 03:47:45PM +0100, Roy Marples wrote:
> > > On 25/04/2019 15:16, Matt Horan wrote:
> > > > > Does dhcpcd recover from carrier loss?
> > > >
> > > > It doesn't seem so. After the first reboot I waited over a minute and
> > > > dhcpcd never got a lease. When I restarted dhcpcd it got a lease
> > > > immediately.
> > > >
> > > > I do see that dhcpcd forks to the background, so it should still be
> > > > running, but seems to not detect that carrier has returned. IPv4 works
> > > > fine during the entire process, so I believe the interface is still up.
> > >
> > > OK.
> > >
> > > Can you add this to the top of dhcpcd.conf please?
> > >
> > > debug
> > > logfile /var/log/dhcpcd.log
> > >
> > > Reboot and then email me the log - privately if you feel it has
> > > sensitive data.
> >
> > dhcpcd.log and dmesg with route monitor output attached.
>
> Reverting 4f903d6ad4adf2de7712d36a921051b4dfd7210a resolves the issue.
> It seems there must be something funky about the link state?
>
> That might help explain why a subsequent restart of dhcpcd works just
> fine.

OK, I have this fixed I think!
Patch attached.

In a nutshell, the interface reports IFM_AVALID via SIOCGIFMEDIA but 
then doesn't set the link status in RTM_IFNFO.

Let me know how that works for you.

Roy

diff --git a/src/dhcpcd.h b/src/dhcpcd.h
index 24782cbc..3e35a3de 100644
--- a/src/dhcpcd.h
+++ b/src/dhcpcd.h
@@ -85,7 +85,6 @@ struct interface {
 	unsigned short vlanid;
 	unsigned int metric;
 	int carrier;
-	bool media_valid;
 	bool wireless;
 	uint8_t ssid[IF_SSIDLEN];
 	unsigned int ssid_len;
diff --git a/src/if-bsd.c b/src/if-bsd.c
index 20467dfc..dd74c2d7 100644
--- a/src/if-bsd.c
+++ b/src/if-bsd.c
@@ -203,6 +203,28 @@ if_closesockets_os(struct dhcpcd_ctx *ctx)
 		close(priv->pf_inet6_fd);
 }
 
+static int
+if_carrier_flags(struct interface *ifp, unsigned int flags)
+{
+	struct ifmediareq ifmr = { .ifm_status = 0 };
+
+	strlcpy(ifmr.ifm_name, ifp->name, sizeof(ifmr.ifm_name));
+	if (ioctl(ifp->ctx->pf_inet_fd, SIOCGIFMEDIA, &ifmr) == -1 ||
+	    !(ifmr.ifm_status & IFM_AVALID))
+		return flags & IFF_RUNNING ? LINK_UP : LINK_UNKNOWN;
+
+	return (ifmr.ifm_status & IFM_ACTIVE) ? LINK_UP : LINK_DOWN;
+}
+
+int
+if_carrier(struct interface *ifp)
+{
+
+	if (if_getflags(ifp) == -1)
+		return LINK_UNKNOWN;
+	return if_carrier_flags(ifp, ifp->flags);
+}
+
 static void
 if_linkaddr(struct sockaddr_dl *sdl, const struct interface *ifp)
 {
@@ -987,19 +1009,20 @@ if_ifinfo(struct dhcpcd_ctx *ctx, const struct if_msghdr *ifm)
 {
 	struct interface *ifp;
 	int link_state;
+	unsigned int flags;
 
 	if ((ifp = if_findindex(ctx->ifaces, ifm->ifm_index)) == NULL)
 		return;
 
+	flags = (unsigned int)ifm->ifm_flags;
 	switch (ifm->ifm_data.ifi_link_state) {
 	case LINK_STATE_UNKNOWN:
-		if (ifp->media_valid) {
-			link_state = LINK_DOWN;
-			break;
-		}
-		/* Interface does not report media state, so we have
-		 * to rely on IFF_UP. */
-		/* FALLTHROUGH */
+		/* In theory this is only set when an interface is first
+		 * initiaised.
+		 * However whilst some drivers report an active link
+		 * via SIOCGIFMEDIA, they don't bother to announce it
+		 * via a routing message. */
+		link_state = if_carrier_flags(ifp, flags);
 	case LINK_STATE_UP:
 		link_state = ifm->ifm_flags & IFF_UP ? LINK_UP : LINK_DOWN;
 		break;
@@ -1008,8 +1031,7 @@ if_ifinfo(struct dhcpcd_ctx *ctx, const struct if_msghdr *ifm)
 		break;
 	}
 
-	dhcpcd_handlecarrier(ctx, link_state,
-	    (unsigned int)ifm->ifm_flags, ifp->name);
+	dhcpcd_handlecarrier(ctx, link_state, flags, ifp->name);
 }
 
 static void
diff --git a/src/if.c b/src/if.c
index efc4314c..28597dc2 100644
--- a/src/if.c
+++ b/src/if.c
@@ -126,65 +126,37 @@ if_closesockets(struct dhcpcd_ctx *ctx)
 }
 
 int
-if_carrier(struct interface *ifp)
+if_getflags(struct interface *ifp)
 {
-	int r;
-	struct ifreq ifr;
-#ifdef SIOCGIFMEDIA
-	struct ifmediareq ifmr;
-#endif
+	struct ifreq ifr = { .ifr_flags = 0 };
 
-	memset(&ifr, 0, sizeof(ifr));
 	strlcpy(ifr.ifr_name, ifp->name, sizeof(ifr.ifr_name));
-	r = ioctl(ifp->ctx->pf_inet_fd, SIOCGIFFLAGS, &ifr);
-	if (r != -1)
-		ifp->flags = (unsigned int)ifr.ifr_flags;
-
-#ifdef __sun
-	return if_carrier_os(ifp);
-#else
-	if (r == -1)
-		return LINK_UNKNOWN;
-
-#ifdef SIOCGIFMEDIA
-	memset(&ifmr, 0, sizeof(ifmr));
-	strlcpy(ifmr.ifm_name, ifp->name, sizeof(ifmr.ifm_name));
-	if (ioctl(ifp->ctx->pf_inet_fd, SIOCGIFMEDIA, &ifmr) != -1 &&
-	    ifmr.ifm_status & IFM_AVALID)
-	{
-		ifp->media_valid = true;
-		r = (ifmr.ifm_status & IFM_ACTIVE) ? LINK_UP : LINK_DOWN;
-	} else {
-		ifp->media_valid = false;
-		r = ifr.ifr_flags & IFF_RUNNING ? LINK_UP : LINK_UNKNOWN;
-	}
-#else
-	r = ifr.ifr_flags & IFF_RUNNING ? LINK_UP : LINK_DOWN;
-#endif
-#endif /* __sun */
-	return r;
+	if (ioctl(ifp->ctx->pf_inet_fd, SIOCGIFFLAGS, &ifr) == -1)
+		return -1;
+	ifp->flags = (unsigned int)ifr.ifr_flags;
+	return 0;
 }
 
 int
 if_setflag(struct interface *ifp, short flag)
 {
-	struct ifreq ifr;
-	int r;
+	struct ifreq ifr = { .ifr_flags = 0 };
+	short f;
+
+	if (if_getflags(ifp) == -1)
+		return -1;
+
+	f = (short)ifp->flags;
+	if ((f & flag) == flag)
+		return 0;
 
-	memset(&ifr, 0, sizeof(ifr));
 	strlcpy(ifr.ifr_name, ifp->name, sizeof(ifr.ifr_name));
-	r = -1;
-	if (ioctl(ifp->ctx->pf_inet_fd, SIOCGIFFLAGS, &ifr) == 0) {
-		if (flag == 0 || (ifr.ifr_flags & flag) == flag)
-			r = 0;
-		else {
-			ifr.ifr_flags |= flag;
-			if (ioctl(ifp->ctx->pf_inet_fd, SIOCSIFFLAGS, &ifr) ==0)
-				r = 0;
-		}
-		ifp->flags = (unsigned int)ifr.ifr_flags;
-	}
-	return r;
+	ifr.ifr_flags = f | flag;
+	if (ioctl(ifp->ctx->pf_inet_fd, SIOCSIFFLAGS, &ifr) == -1)
+		return -1;
+
+	ifp->flags = (unsigned int)ifr.ifr_flags;
+	return 0;
 }
 
 static int
diff --git a/src/if.h b/src/if.h
index f10a8c37..91bba49b 100644
--- a/src/if.h
+++ b/src/if.h
@@ -111,6 +111,7 @@ int if_getifaddrs(struct ifaddrs **);
 #define	getifaddrs	if_getifaddrs
 #endif
 
+int if_getflags(struct interface *ifp);
 int if_setflag(struct interface *ifp, short flag);
 #define if_up(ifp) if_setflag((ifp), (IFF_UP | IFF_RUNNING))
 bool if_valid_hwaddr(const uint8_t *, size_t);
@@ -133,7 +134,6 @@ int if_carrier(struct interface *);
 int if_makealias(char *, size_t, const char *, int);
 #endif
 
-int if_carrier_os(struct interface *);
 int if_mtu_os(const struct interface *);
 
 /*