httpd-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Roy T. Fielding" <field...@kiwi.ics.uci.edu>
Subject Re: [STATUS] Apache 1.2.2 (Sun 3-Aug-1997 12:22 MET DST)
Date Mon, 04 Aug 1997 07:28:24 GMT
>    o [patch.chunkerr]
>      Dean's "[PATCH] 1.2.2: obscure chunking error":
>      <Pine.LNX.3.95dg3.970729000353.17653A-100000@benchlark.arctic.org>
>      While chunking, it is possible to exit bwrite() or bflush() without
>      having started the next chunk.  It should only happen when there's a
>      write error ... but in some cases it would return without setting the
>      error flags.
>      Status: Dean +1, RSE +0 
>      => ** STILL NEEDS TESTING OR WE IGNORE IT FOR 1.2.2! **

I've looked at this closely and I am going to veto it, at least until
I can be persuaded that it does something good.  This is going to be hard
to explain, but here goes....

>Index: buff.c
>===================================================================
>RCS file: /export/home/cvs/apache/src/buff.c,v
>retrieving revision 1.26
>diff -u -r1.26 buff.c
>--- buff.c	1997/05/29 05:21:15	1.26
>+++ buff.c	1997/07/29 07:02:58
>@@ -750,6 +750,9 @@
> 	    return i;
>     }
> 
>+    /* in case a chunk hasn't been started yet */
>+    if( fb->flags & B_CHUNK ) start_chunk( fb );
>+

There is no need to start a chunk until there is data to be written,
which means either that there is already data in the buffer (in which
case start_chunk would have already been called) or the buffer is empty
and the new data is larger than the buffer size (in which case we would
be calling bcwrite and would not want to call start_chunk first).  Note
that this change will cause two extra memcpy calls if the buffer
is empty and (nbyte >= fb->bufsiz).

> /*
>  * Whilst there is data in the buffer, keep on adding to it and writing it
>  * out
>@@ -776,13 +779,17 @@
> 	    /* it is just too painful to try to re-cram the buffer while
> 	     * chunking
> 	     */
>-	    i = (write_it_all(fb, fb->outbase, fb->outcnt) == -1) ?
>-	            -1 : fb->outcnt;
>-	}
>-	else {
>-	    do i = write(fb->fd, fb->outbase, fb->outcnt);
>-	    while (i == -1 && errno == EINTR && !(fb->flags & B_EOUT));
>+	    if (write_it_all(fb, fb->outbase, fb->outcnt) == -1) {
>+		/* we cannot continue after a chunked error */
>+		doerror (fb, B_WR);
>+		return -1;
>+	    }
>+	    fb->outcnt = 0;
>+	    break;
> 	}
>+	do {
>+	    i = write(fb->fd, fb->outbase, fb->outcnt);
>+	} while (i == -1 && errno == EINTR && !(fb->flags & B_EOUT));
> 	if (i <= 0) {
> 	    if (i == 0) /* return of 0 means non-blocking */
> 	        errno = EAGAIN;

This is one example where a context diff is *much* easier to read.
The only good thing this change does is force doerror, which should really
be done inside write_it_all().  The change also fails to update
fb->bytes_sent on a successful write, which makes it severely broken.

While I understand the bug that this is trying to fix, the fix needs to
be done inside write_it_all() and inside bcwrite(), which would also
simplify the handling of EINTR and EAGAIN in this routine.

....Roy

Mime
View raw message