Re: regression w/latest dhcpcd?

[Roy Marples <roy%marples.name@localhost>]

On 01/05/2019 13:55, John D. Baker wrote:
> On Wed, 1 May 2019, Roy Marples wrote:
>
> > On 01/05/2019 03:44, John D. Baker wrote:
> >
> > > I've now tried it and it works as expected.
> >
> > OK, this might be tricky to solve.
> > Can I get the output of route montior captured during early boot please?
>
> To be clear, your asking about a 'dhcpcd' binary built without
> reverting the commit you mentioned, correct?
>
> > When it fails, is the interface marked IFF_UP?
>
> Before reverting that commit, the output of 'ifconfig le0' showed it
> marked "UP".
>
> Watching the logfiles on the DHCP server showed no requests coming in
> from that host when booting from disk.  When booting from network, I
> could see all three phases (bootloader, kernel, "/etc/rc.d/network"
> ['dhcpcd']).

OK. I strongly suspect this is because the driver reports IFM_AVALID via 
SIOCGIFMEDIA but then doesn't announce link state changes via route(4).
This is very much driver dependant sadly and we have no way of knowing
this little detail. Basically this is a poor driver :(

The attached patch works around this by calling SIOCGIFMEDIA when 
RTM_IFINFO reports an unknown link status.

Let me know how it 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 *);
 
 /*