From owner-freebsd-hackers@FreeBSD.ORG Fri Jun 3 13:14:49 2011 Return-Path: Delivered-To: freebsd-hackers@FreeBSD.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:4f8:fff6::34]) by hub.freebsd.org (Postfix) with ESMTP id 683B2106566C; Fri, 3 Jun 2011 13:14:49 +0000 (UTC) (envelope-from glebius@FreeBSD.org) Received: from cell.glebius.int.ru (glebius.int.ru [81.19.64.117]) by mx1.freebsd.org (Postfix) with ESMTP id DCAF18FC08; Fri, 3 Jun 2011 13:14:48 +0000 (UTC) Received: from cell.glebius.int.ru (localhost [127.0.0.1]) by cell.glebius.int.ru (8.14.4/8.14.4) with ESMTP id p53DEk6v017449; Fri, 3 Jun 2011 17:14:46 +0400 (MSD) (envelope-from glebius@FreeBSD.org) Received: (from glebius@localhost) by cell.glebius.int.ru (8.14.4/8.14.4/Submit) id p53DEkPo017448; Fri, 3 Jun 2011 17:14:46 +0400 (MSD) (envelope-from glebius@FreeBSD.org) X-Authentication-Warning: cell.glebius.int.ru: glebius set sender to glebius@FreeBSD.org using -f Date: Fri, 3 Jun 2011 17:14:46 +0400 From: Gleb Smirnoff To: John Baldwin Message-ID: <20110603131446.GF12033@glebius.int.ru> References: <4DE7B935.9040004@aon.at> <201106030804.23084.jhb@freebsd.org> MIME-Version: 1.0 Content-Type: text/plain; charset=koi8-r Content-Disposition: inline In-Reply-To: <201106030804.23084.jhb@freebsd.org> User-Agent: Mutt/1.5.21 (2010-09-15) Cc: freebsd-hackers@FreeBSD.org, Martin Birgmeier Subject: Re: some strange constructs (bugs?) in if_tun.c X-BeenThere: freebsd-hackers@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: Technical Discussions relating to FreeBSD List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 03 Jun 2011 13:14:49 -0000 On Fri, Jun 03, 2011 at 08:04:22AM -0400, John Baldwin wrote: J> On Thursday, June 02, 2011 12:24:21 pm Martin Birgmeier wrote: J> > I am looking at net/if_tun.c, function tunwrite() (this is 7.4, but 8.2 J> > is nearly the same): J> > J> > There is a local variable "error" which is initialized to zero and then J> > seemingly never changed, until it is used as a return value if J> > m_uiotombuf() fails: J> > J> > ... J> > int error = 0; J> > ... J> > if ((m = m_uiotombuf(uio, M_DONTWAIT, 0, 0, M_PKTHDR)) == NULL) { J> > ifp->if_ierrors++; J> > return (error); J> > } J> > ... J> > a little further down, we see J> > ... J> > if (m->m_len < sizeof(family) && J> > (m = m_pullup(m, sizeof(family))) == NULL) J> > return (ENOBUFS); J> > ... J> > J> > As far as I can see, the first return amounts to "drop the packet, but J> > don't tell anything about it", whereas the second amounts to "drop the J> > packet and say it's due to ENOBUFS". J> > J> > However, the first case is much more like ENOBUFS, so shouldn't we J> > simply say "return (ENOBUFS)" there and remove the "error" variable J> > altogether? J> J> Yes, this error seems to have been introduced in 137101 when if_tun was J> switched to use m_uiotombuf() rather than a home-rolled version. tap(4) had J> the same bug, but it was fixed in 163986. I think this patch should be ok for J> tun(4): J> J> Index: if_tun.c J> =================================================================== J> --- if_tun.c (revision 222565) J> +++ if_tun.c (working copy) J> @@ -126,7 +126,7 @@ static void tunclone(void *arg, struct ucred *cred J> int namelen, struct cdev **dev); J> static void tuncreate(const char *name, struct cdev *dev); J> static int tunifioctl(struct ifnet *, u_long, caddr_t); J> -static int tuninit(struct ifnet *); J> +static void tuninit(struct ifnet *); J> static int tunmodevent(module_t, int, void *); J> static int tunoutput(struct ifnet *, struct mbuf *, struct sockaddr *, J> struct route *ro); J> @@ -494,14 +494,13 @@ tunclose(struct cdev *dev, int foo, int bar, struc J> return (0); J> } J> J> -static int J> +static void J> tuninit(struct ifnet *ifp) J> { J> struct tun_softc *tp = ifp->if_softc; J> #ifdef INET J> struct ifaddr *ifa; J> #endif J> - int error = 0; J> J> TUNDEBUG(ifp, "tuninit\n"); J> J> @@ -528,7 +527,6 @@ tuninit(struct ifnet *ifp) J> if_addr_runlock(ifp); J> #endif J> mtx_unlock(&tp->tun_mtx); J> - return (error); J> } J> J> /* J> @@ -552,12 +550,12 @@ tunifioctl(struct ifnet *ifp, u_long cmd, caddr_t J> mtx_unlock(&tp->tun_mtx); J> break; J> case SIOCSIFADDR: J> - error = tuninit(ifp); J> - TUNDEBUG(ifp, "address set, error=%d\n", error); J> + tuninit(ifp); J> + TUNDEBUG(ifp, "address set\n"); J> break; J> case SIOCSIFDSTADDR: J> - error = tuninit(ifp); J> - TUNDEBUG(ifp, "destination address set, error=%d\n", error); J> + tuninit(ifp); J> + TUNDEBUG(ifp, "destination address set\n"); J> break; J> case SIOCSIFMTU: J> ifp->if_mtu = ifr->ifr_mtu; J> @@ -857,7 +855,6 @@ tunwrite(struct cdev *dev, struct uio *uio, int fl J> struct tun_softc *tp = dev->si_drv1; J> struct ifnet *ifp = TUN2IFP(tp); J> struct mbuf *m; J> - int error = 0; J> uint32_t family; J> int isr; J> J> @@ -877,7 +874,7 @@ tunwrite(struct cdev *dev, struct uio *uio, int fl J> J> if ((m = m_uiotombuf(uio, M_DONTWAIT, 0, 0, M_PKTHDR)) == NULL) { J> ifp->if_ierrors++; J> - return (error); J> + return (ENOBUFS); J> } J> J> m->m_pkthdr.rcvif = ifp; Yes, that would be correct. Sorry, my bug :( -- Totus tuus, Glebius.