Date: Thu, 14 Oct 2010 12:16:01 +1100 (EST) From: Bruce Evans <brde@optusnet.com.au> To: Jung-uk Kim <jkim@FreeBSD.org> Cc: svn-src-head@FreeBSD.org, Roman Divacky <rdivacky@FreeBSD.org>, src-committers@FreeBSD.org, Rui Paulo <rpaulo@FreeBSD.org>, svn-src-all@FreeBSD.org Subject: Re: svn commit: r213793 - in head/sys/dev: ce cp Message-ID: <20101014113203.N1092@besplex.bde.org> In-Reply-To: <201010131359.44590.jkim@FreeBSD.org> References: <201010131717.o9DHHobD094458@svn.freebsd.org> <20101013172757.GA21579@freebsd.org> <201010131359.44590.jkim@FreeBSD.org>
next in thread | previous in thread | raw e-mail | index | archive | help
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
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20101014113203.N1092>