You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@httpd.apache.org by Yann Ylavic <yl...@gmail.com> on 2016/08/05 15:43:27 UTC
Re: svn commit: r1755124 - /httpd/httpd/trunk/server/protocol.c
On Thu, Aug 4, 2016 at 12:58 AM, <wr...@apache.org> wrote:
> Author: wrowe
> Date: Wed Aug 3 22:58:10 2016
> New Revision: 1755124
>
> URL: http://svn.apache.org/viewvc?rev=1755124&view=rev
> Log:
> Reformat for indentation following r1755123, Whitespace Only
>
> Modified:
> httpd/httpd/trunk/server/protocol.c
We could save yet another level of indentation with:
Index: server/protocol.c
===================================================================
--- server/protocol.c (revision 1755347)
+++ server/protocol.c (working copy)
@@ -788,7 +788,7 @@ AP_DECLARE(void) ap_get_mime_headers_core(request_
* Read header lines until we get the empty separator line, a read error,
* the connection closes (EOF), reach the server limit, or we timeout.
*/
- while(1) {
+ do {
apr_status_t rv;
field = NULL;
@@ -903,8 +903,16 @@ AP_DECLARE(void) ap_get_mime_headers_core(request_
*/
continue;
}
- else if (last_field != NULL) {
+ if (last_field == NULL) {
+ /* Keep track of this first header line so that we can extend it
+ * across any obs-fold or parse it on the next loop iteration.
+ */
+ last_field = field;
+ last_len = len;
+ continue;
+ }
+
/* Process the previous last_field header line with all obs-folded
* segments already concatinated (this is not operating on the
* most recently read input line).
@@ -986,25 +994,14 @@ AP_DECLARE(void) ap_get_mime_headers_core(request_
apr_table_addn(r->headers_in, last_field, value);
- /* This last_field header is now stored in headers_in,
- * resume processing of the current input line.
- */
- }
-
- /* Found the terminating empty end-of-headers line, stop. */
- if (len == 0) {
- break;
- }
-
- /* Keep track of this new header line so that we can extend it across
- * any obs-fold or parse it on the next loop iteration. We referenced
- * our previously allocated buffer in r->headers_in,
- * so allocate a fresh buffer if required.
+ /* This last_field header is now stored in headers_in,
+ * resume processing of the current input line, so
+ * allocate a fresh buffer if required.
*/
alloc_len = 0;
last_field = field;
last_len = len;
- }
+ } while (last_len);
/* Combine multiple message-header fields with the same
* field-name, following RFC 2616, 4.2.
_
and then reindent the block left shifted above (in a second whitespace
only commit).
Not sure this old shared/participatory code deserves so much mangling,
that's no functional change, just readability improvement (maybe)...
WDYT?
Re: svn commit: r1755124 - /httpd/httpd/trunk/server/protocol.c
Posted by William A Rowe Jr <wr...@rowe-clan.net>.
On Fri, Aug 5, 2016 at 12:50 PM, Yann Ylavic <yl...@gmail.com> wrote:
> > while adding
> > the unnecessary verification of last_len != 0 from line 904, so I'd say
> it's
> > a
> > net loss of legibility in spite of gaining us 4 characters. Just my 2c.
>
> The 'len' verification is necessary because it can be zero when
> last_field is NULL (legitimately, no headers at all), and we must
> leave still.
>
I thought that was a bug, but you caught it in the no-lines case by hitting
the trailing while() test. What I referred to was the continue from line
904
where we had merged an obs-fold, where can't have an empty last_len,
so the test from that continue is a wasted cycle.
Re: svn commit: r1755124 - /httpd/httpd/trunk/server/protocol.c
Posted by Yann Ylavic <yl...@gmail.com>.
On Fri, Aug 5, 2016 at 7:33 PM, William A Rowe Jr <wr...@rowe-clan.net> wrote:
> On Fri, Aug 5, 2016 at 10:43 AM, Yann Ylavic <yl...@gmail.com> wrote:
>>
>> @@ -903,8 +903,16 @@ AP_DECLARE(void) ap_get_mime_headers_core(request_
>> */
>> continue;
>> }
>> - else if (last_field != NULL) {
>>
>> + if (last_field == NULL) {
>> + /* Keep track of this first header line so that we can extend
>> it
>> + * across any obs-fold or parse it on the next loop
>> iteration.
>> + */
>> + last_field = field;
>> + last_len = len;
>> + continue;
>> + }
>> +
>> /* Process the previous last_field header line with all
>> obs-folded
>> * segments already concatinated (this is not operating on
>> the
>> * most recently read input line).
>
>
> This patch makes it less clear that the continue; case above also avoided
> the empty-line case (that was clearer in the main loop imo),
Personnaly an if-continued followed by an else make me think that
there is (at least) one top if-not-continued in the series (often hard
to follow construction), but this isn't the case here...
Anyway, I don't like too much my patch finally because it duplicates
logic code (few but still).
> while adding
> the unnecessary verification of last_len != 0 from line 904, so I'd say it's
> a
> net loss of legibility in spite of gaining us 4 characters. Just my 2c.
The 'len' verification is necessary because it can be zero when
last_field is NULL (legitimately, no headers at all), and we must
leave still.
Re: svn commit: r1755124 - /httpd/httpd/trunk/server/protocol.c
Posted by William A Rowe Jr <wr...@rowe-clan.net>.
On Fri, Aug 5, 2016 at 10:43 AM, Yann Ylavic <yl...@gmail.com> wrote:
> @@ -903,8 +903,16 @@ AP_DECLARE(void) ap_get_mime_headers_core(request_
> */
> continue;
> }
> - else if (last_field != NULL) {
>
> + if (last_field == NULL) {
> + /* Keep track of this first header line so that we can extend
> it
> + * across any obs-fold or parse it on the next loop iteration.
> + */
> + last_field = field;
> + last_len = len;
> + continue;
> + }
> +
> /* Process the previous last_field header line with all
> obs-folded
> * segments already concatinated (this is not operating on the
> * most recently read input line).
>
This patch makes it less clear that the continue; case above also avoided
the empty-line case (that was clearer in the main loop imo), while adding
the unnecessary verification of last_len != 0 from line 904, so I'd say
it's a
net loss of legibility in spite of gaining us 4 characters. Just my 2c.