svn commit: r213793 - in head/sys/dev: ce cp

[Jung-uk Kim]

On Wednesday 13 October 2010 09:16 pm, Bruce Evans wrote:
> 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".

I don't understand your remarks about PP_FR being even/odd.  Maybe you 
are confused '&&' with '&'? ;-)

Is the attached patch okay for you, then?

Jung-uk Kim
-------------- next part --------------
Index: sys/dev/ce/if_ce.c
===================================================================
--- sys/dev/ce/if_ce.c	(revision 213846)
+++ 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;
Index: sys/dev/cp/if_cp.c
===================================================================
--- sys/dev/cp/if_cp.c	(revision 213846)
+++ sys/dev/cp/if_cp.c	(working copy)
@@ -1052,9 +1052,11 @@ static int cp_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;