Skip site navigation (1)Skip section navigation (2)
Date:      Fri, 17 Aug 2012 15:03:06 -0400
From:      John Baldwin <jhb@freebsd.org>
To:        Ian Lepore <freebsd@damnhippie.dyndns.org>
Cc:        svn-src-head@freebsd.org, svn-src-all@freebsd.org, src-committers@freebsd.org
Subject:   Re: svn commit: r239356 - head/sbin/dhclient
Message-ID:  <201208171503.06203.jhb@freebsd.org>
In-Reply-To: <1345226994.27688.129.camel@revolution.hippie.lan>
References:  <201208171553.q7HFrhuf090457@svn.freebsd.org> <201208171331.10655.jhb@freebsd.org> <1345226994.27688.129.camel@revolution.hippie.lan>

next in thread | previous in thread | raw e-mail | index | archive | help
On Friday, August 17, 2012 2:09:54 pm Ian Lepore wrote:
> On Fri, 2012-08-17 at 13:31 -0400, John Baldwin wrote:
> > On Friday, August 17, 2012 1:02:14 pm Ian Lepore wrote:
> > > On Fri, 2012-08-17 at 15:53 +0000, John Baldwin wrote:
> > > > Author: jhb
> > > > Date: Fri Aug 17 15:53:43 2012
> > > > New Revision: 239356
> > > > URL: http://svn.freebsd.org/changeset/base/239356
> > > > 
> > > > Log:
> > > >   Fix dhclient to properly exit and teardown the configured lease when
> > > >   link is lost.  devd will start a new dhclient instance when link is
> > > >   restored.
> > > >   
> > > >   PR:		bin/166656
> > > >   Submitted by:	Peter Jeremy (mostly)
> > > >   Reviewed by:	brooks (earlier version from Peter)
> > > >   MFC after:	1 month
> > > > 
> > > > Modified:
> > > >   head/sbin/dhclient/dhclient.c
> > > > 
> > > > Modified: head/sbin/dhclient/dhclient.c
> > > > ==============================================================================
> > > > --- head/sbin/dhclient/dhclient.c	Fri Aug 17 14:22:56 2012	(r239355)
> > > > +++ head/sbin/dhclient/dhclient.c	Fri Aug 17 15:53:43 2012	(r239356)
> > > > @@ -278,6 +278,11 @@ routehandler(struct protocol *p)
> > > >  			    ifi->name);
> > > >  			goto die;
> > > >  		}
> > > > +		if (!interface_link_status(ifi->name)) {
> > > > +			warning("Interface %s is down, dhclient exiting",
> > > > +			    ifi->name);
> > > > +			goto die;
> > > > +		}
> > > >  		break;
> > > >  	case RTM_IFANNOUNCE:
> > > >  		ifan = (struct if_announcemsghdr *)rtm;
> > > > @@ -316,6 +321,8 @@ routehandler(struct protocol *p)
> > > >  
> > > >  die:
> > > >  	script_init("FAIL", NULL);
> > > > +	if (ifi->client->active)
> > > > +		script_write_params("old_", ifi->client->active);
> > > >  	if (ifi->client->alias)
> > > >  		script_write_params("alias_", ifi->client->alias);
> > > >  	script_go();
> > > 
> > > I think the attached patch should give the same result without needing
> > > to create/destroy a socket to check the link status every time a routing
> > > info message arrives.  I've actually had this patch in my head for
> > > several years, I just hadn't gotten around to submitting it yet.
> > 
> > Hmm, OpenBSD does check that, but they also seem to verify it via SIOCGMEDIA
> > as well.  Do we think this is as reliable?  In the kernel we only seem to
> > honor this if the NIC explicitly supports the capability:
> > 
> > #define RT_LINK_IS_UP(ifp)	(!((ifp)->if_capabilities & IFCAP_LINKSTATE) \
> > 				 || (ifp)->if_link_state == LINK_STATE_UP)
> > 
> > Also, I don't think routing info messages are all that common of an event are they?
> > 
> 
> I actually don't know how common routing info messages are; the ways I
> use freebsd don't include a lot of heavy network usage.

They appear to be rare:

Finding symbol: rt_ifmsg

Database directory: /home/jhb/work/freebsd/svn/head/sys/
-------------------------------------------------------------------------------
*** net/route.h:
<global>[363]                  void rt_ifmsg(struct ifnet *);

*** dev/usb/usb_pf.c:
usbpf_clone_create[202]        rt_ifmsg(ifp);

*** net/if.c:
if_unroute[1841]               rt_ifmsg(ifp);
if_route[1863]                 rt_ifmsg(ifp);
do_link_state_change[1903]     rt_ifmsg(ifp);
ifhwioctl[2293]                rt_ifmsg(ifp);
if_setflag[2678]               rt_ifmsg(ifp);

*** net/rtsock.c:
rt_ifmsg[1256]                 rt_ifmsg(struct ifnet *ifp)

So, adding or removing routes, link state changes, changing the MTU, and
changing an interface flag (e.g. ifconfig down).  These all seem to be
fairly rare to me, and generally require running ifconfig or route, etc.

> The IFCAP_LINKSTATE question is interesting.  The #define comment for it
> says "the runtime link state is dynamic."  The RT_LINK_IS_UP() macro is
> interesting too, in that it appears to assume that the link must be up
> if it isn't dynamic.  I think that means the more-correct way than I
> posted would be a test such as
> 
>   if (!RT_LINK_IS_UP(&ifm->ifm_data))
> 
> It looks like IFCAP_LINKSTATE is added to if_capabilities by
> miibus_attach() and a few specific NIC drivers (presumably ones that
> don't use standard MII attachements to their PHYs).
> 
> I would hope all that adds up to the concept that any network driver in
> control of links that can come and go will either automatically do the
> right thing by using the miibus code, or by using its own equivelent
> code when mii isn't appropriate, or the driver is buggy.  Hopefully
> there aren't any in the last category. :)

I think pseudo-interfaces complicate things somewhat, but many of those
(vlan(4), bridge(4), etc.) appear to do something sane I think.  It's not
clear to me that we currently assume that ifi_link_state is definitively
valid.

-- 
John Baldwin



Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?201208171503.06203.jhb>