You are viewing a plain text version of this content. The canonical link for it is here.
Posted to cvs@httpd.apache.org by jo...@apache.org on 2005/07/18 16:39:17 UTC
svn commit: r219501 - /httpd/httpd/branches/2.0.x/STATUS
Author: jorton
Date: Mon Jul 18 07:39:01 2005
New Revision: 219501
URL: http://svn.apache.org/viewcvs?rev=219501&view=rev
Log:
Attempt to review the proxy patch...
Modified:
httpd/httpd/branches/2.0.x/STATUS
Modified: httpd/httpd/branches/2.0.x/STATUS
URL: http://svn.apache.org/viewcvs/httpd/httpd/branches/2.0.x/STATUS?rev=219501&r1=219500&r2=219501&view=diff
==============================================================================
--- httpd/httpd/branches/2.0.x/STATUS (original)
+++ httpd/httpd/branches/2.0.x/STATUS Mon Jul 18 07:39:01 2005
@@ -115,7 +115,17 @@
Revert r219061 to properly test this patch, as r219061 masks the
underlying bug (although it is a -good- patch in and of itself).
- +1 wrowe, jim
+ +1: wrowe, jim
+ -1: jorton: this is a massive patch and extremely hard to review
+ for actual interesting content; it is mixed in with all sorts
+ of unrelated stuff. It needs to at least be split up or
+ the unrelated stuff removed.
+
+ unrelated change: s/apr_strnatcasecmp/strcasecmp/
+ unrelated change: s/b/bb/ on variable+parameter names a few times
+ unrelated change: whitespaces changes all over the shop
+ spurious change:? send_request_body() appears to have been inlined
+ unrelated change: Via header handling
trawick noted on list: we elected C-L not for efficiency, but because
it's the most widely supported [paraphrasing]
Re: svn commit: r219501 - /httpd/httpd/branches/2.0.x/STATUS
Posted by "William A. Rowe, Jr." <wr...@rowe-clan.net>.
At 09:39 AM 7/18/2005, jorton@apache.org wrote:
>Author: jorton
>Date: Mon Jul 18 07:39:01 2005
>New Revision: 219501
>
>Attempt to review the proxy patch...
Joe, one reason it's -very- hard to follow this patch is because
of r171205, never released, which didn't follow the RFC in attempting
to close the flaws (and in fact was very buggy in my edge-case tests,
although I'd rather fix than veto and revert.)
Review the delta to 2.0.54, which makes things a little clearer;
http://people.apache.org/~wrowe/httpd-2.0.54-proxy-request.patch
this will shine allot more light on where proxy has moved since
the last release.
Bill
Re: svn commit: r219501 - /httpd/httpd/branches/2.0.x/STATUS
Posted by "William A. Rowe, Jr." <wr...@rowe-clan.net>.
At 09:39 AM 7/18/2005, jorton@apache.org wrote:
>+ -1: jorton: this is a massive patch and extremely hard to review
>+ for actual interesting content; it is mixed in with all sorts
>+ of unrelated stuff. It needs to at least be split up or
>+ the unrelated stuff removed.
>+
>+ unrelated change: s/apr_strnatcasecmp/strcasecmp/
apr_strnatcasecmp means something different than strcasecmp,
I'm guessing the author was cutting and pasting, and never noticed
the distinction. (Was probably looking for portable strcasecmp,
which we already ensure will be portable.)
>+ unrelated change: s/b/bb/ on variable+parameter names a few times
axed, good point.
>+ unrelated change: whitespaces changes all over the shop
minimized if possible.
The point is that the new code can actually be compared to httpd-2.1
- it makes sense to keep identical code in sync.
>+ spurious change:? send_request_body() appears to have been inlined
Not spurious. We were wasting alot of cycles to make send_request_body
disjoint from ap_proxy_http_request. It isn't; the actions performed
in ap_proxy_http_request already do all the work for us, we lost that
memory and repeated some of the process in the name of separate fn()'s.
By splitting this code across two functions it was very hard to review
the code. I agree simple patch review is good, but the fact is that
the code itself was too hard for anyone to review, thus these bugs that
had crept into the refactoring a few years back.
The attached patch (available unmauled from
http://people.apache.org/~wrowe/httpd-2.0-proxy-request-4.patch)
should address all of your valid concerns.
I consider your other concerns invalid because the original code
was otherwise illegible, duplicitous (which introduced even MORE
bugs), and never followed style conventions. 40% of the time I
am trying to review code in an 80 column vim window in wrap mode,
and some of the proxy code looks like no other code in httpd.
I've tried to make the new patch as simple as possible, without
being so simple as to make review of proxy_http.c impossible.
Since only five people in this world can apparently read the
-old- code, I consider making the -code- more legible than the
actual -patch- a valid reason for a somewhat more complex patch.
Bill
Re: svn commit: r219501 - /httpd/httpd/branches/2.0.x/STATUS
Posted by "William A. Rowe, Jr." <wr...@rowe-clan.net>.
At 09:39 AM 7/18/2005, jorton@apache.org wrote:
>Author: jorton
>Date: Mon Jul 18 07:39:01 2005
>New Revision: 219501
>
>Attempt to review the proxy patch...
Joe, one reason it's -very- hard to follow this patch is because
of r171205, never released, which didn't follow the RFC in attempting
to close the flaws (and in fact was very buggy in my edge-case tests,
although I'd rather fix than veto and revert.)
Review the delta to 2.0.54, which makes things a little clearer;
http://people.apache.org/~wrowe/httpd-2.0.54-proxy-request.patch
this will shine allot more light on where proxy has moved since
the last release.
Bill
Re: svn commit: r219501 - /httpd/httpd/branches/2.0.x/STATUS
Posted by "William A. Rowe, Jr." <wr...@rowe-clan.net>.
At 09:39 AM 7/18/2005, jorton@apache.org wrote:
>+ -1: jorton: this is a massive patch and extremely hard to review
>+ for actual interesting content; it is mixed in with all sorts
>+ of unrelated stuff. It needs to at least be split up or
>+ the unrelated stuff removed.
>+
>+ unrelated change: s/apr_strnatcasecmp/strcasecmp/
apr_strnatcasecmp means something different than strcasecmp,
I'm guessing the author was cutting and pasting, and never noticed
the distinction. (Was probably looking for portable strcasecmp,
which we already ensure will be portable.)
>+ unrelated change: s/b/bb/ on variable+parameter names a few times
axed, good point.
>+ unrelated change: whitespaces changes all over the shop
minimized if possible.
The point is that the new code can actually be compared to httpd-2.1
- it makes sense to keep identical code in sync.
>+ spurious change:? send_request_body() appears to have been inlined
Not spurious. We were wasting alot of cycles to make send_request_body
disjoint from ap_proxy_http_request. It isn't; the actions performed
in ap_proxy_http_request already do all the work for us, we lost that
memory and repeated some of the process in the name of separate fn()'s.
By splitting this code across two functions it was very hard to review
the code. I agree simple patch review is good, but the fact is that
the code itself was too hard for anyone to review, thus these bugs that
had crept into the refactoring a few years back.
The attached patch (available unmauled from
http://people.apache.org/~wrowe/httpd-2.0-proxy-request-4.patch)
should address all of your valid concerns.
I consider your other concerns invalid because the original code
was otherwise illegible, duplicitous (which introduced even MORE
bugs), and never followed style conventions. 40% of the time I
am trying to review code in an 80 column vim window in wrap mode,
and some of the proxy code looks like no other code in httpd.
I've tried to make the new patch as simple as possible, without
being so simple as to make review of proxy_http.c impossible.
Since only five people in this world can apparently read the
-old- code, I consider making the -code- more legible than the
actual -patch- a valid reason for a somewhat more complex patch.
Bill