From owner-svn-src-all@FreeBSD.ORG Thu Oct 14 01:16:06 2010 Return-Path: Delivered-To: svn-src-all@FreeBSD.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:4f8:fff6::34]) by hub.freebsd.org (Postfix) with ESMTP id 16D2B106566C; Thu, 14 Oct 2010 01:16:06 +0000 (UTC) (envelope-from brde@optusnet.com.au) Received: from mail03.syd.optusnet.com.au (mail03.syd.optusnet.com.au [211.29.132.184]) by mx1.freebsd.org (Postfix) with ESMTP id 9FE148FC12; Thu, 14 Oct 2010 01:16:05 +0000 (UTC) Received: from c122-106-146-165.carlnfd1.nsw.optusnet.com.au (c122-106-146-165.carlnfd1.nsw.optusnet.com.au [122.106.146.165]) by mail03.syd.optusnet.com.au (8.13.1/8.13.1) with ESMTP id o9E1G1TA002070 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=NO); Thu, 14 Oct 2010 12:16:03 +1100 Date: Thu, 14 Oct 2010 12:16:01 +1100 (EST) From: Bruce Evans X-X-Sender: bde@besplex.bde.org To: Jung-uk Kim In-Reply-To: <201010131359.44590.jkim@FreeBSD.org> Message-ID: <20101014113203.N1092@besplex.bde.org> References: <201010131717.o9DHHobD094458@svn.freebsd.org> <20101013172757.GA21579@freebsd.org> <201010131359.44590.jkim@FreeBSD.org> MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII; format=flowed Cc: svn-src-head@FreeBSD.org, Roman Divacky , src-committers@FreeBSD.org, Rui Paulo , svn-src-all@FreeBSD.org Subject: Re: svn commit: r213793 - in head/sys/dev: ce cp X-BeenThere: svn-src-all@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: "SVN commit messages for the entire src tree \(except for " user" and " projects" \)" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 14 Oct 2010 01:16:06 -0000 On Wed, 13 Oct 2010, Jung-uk Kim wrote: > On Wednesday 13 October 2010 01:27 pm, Roman Divacky wrote: >>> >>> Modified: head/sys/dev/ce/if_ce.c >>> ================================================================= >>> ============= --- head/sys/dev/ce/if_ce.c Wed Oct 13 17:16:08 >>> 2010 (r213792) +++ head/sys/dev/ce/if_ce.c Wed Oct 13 17:17:50 >>> 2010 (r213793) @@ -1313,7 +1313,7 @@ static int ce_ioctl (struct >>> cdev *dev, u IFP2SP(d->ifp)->pp_flags &= ~(PP_FR); >>> IFP2SP(d->ifp)->pp_flags |= PP_KEEPALIVE; >>> d->ifp->if_flags |= PP_CISCO; >>> - } else if (! strcmp ("fr", (char*)data) && PP_FR) { >>> + } else if (! strcmp ("fr", (char*)data)) { >> >> this is wrong I think... the PP_FR was used for compiling in/out >> support for something.. see the comment: >> >> /* If we don't have Cronyx's sppp version, we don't have fr support >> via sppp */ #ifndef PP_FR >> #define PP_FR 0 >> #endif >> >> note that PP_FR is used in some other places as a flag. I guess >> that by compiling with something like make -DPP_FR=42 some magic >> would happen. >> >> anyway - this does not look like a bug but like an intent, please >> revert. > > I think the attached patch should do. % Index: sys/dev/ce/if_ce.c % =================================================================== % --- sys/dev/ce/if_ce.c (revision 213782) % +++ sys/dev/ce/if_ce.c (working copy) % @@ -1313,9 +1313,11 @@ static int ce_ioctl (struct cdev *dev, u_long cmd, % IFP2SP(d->ifp)->pp_flags &= ~(PP_FR); % IFP2SP(d->ifp)->pp_flags |= PP_KEEPALIVE; % d->ifp->if_flags |= PP_CISCO; % - } else if (! strcmp ("fr", (char*)data) && PP_FR) { % +#if PP_FR > 0 % + } else if (! strcmp ("fr", (char*)data)) { % d->ifp->if_flags &= ~(PP_CISCO); % IFP2SP(d->ifp)->pp_flags |= PP_FR | PP_KEEPALIVE; % +#endif % } else if (! strcmp ("ppp", (char*)data)) { % IFP2SP(d->ifp)->pp_flags &= ~PP_FR; % IFP2SP(d->ifp)->pp_flags &= ~PP_KEEPALIVE; % ... This gives different behaviour if PP_FR is even or negative. Even values used to fail the match, but now the match succeeds for even values > 0 and then the bits of PP_FR are put in pp_flags. Negative values used to pass the match if they were odd, but now the match is not attempted for any negative value. It just might be useful for PP_FR to have multiple bits set, with the 1 bit used for enabling this and the other bits used for setting pp_flags. If not, then only values of 0 and 1 for PP_FR make sense, and the ifdef should be "#if PP_FR == 1". One of the 3 style bugs of "! strcmp (...)" is larger now. The normal style of "strcmp(...) == 0" would have inhibited this bug, but perhaps "!" was used to emphasize that the result is boolean. The 3 strcmp's visible in the above patch are the only ones in the file with the "! " style bugs. The normal style of explicitly comparing the result of strcmp with 0 is used 2 times, with consistency only for using a gnu-style space before the left parentheses. I don't see why clang complained about this. "! strcmp()" has a boolean result with value of 1 for "true". It is perfectly valid to AND this result with a boolean flag that also has value 1 for "true", or with an integral bitmap that uses bit 0 (value 1) for this test. Bruce