Em Seg, 2015-08-03 às 12:35 +0300, Maxim Dounin escreveu:
> Hello!

Hi! Thanks for the review.

> On Wed, Jul 29, 2015 at 05:15:24PM -0300, Jonh Wendell wrote:
>
> > src/http/ngx_http_header_filter_module.c | 5 +
> > src/http/ngx_http_request.h | 2 +
> > src/http/ngx_http_variables.c | 92
> > ++++++++++++++++++++++++++++++++
> > 3 files changed, 99 insertions(+), 0 deletions(-)
> >
> >
> > # HG changeset patch
> > # User Jonh Wendell <jonh.wendell@gmail.com>
> > # Date 1438199955 10800
> > # Wed Jul 29 16:59:15 2015 -0300
> > # Node ID 2412f965360cdbf9d15280e8ee9fa1a28a3c86ca
> > # Parent 341e4303d25be159d4773b819d0ec055ba711afb
> > Add log variables $http_all and $sent_http_all
>
> These variables names are not compatible with $http_<name> and
> $sent_http_<name> variables as used by nginx.

Indeed. This was lack of creativity from myself. This is easy to
change. Suggestions? :)

> >
> > These are meant to log the entire request and response
> > headers, respectively.
> >
> > There are cases when we want to log the whole request (or response)
> > headers, for example, when we don't know in advance the
> > field the client sends. Currently we must know exactly the
> > header fields we want to log.
> >
> > This patch adds these two variables that contains
> >
> > $http_all: all request headers as received from client
> > $sent_http_all: all response headers as sent to the client
> >
> > Closes #426.
> >
> > diff -r 341e4303d25b -r 2412f965360c
> > src/http/ngx_http_header_filter_module.c
> > --- a/src/http/ngx_http_header_filter_module.c Thu Jul 16
> > 14:20:48 2015 +0300
> > +++ b/src/http/ngx_http_header_filter_module.c Wed Jul 29
> > 16:59:15 2015 -0300
> > @@ -608,6 +608,11 @@
> > ngx_log_debug2(NGX_LOG_DEBUG_HTTP, c->log, 0,
> > "%*s", (size_t) (b->last - b->pos), b->pos);
> >
> > + /* prepare the complete headers for eventual logging - strip
> > out the last "\r\n" */
> > + r->headers_out.final_response.len = b->last - b->pos -
> > (sizeof(CRLF) - 1);
> > + r->headers_out.final_response.data = ngx_palloc(r->pool, r
> > ->headers_out.final_response.len);
> > + ngx_memcpy(r->headers_out.final_response.data, b->pos, r
> > ->headers_out.final_response.len);
> > +
>
> This doesn't looks like an acceptable solution.
> (Not even talking about coding style issues.)

You didn't like the creation of an additional struct field? The "waste"
of memory allocated by this field if it's not used for logging?

What could be an acceptable solution here?

> [...]
>
> > + buf = ngx_sprintf(buf, "%V: %V\r\n", &header[i].key,
> > &header[i].value);
>
> I can't say I ike the idea of reconstructing headers.

Do we have these headers available somewhere without the need of
reconstructing them? If not, would you prefer having them so that other
parts of code could benefit from it?

> [...]
>

Again, thanks for reviewing it. That's my first patch/contact with
nginx, I'd love to get this feature in :)

_______________________________________________
nginx-devel mailing list
nginx-devel@nginx.org
http://mailman.nginx.org/mailman/listinfo/nginx-devel