Date: Sun, 17 Nov 2002 13:15:44 -0500 (EST) From: Robert Watson <rwatson@FreeBSD.org> To: Luigi Rizzo <luigi@FreeBSD.org> Cc: cvs-committers@FreeBSD.org, cvs-all@FreeBSD.org Subject: Re: cvs commit: src/sys/netinet ip_output.c ip_var.h Message-ID: <Pine.NEB.3.96L.1021117130133.93303D-100000@fledge.watson.org> In-Reply-To: <200211171630.gAHGUjtH096646@repoman.freebsd.org>
next in thread | previous in thread | raw e-mail | index | archive | help
I have to say that, on face value, I'm very concerned by this recent
commit spree. The diffs are almost impossible to read since they include
code relocation, style changes, and "non-functional changes" that
substantially change the code flow. I'm still trying to get a grasp of
exactly what has changed, but my initial response is "This doesn't look
good". Please do not *ever* combine code relocation with other classes of
changes such as style changes and changes like the m_copy->m_copypacket
changes. It makes it impossible to read and be sure things are right with
a relatively quick glance. For example, the following changes are very
difficult to read:
...
- for (; off < (u_short)ip->ip_len; off += len) {
+ for (nfrags=1; off < ip->ip_len; off += len, nfrags++) {
XXX removal of a variable, removal of a cast, etc -- combining these
changes makes the diff hard to read.
@@ -1140,76 +1218,42 @@
mhip->ip_sum = in_cksum(m, mhlen);
*mnext = m;
mnext = &m->m_nextpkt;
- nfrags++;
XXX This appears to have moved.. somewhere..
}
ipstat.ips_ofragments += nfrags;
- /* set first/last markers for fragment chain */
- m->m_flags |= M_LASTFRAG;
+ /* set first markers for fragment chain */
m0->m_flags |= M_FIRSTFRAG | M_FRAG;
m0->m_pkthdr.csum_data = nfrags;
XXX Functional change here, apparently
/*
- * Update first fragment by trimming what's been copied out
- * and updating header, then send each fragment (in order).
+ * Update first fragment by trimming what has been copied out
+ * and updating header.
XXX Gratuitous style change mixed up in functional changes.
*/
- m = m0;
- m_adj(m, hlen + firstlen - (u_short)ip->ip_len);
- m->m_pkthdr.len = hlen + firstlen;
- ip->ip_len = htons((u_short)m->m_pkthdr.len);
+ m_adj(m0, hlen + firstlen - ip->ip_len);
+ m0->m_pkthdr.len = hlen + firstlen;
+ ip->ip_len = htons((u_short)m0->m_pkthdr.len);
XXX Variable renaming thrown into the mix.
ip->ip_off |= IP_MF;
In some places you've changed comments, removed casting, and apparently
changed functional behavior. Doing this within a day or two of the code
freeze is unwise, because subtle bugs could have slipped in here that will
be almost impossible to track down on the basis that so much moved around.
I'm concerned also that you didn't provide advance notice to the release
engineering team that you had decided to reorganize substantial chunks of
the IP output processing--while Sam has also been doing some IP stack
re-factoring late in the game, he's run the diffs by re@ and gotten
reviews out of a number of people individuall. The diffs resulting from
these changes are essentially garbage due to the syntactic/semantic mess.
I'd actually like to see this backed out -- not because the changes are
necessarily wrong, but because it's difficult to see if they are right.
I'd then like to see them re-introduced carefully in the following
sequence:
(1) Style cleanup first, including function header changes, line wrapping
changes, register removal, etc.
(2) Separation of IP fragment handling code avoiding funtional and
syntactic changes as much as possible.
(3) Any structural cleanups that can now be performed based on the
factoring out of fragment code.
(4) Variable renaming: earlier, preserve the 'm0' as opposed to 'm' usage
for the fragment code so it's possible to see that nothing changes.
Otherwise it's all one big diff with every line changes.
(5) m_copypacket changes.
That way if any of these steps introduces bugs, we can easily grab the
source from in between the steps. It will also be easier to understand
the sequence of events. Also, btw, combining all these changes breaks all
outstanding diffs against the code -- if this is done in steps it should
be possible to integrate the changes revision by revision and avoid
serious conflicts.
I'd like to see this happen for the other changes you've dropped in in the
last few days also: really separate functional changes from style changes,
etc. All seemed good in principle, but almost impossible to read, and
hard to debug if there are any problems.
Robert N M Watson FreeBSD Core Team, TrustedBSD Projects
robert@fledge.watson.org Network Associates Laboratories
On Sun, 17 Nov 2002, Luigi Rizzo wrote:
> luigi 2002/11/17 08:30:44 PST
>
> Modified files:
> sys/netinet ip_output.c ip_var.h
> Log:
> Move the ip_fragment code from ip_output() to a separate function,
> so that it can be reused elsewhere (there is a number of places
> where it can be useful). This also trims some 200 lines from
> the body of ip_output(), which helps readability a bit.
>
> (This change was discussed a few weeks ago on the mailing lists,
> Julian agreed, silence from others. It is not a functional change,
> so i expect it to be ok to commit it now but i am happy to back it
> out if there are objections).
>
> While at it, fix some function headers and replace m_copy() with
> m_copypacket() where applicable.
>
> MFC after: 1 week
>
> Revision Changes Path
> 1.171 +168 -143 src/sys/netinet/ip_output.c
> 1.69 +2 -0 src/sys/netinet/ip_var.h
>
To Unsubscribe: send mail to majordomo@FreeBSD.org
with "unsubscribe cvs-all" in the body of the message
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?Pine.NEB.3.96L.1021117130133.93303D-100000>