# HG changeset patch # User Roy Marples # Date 1601309378 -3600 # Node ID 8e2b8ce8c9724ac5a4904ea7ac001532599cab2f # Parent 6792cb73e0212676e1b75593550d89ebdc7362b5 BSD: struct if_data->ifi_link_state is the single source of truth Vastly improve and simplify link detection on BSD. dhcpcd either examines the whole system via getifaddrs(3) or reacts to events via route(4). In both cases we have struct if_data which has ifi_link_state. Armed with this knowledge, we no longer need SIOCGIFDATA or SIOCGIFMEDIA. To solve the issue of newly attached interfaces having LINK_STATE_UNKNOWN or some interfaces not even changing it, we only change the local knowledge of interface flags when reports them by getifaddrs(3) or route(4) when we change them. For example, if we set IFF_UP and it succeeds we don't set this internally until reported by the kernel as above. This keeps flags and link state in sync with each other. The hope is that the kernel can set the real link state before it reports IFF_UP. As such, we no longer require the poll option or need to enter a tight loop for old interfaces. diff -r 6792cb73e021 -r 8e2b8ce8c972 src/dhcpcd.c --- a/src/dhcpcd.c Sun Sep 27 11:28:03 2020 +0100 +++ b/src/dhcpcd.c Mon Sep 28 17:09:38 2020 +0100 @@ -432,8 +432,6 @@ /* De-activate the interface */ ifp->active = IF_INACTIVE; ifp->options->options &= ~DHCPCD_STOPPING; - /* Set the link state to unknown as we're no longer tracking it. */ - ifp->carrier = LINK_UNKNOWN; if (!(ctx->options & (DHCPCD_MASTER | DHCPCD_TEST))) eloop_exit(ctx->eloop, EXIT_FAILURE); @@ -704,42 +702,32 @@ } void -dhcpcd_handlecarrier(struct dhcpcd_ctx *ctx, int carrier, unsigned int flags, - const char *ifname) +dhcpcd_handlecarrier(struct interface *ifp, int carrier, unsigned int flags) { - struct interface *ifp; - - ifp = if_find(ctx->ifaces, ifname); - if (ifp == NULL || - ifp->options == NULL || !(ifp->options->options & DHCPCD_LINK)) - return; + bool nolink = ifp->options == NULL || + !(ifp->options->options & DHCPCD_LINK); + ifp->flags = flags; if (carrier == LINK_UNKNOWN) { - if (ifp->wireless) { + if (ifp->wireless) carrier = LINK_DOWN; - ifp->flags = flags; - } else - carrier = if_carrier(ifp); - } else - ifp->flags = flags; - if (carrier == LINK_UNKNOWN) - carrier = IF_UPANDRUNNING(ifp) ? LINK_UP : LINK_DOWN; + else + carrier = IF_UPANDRUNNING(ifp) ? LINK_UP : LINK_DOWN; + } if (carrier == LINK_DOWN || (ifp->flags & IFF_UP) == 0) { if (ifp->carrier != LINK_DOWN) { - int oldcarrier = ifp->carrier; - #ifdef NOCARRIER_PRESERVE_IP if (ifp->flags & IFF_UP && - !(ifp->options->options & DHCPCD_ANONYMOUS)) + (ifp->options == NULL || + !(ifp->options->options & DHCPCD_ANONYMOUS))) ifp->carrier = LINK_DOWN_IFFUP; else #endif ifp->carrier = LINK_DOWN; - if (!ifp->active) + if (!ifp->active || nolink) return; - if (oldcarrier == LINK_UP) - loginfox("%s: carrier lost", ifp->name); + loginfox("%s: carrier lost", ifp->name); script_runreason(ifp, "NOCARRIER"); #ifdef NOCARRIER_PRESERVE_IP if (ifp->flags & IFF_UP && @@ -801,7 +789,7 @@ #endif } } - if (!ifp->active) + if (!ifp->active || nolink) return; dhcpcd_initstate(ifp, 0); script_runreason(ifp, "CARRIER"); @@ -878,20 +866,11 @@ struct interface *ifp = arg; struct if_options *ifo = ifp->options; - if (ifo->options & DHCPCD_LINK) { - switch (ifp->carrier) { - case LINK_UP: - break; - case LINK_DOWN: - loginfox("%s: waiting for carrier", ifp->name); - return; - case LINK_UNKNOWN: - /* No media state available. - * Loop until both IFF_UP and IFF_RUNNING are set */ - if (ifo->poll == 0) - if_pollinit(ifp); - return; - } + if (ifo->options & DHCPCD_LINK && (ifp->carrier == LINK_DOWN || + (ifp->carrier == LINK_UNKNOWN && !IF_UPANDRUNNING(ifp)))) + { + loginfox("%s: waiting for carrier", ifp->name); + return; } if (ifo->options & (DHCPCD_DUID | DHCPCD_IPV6) && @@ -1000,9 +979,6 @@ logerr(__func__); } - if (ifp->options->poll != 0) - if_pollinit(ifp); - dhcpcd_startinterface(ifp); } @@ -1138,14 +1114,14 @@ static void dhcpcd_checkcarrier(void *arg) { - struct interface *ifp = arg; - int carrier; + struct interface *ifp0 = arg, *ifp; - /* Check carrier here rather than setting LINK_UNKNOWN. - * This is because we force LINK_UNKNOWN as down for wireless which - * we do not want when dealing with a route socket overflow. */ - carrier = if_carrier(ifp); - dhcpcd_handlecarrier(ifp->ctx, carrier, ifp->flags, ifp->name); + ifp = if_find(ifp0->ctx->ifaces, ifp0->name); + if (ifp == NULL || ifp->carrier == ifp0->carrier) + return; + + dhcpcd_handlecarrier(ifp, ifp0->carrier, ifp0->flags); + if_free(ifp0); } #ifndef SMALL @@ -1231,10 +1207,10 @@ ifp1 = if_find(ctx->ifaces, ifp->name); if (ifp1 != NULL) { /* If the interface already exists, - * check carrier state. */ + * check carrier state. + * dhcpcd_checkcarrier will free ifp. */ eloop_timeout_add_sec(ctx->eloop, 0, - dhcpcd_checkcarrier, ifp1); - if_free(ifp); + dhcpcd_checkcarrier, ifp); continue; } TAILQ_INSERT_TAIL(ctx->ifaces, ifp, next); diff -r 6792cb73e021 -r 8e2b8ce8c972 src/dhcpcd.conf.5.in --- a/src/dhcpcd.conf.5.in Sun Sep 27 11:28:03 2020 +0100 +++ b/src/dhcpcd.conf.5.in Mon Sep 28 17:09:38 2020 +0100 @@ -24,7 +24,7 @@ .\" OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF .\" SUCH DAMAGE. .\" -.Dd September 15, 2020 +.Dd September 28, 2020 .Dt DHCPCD.CONF 5 .Os .Sh NAME @@ -541,12 +541,6 @@ You should only set this for buggy interface drivers. .It Ic noup Don't bring the interface up when in master mode. -If -.Nm -cannot determine the carrier state, -.Nm -will enter a tight polling loop until the interface is marked up and running -or a valid carrier state is reported. .It Ic option Ar option Requests the .Ar option @@ -604,12 +598,6 @@ detects an address added to a point to point interface (PPP, TUN, etc) then it will set the listed DHCP options to the destination address of the interface. -.It Ic poll Op Ar time -Polls the interface every -.Ar time -milliseconds (default of 100) to check flags and carrier status. -This option should only be used if the driver does not report link state -changes but can report the link state. .It Ic profile Ar name Subsequent options are only parsed for this profile .Ar name . diff -r 6792cb73e021 -r 8e2b8ce8c972 src/dhcpcd.h --- a/src/dhcpcd.h Sun Sep 27 11:28:03 2020 +0100 +++ b/src/dhcpcd.h Mon Sep 28 17:09:38 2020 +0100 @@ -271,7 +271,7 @@ void dhcpcd_linkoverflow(struct dhcpcd_ctx *); int dhcpcd_handleargs(struct dhcpcd_ctx *, struct fd_list *, int, char **); -void dhcpcd_handlecarrier(struct dhcpcd_ctx *, int, unsigned int, const char *); +void dhcpcd_handlecarrier(struct interface *, int, unsigned int); int dhcpcd_handleinterface(void *, int, const char *); void dhcpcd_handlehwaddr(struct interface *, uint16_t, const void *, uint8_t); void dhcpcd_dropinterface(struct interface *, const char *); diff -r 6792cb73e021 -r 8e2b8ce8c972 src/if-bsd.c --- a/src/if-bsd.c Sun Sep 27 11:28:03 2020 +0100 +++ b/src/if-bsd.c Mon Sep 28 17:09:38 2020 +0100 @@ -377,84 +377,23 @@ return ioctl(ctx->pf_inet_fd, cmd, &ifr); } -static int -if_carrier0(struct interface *ifp) +int +if_carrier(__unused struct interface *ifp, const void *ifadata) { - struct ifmediareq ifmr = { .ifm_status = 0 }; - - /* Not really needed, but the other OS update flags here also */ - if (if_getflags(ifp) == -1) - return LINK_UNKNOWN; - - strlcpy(ifmr.ifm_name, ifp->name, sizeof(ifmr.ifm_name)); - if (ioctl(ifp->ctx->pf_inet_fd, SIOCGIFMEDIA, &ifmr) == -1) - return LINK_UNKNOWN; - - if (!(ifmr.ifm_status & IFM_AVALID)) - return LINK_UNKNOWN; - - return (ifmr.ifm_status & IFM_ACTIVE) ? LINK_UP : LINK_DOWN; -} - -int -if_carrier(struct interface *ifp) -{ - int carrier = if_carrier0(ifp); - -#ifdef SIOCGIFDATA - if (carrier != LINK_UNKNOWN) - return carrier; - -#ifdef __NetBSD__ - struct ifdatareq ifdr = { .ifdr_data.ifi_link_state = 0 }; - struct if_data *ifdata = &ifdr.ifdr_data; - - strlcpy(ifdr.ifdr_name, ifp->name, sizeof(ifdr.ifdr_name)); - if (ioctl(ifp->ctx->pf_inet_fd, SIOCGIFDATA, &ifdr) == -1) - return LINK_UNKNOWN; -#else - struct if_data ifd = { .ifi_link_state = 0 }; - struct if_data *ifdata = &ifd; + const struct if_data *ifi = ifadata; - if (if_indirect_ioctl(ifp->ctx, ifp->name, SIOCGIFDATA, - &ifd, sizeof(ifd)) == -1) - return LINK_UNKNOWN; -#endif + /* + * Every BSD returns this and it is the sole source of truth. + * Not all BSD's support SIOCGIFDATA and not all interfaces + * support SIOCGIFMEDIA. + */ + assert(ifadata != NULL); - switch (ifdata->ifi_link_state) { - case LINK_STATE_DOWN: - return LINK_DOWN; -#ifdef LINK_STATE_HALF_DUPLEX - case LINK_STATE_HALF_DUPLEX: - case LINK_STATE_FULL_DUPLEX: -#endif - case LINK_STATE_UP: + if (ifi->ifi_link_state >= LINK_STATE_UP) return LINK_UP; - } - return LINK_UNKNOWN; -#else -#warning no SIOCGIFDATA - not all interfaces support SIOCGIFMEDIA - return carrier; -#endif -} - -int -if_carrier_ifadata(struct interface *ifp, void *ifadata) -{ - int carrier = if_carrier0(ifp); - struct if_data *ifdata; - - if (carrier != LINK_UNKNOWN || ifadata == NULL) - return carrier; - - ifdata = ifadata; - switch (ifdata->ifi_link_state) { - case LINK_STATE_DOWN: - return LINK_DOWN; - case LINK_STATE_UP: - return LINK_UP; - } - return LINK_UNKNOWN; + if (ifi->ifi_link_state == LINK_STATE_UNKNOWN) + return LINK_UNKNOWN; + return LINK_DOWN; } static void @@ -1254,24 +1193,8 @@ if ((ifp = if_findindex(ctx->ifaces, ifm->ifm_index)) == NULL) return 0; - switch (ifm->ifm_data.ifi_link_state) { - case LINK_STATE_UNKNOWN: - link_state = LINK_UNKNOWN; - break; -#ifdef LINK_STATE_FULL_DUPLEX - case LINK_STATE_HALF_DUPLEX: /* FALLTHROUGH */ - case LINK_STATE_FULL_DUPLEX: /* FALLTHROUGH */ -#endif - case LINK_STATE_UP: - link_state = LINK_UP; - break; - default: - link_state = LINK_DOWN; - break; - } - - dhcpcd_handlecarrier(ctx, link_state, - (unsigned int)ifm->ifm_flags, ifp->name); + link_state = if_carrier(ifp, &ifm->ifm_data); + dhcpcd_handlecarrier(ifp, link_state, (unsigned int)ifm->ifm_flags); return 0; } diff -r 6792cb73e021 -r 8e2b8ce8c972 src/if-linux.c --- a/src/if-linux.c Sun Sep 27 11:28:03 2020 +0100 +++ b/src/if-linux.c Mon Sep 28 17:09:38 2020 +0100 @@ -509,22 +509,13 @@ } int -if_carrier(struct interface *ifp) +if_carrier(struct interface *ifp, __unused const void *ifadata) { - if (if_getflags(ifp) == -1) - return LINK_UNKNOWN; return ifp->flags & IFF_RUNNING ? LINK_UP : LINK_DOWN; } int -if_carrier_ifadata(struct interface *ifp, __unused void *ifadata) -{ - - return if_carrier(ifp); -} - -int if_getnetlink(struct dhcpcd_ctx *ctx, struct iovec *iov, int fd, int flags, int (*cb)(struct dhcpcd_ctx *, void *, struct nlmsghdr *), void *cbarg) { @@ -1018,9 +1009,9 @@ dhcpcd_handlehwaddr(ifp, ifi->ifi_type, hwa, hwl); } - dhcpcd_handlecarrier(ctx, + dhcpcd_handlecarrier(ifp, ifi->ifi_flags & IFF_RUNNING ? LINK_UP : LINK_DOWN, - ifi->ifi_flags, ifn); + ifi->ifi_flags); return 0; } diff -r 6792cb73e021 -r 8e2b8ce8c972 src/if-options.c --- a/src/if-options.c Sun Sep 27 11:28:03 2020 +0100 +++ b/src/if-options.c Mon Sep 28 17:09:38 2020 +0100 @@ -165,7 +165,6 @@ {"inactive", no_argument, NULL, O_INACTIVE}, {"mudurl", required_argument, NULL, O_MUDURL}, {"link_rcvbuf", required_argument, NULL, O_LINK_RCVBUF}, - {"poll", optional_argument, NULL, O_POLL}, {NULL, 0, NULL, '\0'} }; @@ -2235,18 +2234,6 @@ } #endif break; - case O_POLL: - if (arg == NULL) { - ifo->poll = IF_POLL_UP; - break; - } - ifo->poll = (unsigned long) - strtou(arg, NULL, 0, 0, ULONG_MAX, &e); - if (e) { - logerrx("failed to convert poll %s", arg); - return -1; - } - break; default: return 0; } diff -r 6792cb73e021 -r 8e2b8ce8c972 src/if-options.h --- a/src/if-options.h Sun Sep 27 11:28:03 2020 +0100 +++ b/src/if-options.h Mon Sep 28 17:09:38 2020 +0100 @@ -180,7 +180,6 @@ #define O_INACTIVE O_BASE + 47 #define O_MUDURL O_BASE + 48 #define O_MSUSERCLASS O_BASE + 49 -#define O_POLL O_BASE + 50 extern const struct option cf_options[]; @@ -216,7 +215,6 @@ time_t mtime; uint8_t iaid[4]; int metric; - unsigned long poll; uint8_t requestmask[256 / NBBY]; uint8_t requiremask[256 / NBBY]; uint8_t nomask[256 / NBBY]; diff -r 6792cb73e021 -r 8e2b8ce8c972 src/if-sun.c --- a/src/if-sun.c Sun Sep 27 11:28:03 2020 +0100 +++ b/src/if-sun.c Mon Sep 28 17:09:38 2020 +0100 @@ -203,7 +203,7 @@ } int -if_carrier(struct interface *ifp) +if_carrier(struct interface *ifp, __unused const void *ifadata) { kstat_ctl_t *kcp; kstat_t *ksp; @@ -246,13 +246,6 @@ } int -if_carrier_ifadata(struct interface *ifp, __unused void *ifadata) -{ - - return if_carrier(ifp); -} - -int if_mtu_os(const struct interface *ifp) { dlpi_handle_t dh; @@ -1065,7 +1058,7 @@ state = LINK_UP; flags |= IFF_UP; } - dhcpcd_handlecarrier(ctx, state, flags, ifp->name); + dhcpcd_handlecarrier(ifp, state, flags); return 0; } diff -r 6792cb73e021 -r 8e2b8ce8c972 src/if.c --- a/src/if.c Sun Sep 27 11:28:03 2020 +0100 +++ b/src/if.c Mon Sep 28 17:09:38 2020 +0100 @@ -185,7 +185,11 @@ if_ioctl(ifp->ctx, SIOCSIFFLAGS, &ifr, sizeof(ifr)) == -1) return -1; - ifp->flags = (unsigned int)ifr.ifr_flags; + /* + * Do NOT set ifp->flags here. + * We need to listen for flag updates from the kernel as they + * need to sync with carrier. + */ return 0; } @@ -687,37 +691,13 @@ #endif ifp->active = active; - ifp->carrier = if_carrier_ifadata(ifp, ifa->ifa_data); + ifp->carrier = if_carrier(ifp, ifa->ifa_data); TAILQ_INSERT_TAIL(ifs, ifp, next); } return ifs; } -static void -if_poll(void *arg) -{ - struct interface *ifp = arg; - unsigned int flags = ifp->flags; - int carrier; - - carrier = if_carrier(ifp); /* if_carrier will update ifp->flags */ - if (ifp->carrier != carrier || ifp->flags != flags) - dhcpcd_handlecarrier(ifp->ctx, carrier, ifp->flags, ifp->name); - - if (ifp->options->poll != 0 || ifp->carrier != LINK_UP) - if_pollinit(ifp); -} - -int -if_pollinit(struct interface *ifp) -{ - unsigned long msec; - - msec = ifp->options->poll != 0 ? ifp->options->poll : IF_POLL_UP; - return eloop_timeout_add_msec(ifp->ctx->eloop, msec, if_poll, ifp); -} - /* * eth0.100:2 OR eth0i100:2 (seems to be NetBSD xvif(4) only) * diff -r 6792cb73e021 -r 8e2b8ce8c972 src/if.h --- a/src/if.h Sun Sep 27 11:28:03 2020 +0100 +++ b/src/if.h Mon Sep 28 17:09:38 2020 +0100 @@ -159,9 +159,7 @@ int if_domtu(const struct interface *, short int); #define if_getmtu(ifp) if_domtu((ifp), 0) #define if_setmtu(ifp, mtu) if_domtu((ifp), (mtu)) -int if_carrier(struct interface *); -int if_carrier_ifadata(struct interface *, void *); -int if_pollinit(struct interface *ifp); +int if_carrier(struct interface *, const void *); #ifdef ALIAS_ADDR int if_makealias(char *, size_t, const char *, int);