You are viewing a plain text version of this content. The canonical link for it is here.
Posted to cvs@httpd.apache.org by ni...@apache.org on 2007/12/08 16:44:55 UTC
svn commit: r602485 - /httpd/httpd/branches/2.2.x/STATUS
Author: niq
Date: Sat Dec 8 07:44:54 2007
New Revision: 602485
URL: http://svn.apache.org/viewvc?rev=602485&view=rev
Log:
Vote, promote, comment.
Modified:
httpd/httpd/branches/2.2.x/STATUS
Modified: httpd/httpd/branches/2.2.x/STATUS
URL: http://svn.apache.org/viewvc/httpd/httpd/branches/2.2.x/STATUS?rev=602485&r1=602484&r2=602485&view=diff
==============================================================================
--- httpd/httpd/branches/2.2.x/STATUS (original)
+++ httpd/httpd/branches/2.2.x/STATUS Sat Dec 8 07:44:54 2007
@@ -79,6 +79,25 @@
PATCHES ACCEPTED TO BACKPORT FROM TRUNK:
[ start all new proposals below, under PATCHES PROPOSED. ]
+ * mod_status: For long request lines we only see the front part of
+ the requests in mod_status, which is useless if they only vary
+ in their ending bits. Allow admin to specify whether they want
+ to see the 1st 63 chars of the request, or the last.
+ trunk:
+ http://svn.apache.org/viewvc?view=rev&revision=590641
+ 2.2.x:
+ http://people.apache.org/~jim/patches/reqtail-patch.txt
+ +1: jim, rpluem, niq
+
+ * core: Change etag generation to produce identical results on
+ 32-bit and 64-bit platforms.
+ PR 40064.
+ Trunk version of patch:
+ http://svn.apache.org/viewvc?view=rev&rev=517238
+ http://svn.apache.org/viewvc?view=rev&rev=517654
+ Backport version for 2.2.x of patch:
+ Trunk version of patch works
+ +1: rpluem, jim, niq
PATCHES PROPOSED TO BACKPORT FROM TRUNK:
[ New proposals should be added at the end of the list ]
@@ -117,7 +136,7 @@
Trunk version of patch works
+1: jim, rpluem
niq: Doesn't allocating 2^n+1 bytes risk being horribly inefficient?
- Would it make sense to reduce AP_IOBUFSIZE to 8091 so buf doesn't
+ Would it make sense to reduce AP_IOBUFSIZE to 8191 so buf doesn't
go one over a big boundary?
jim: The issue is that we allocate an array of AP_IOBUFSIZE
but go beyond that (note we send the end to curpos+AP_IOBUFSIZE)
@@ -130,16 +149,14 @@
code do this by setting the endpos to one less to "save"
space for the NUL, but they don't have related sections
which assume a set size; this does (AP_IOBUFSIZE).
-
- * mod_status: For long request lines we only see the front part of
- the requests in mod_status, which is useless if they only vary
- in their ending bits. Allow admin to specify whether they want
- to see the 1st 63 chars of the request, or the last.
- trunk:
- http://svn.apache.org/viewvc?view=rev&revision=590641
- 2.2.x:
- http://people.apache.org/~jim/patches/reqtail-patch.txt
- +1: jim, rpluem
+ niq: You're missing my point. That this patch fixes a bug is
+ perfectly clear. But it does so by allocating (AP_IOBUFSIZE+1)
+ bytes. My point: isn't that likely to be horribly inefficient
+ if AP_IOBUFSIZE == 2^n+1? THEREFORE this fix should be
+ accompanied by decrementing AP_IOBUFSIZE, so that the
+ allocation of AP_IOBUFSIZE+1 bytes becomes 2^n.
+ Correct me if I'm wrong about that mattering ever, on
+ any platform and architecture.
* mod_proxy: Support variable interpolation in reverse proxy configuration
(revised proposal dealing with wrowe's concerns).
@@ -171,16 +188,6 @@
make it harder to recognize regressions.
Trunk version of patch:
http://svn.apache.org/viewcvs.cgi?rev=599589&view=rev
- Backport version for 2.2.x of patch:
- Trunk version of patch works
- +1: rpluem, jim
-
- * core: Change etag generation to produce identical results on
- 32-bit and 64-bit platforms.
- PR 40064.
- Trunk version of patch:
- http://svn.apache.org/viewvc?view=rev&rev=517238
- http://svn.apache.org/viewvc?view=rev&rev=517654
Backport version for 2.2.x of patch:
Trunk version of patch works
+1: rpluem, jim
Re: svn commit: r602485 - /httpd/httpd/branches/2.2.x/STATUS
Posted by Jim Jagielski <ji...@jaguNET.com>.
On Dec 8, 2007, at 10:44 AM, niq@apache.org wrote:
> + niq: You're missing my point. That this patch fixes a bug is
> + perfectly clear. But it does so by allocating
> (AP_IOBUFSIZE+1)
> + bytes. My point: isn't that likely to be horribly
> inefficient
> + if AP_IOBUFSIZE == 2^n+1? THEREFORE this fix should be
> + accompanied by decrementing AP_IOBUFSIZE, so that the
> + allocation of AP_IOBUFSIZE+1 bytes becomes 2^n.
> + Correct me if I'm wrong about that mattering ever, on
> + any platform and architecture.
>
Nick, as I understand it, your issue is whether it is
functionally inefficient to allocate space that is not
a multiple of 2, is that right? That is a buffer of 2^n
is "better" than one which is not? Since this is a local
allocation and we're always just using AP_IOBUFSIZE
worth of the data in it, it really doesn't matter. In
fact, decreasing AP_IOBUFSIZE would be very bad because
that is a value which is used for all sorts of network
and buffering codepaths, in which case having a 2^n size
*is* crucial.
As far as the patch is concerned, the issue is that
there are conditions where vd.vbuff.curpos is actually
pointing to vrprintf_buf + AP_IOBUFSIZE, which is
1 outside of an allocation of vrprintf_buf[AP_IOBUFSIZE].
So when the
*(vd.vbuff.curpos) = '\0';
is done, we're outside the bounds. There are 2 ways to
fix this. They current way which is simply to ensure that
vrprintf_buf + AP_IOBUFSIZE is inside the allocation OR
we can simply drop the "terminate string" line. Both address
the issue.
The reason I did the 1st is that it fixed the issue and
that I couldn't see the reason for the string-terminate line
but figured it was there for a reason anyway... I've been
able to profile all this and see that the line is really
not needed at all. So in r602491 I've changed to the
2nd fix, even though the 1st is fine.