Date: Tue, 16 Jun 2009 09:27:33 -0400 From: John Baldwin <jhb@freebsd.org> To: Bruce Evans <brde@optusnet.com.au> Cc: svn-src-head@freebsd.org, svn-src-all@freebsd.org, src-committers@freebsd.org Subject: Re: svn commit: r193941 - head/sys/netinet Message-ID: <200906160927.34285.jhb@freebsd.org> In-Reply-To: <20090616091039.Q25629@delplex.bde.org> References: <200906101827.n5AIRFoR022115@svn.freebsd.org> <200906151328.32279.jhb@freebsd.org> <20090616091039.Q25629@delplex.bde.org>
next in thread | previous in thread | raw e-mail | index | archive | help
On Monday 15 June 2009 8:04:42 pm Bruce Evans wrote:
> On Mon, 15 Jun 2009, John Baldwin wrote:
>
> > On Friday 12 June 2009 8:56:56 pm Bruce Evans wrote:
> >> On Fri, 12 Jun 2009, John Baldwin wrote:
> > ...
> >>> FWIW, the variables were signed before they were changed to unsigned and are now
> >>> back as signed again. It just seems really odd to have the types not match
> >>> 'ticks' especially since many of the values are just cached copies of 'ticks'.
> >>
> >> No, they were originally u_long and stayed that way until you changed them
> >> (except possibly for newer variables). The type of `ticks' is just wrong,
> >> and fixing that would take more work, but there is no reason to propagate
> >> this bug to new variables. The original version is:
> >
> > Gah, I had misremembered the diffs, they were indeed unsigned. Here is my
> > attempt at trying to fix things then based on my understanding so far:
>
> This is about what I want.
Ok, I will commit in a bit with some fixes.
> > - I cast 'ticks' to u_int in the code to compute a new isn. I'm not sure if
> > this is needed.
>
> This shouldn't be needed.
Ok, I've removed it.
> > --- //depot/projects/smpng/sys/netinet/tcp_timer.c 2009/04/14 19:06:19
> > +++ //depot/user/jhb/socket/netinet/tcp_timer.c 2009/06/15 17:22:13
> > @@ -418,8 +418,8 @@
> > * backoff that we would use if retransmitting.
> > */
> > if (tp->t_rxtshift == TCP_MAXRXTSHIFT &&
> > - ((ticks - tp->t_rcvtime) >= tcp_maxpersistidle ||
> > - (ticks - tp->t_rcvtime) >= TCP_REXMTVAL(tp) * tcp_totbackoff)) {
> > + (ticks - tp->t_rcvtime >= tcp_maxpersistidle ||
> > + ticks - tp->t_rcvtime >= TCP_REXMTVAL(tp) * tcp_totbackoff)) {
> > TCPSTAT_INC(tcps_persistdrop);
> > tp = tcp_drop(tp, ETIMEDOUT);
> > goto out;
>
> Do we want to keep the non-KNF indentation (1 char extra to line things up)?
It is easier to read with it lined up and in this case I just went with the
existing style rather than reindenting.
> > --- //depot/projects/smpng/sys/netinet/tcp_var.h 2009/06/12 13:45:50
> > +++ //depot/user/jhb/socket/netinet/tcp_var.h 2009/06/15 17:22:13
> > @@ -139,12 +139,12 @@
> >
> > u_int t_maxopd; /* mss plus options */
> >
> > - int t_rcvtime; /* inactivity time */
> > - int t_starttime; /* time connection was established */
> > - int t_rtttime; /* round trip time */
> > + u_int t_rcvtime; /* inactivity time */
> > + u_int t_starttime; /* time connection was established */
> > + u_int t_rtttime; /* round trip time */
>
> I would prefer the delta-times (like t_rtttime) to remain as ints. Having
> everything of the same type is safer, and having everything of signed
> type allows better overflow checking (gcc -ftrapv (*)). Whether ints are
> actually safer for r_tttime depend on how it is used. Oops, I misread
> t_rtttime as being the rtt and a delta-time. It seems to be actually
> an absolute time for the start of rtt measurement. Its comment is thus
> now just wrong.
Ok, I've updated the comment instead and left it as u_int.
--
John Baldwin
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?200906160927.34285.jhb>