Hello! On Fri, Jun 02, 2017 at 08:33:45PM -0700, Piotr Sikora via nginx-devel wrote:
> # HG changeset patch > # User Piotr Sikora <[email protected]> > # Date 1490351854 25200 > # Fri Mar 24 03:37:34 2017 -0700 > # Node ID 41c09a2fd90410e25ad8515793bd48028001c954 > # Parent 716852cce9136d977b81a2d1b8b6f9fbca0dce49 > Added support for trailers in HTTP responses. > > Example: > > ngx_table_elt_t *h; > > h = ngx_list_push(&r->headers_out.trailers); > if (h == NULL) { > return NGX_ERROR; > } > > ngx_str_set(&h->key, "Fun"); > ngx_str_set(&h->value, "with trailers"); > h->hash = ngx_hash_key_lc(h->key.data, h->key.len); > > The code above adds "Fun: with trailers" trailer to the response. > > Modules that want to emit trailers must set r->expect_trailers = 1 > in header filter, otherwise they might not be emitted for HTTP/1.1 > responses that aren't already chunked. > > This change also adds $sent_trailer_* variables. > > Signed-off-by: Piotr Sikora <[email protected]> Overall looks good, see some additional comments below. > > diff -r 716852cce913 -r 41c09a2fd904 > src/http/modules/ngx_http_chunked_filter_module.c > --- a/src/http/modules/ngx_http_chunked_filter_module.c > +++ b/src/http/modules/ngx_http_chunked_filter_module.c > @@ -17,6 +17,7 @@ typedef struct { > > > static ngx_int_t ngx_http_chunked_filter_init(ngx_conf_t *cf); > +static ngx_chain_t *ngx_http_chunked_create_trailers(ngx_http_request_t *r); > > > static ngx_http_module_t ngx_http_chunked_filter_module_ctx = { > @@ -69,27 +70,28 @@ ngx_http_chunked_header_filter(ngx_http_ > return ngx_http_next_header_filter(r); > } > > - if (r->headers_out.content_length_n == -1) { > - if (r->http_version < NGX_HTTP_VERSION_11) { > + if (r->headers_out.content_length_n == -1 || r->expect_trailers) { > + I actually think that using two lines as initially suggested is more readable and more in line with current style. YMMV. - if (r->headers_out.content_length_n == -1 || r->expect_trailers) { - + if (r->headers_out.content_length_n == -1 + || r->expect_trailers) + { [...] > @@ -230,6 +223,105 @@ ngx_http_chunked_body_filter(ngx_http_re > } > > > +static ngx_chain_t * > +ngx_http_chunked_create_trailers(ngx_http_request_t *r) > +{ [...] > + len += header[i].key.len + sizeof(": ") - 1 > + + header[i].value.len + sizeof(CRLF) - 1; > + } > + > + ctx = ngx_http_get_module_ctx(r, ngx_http_chunked_filter_module); Usual approach is to pass context into internal functions if needed and already available in the calling functions. static ngx_chain_t * -ngx_http_chunked_create_trailers(ngx_http_request_t *r) +ngx_http_chunked_create_trailers(ngx_http_request_t *r, + ngx_http_chunked_filter_ctx_t *ctx) { (and more, see full patch below) > + > + cl = ngx_chain_get_free_buf(r->pool, &ctx->free); > + if (cl == NULL) { > + return NULL; > + } > + > + b = cl->buf; > + > + b->tag = (ngx_buf_tag_t) &ngx_http_chunked_filter_module; > + b->temporary = 0; > + b->memory = 1; > + b->last_buf = 1; > + > + b->start = ngx_palloc(r->pool, len); > + if (b->start == NULL) { > + return NULL; > + } I would prefer to preserve the typical code path (when there are no trailers) without an extra allocation. It looks like it would be as trivail as: @@ -273,14 +273,18 @@ ngx_http_chunked_create_trailers(ngx_htt b->memory = 1; b->last_buf = 1; + if (len == sizeof(CRLF "0" CRLF CRLF) - 1) { + b->pos = (u_char *) CRLF "0" CRLF CRLF; + b->last = b->pos + sizeof(CRLF "0" CRLF CRLF) - 1; + return cl; + } + b->start = ngx_palloc(r->pool, len); if (b->start == NULL) { return NULL; } Note well that b->start is intentionally not touched in the previous code. As buffers are reused, b->start, if set, is expected to point to a chunk of memory big enough to contain "0000000000000000" CRLF, as allocated in the ngx_http_chunked_body_filter(). While this is not critical in this particular code path, as last-chunk is expected to be only created once, the resulting code is confusing: while it provides b->tag to make the buffer reusable, it doesn't maintain required invariant on b->start. Trivial solution would be to avoid setting b->start / b->end as it was done in the previous code and still done in the CRLF case. - b->start = ngx_palloc(r->pool, len); - if (b->start == NULL) { + b->pos = ngx_palloc(r->pool, len); + if (b->pos == NULL) { return NULL; } - b->end = b->last + len; - b->pos = b->start; - b->last = b->start; + b->last = b->pos; Full patch with the above comments: diff --git a/src/http/modules/ngx_http_chunked_filter_module.c b/src/http/modules/ngx_http_chunked_filter_module.c --- a/src/http/modules/ngx_http_chunked_filter_module.c +++ b/src/http/modules/ngx_http_chunked_filter_module.c @@ -17,7 +17,8 @@ typedef struct { static ngx_int_t ngx_http_chunked_filter_init(ngx_conf_t *cf); -static ngx_chain_t *ngx_http_chunked_create_trailers(ngx_http_request_t *r); +static ngx_chain_t *ngx_http_chunked_create_trailers(ngx_http_request_t *r, + ngx_http_chunked_filter_ctx_t *ctx); static ngx_http_module_t ngx_http_chunked_filter_module_ctx = { @@ -70,8 +71,9 @@ ngx_http_chunked_header_filter(ngx_http_ return ngx_http_next_header_filter(r); } - if (r->headers_out.content_length_n == -1 || r->expect_trailers) { - + if (r->headers_out.content_length_n == -1 + || r->expect_trailers) + { clcf = ngx_http_get_module_loc_conf(r, ngx_http_core_module); if (r->http_version >= NGX_HTTP_VERSION_11 @@ -181,7 +183,7 @@ ngx_http_chunked_body_filter(ngx_http_re } if (cl->buf->last_buf) { - tl = ngx_http_chunked_create_trailers(r); + tl = ngx_http_chunked_create_trailers(r, ctx); if (tl == NULL) { return NGX_ERROR; } @@ -224,15 +226,15 @@ ngx_http_chunked_body_filter(ngx_http_re static ngx_chain_t * -ngx_http_chunked_create_trailers(ngx_http_request_t *r) +ngx_http_chunked_create_trailers(ngx_http_request_t *r, + ngx_http_chunked_filter_ctx_t *ctx) { - size_t len; - ngx_buf_t *b; - ngx_uint_t i; - ngx_chain_t *cl; - ngx_list_part_t *part; - ngx_table_elt_t *header; - ngx_http_chunked_filter_ctx_t *ctx; + size_t len; + ngx_buf_t *b; + ngx_uint_t i; + ngx_chain_t *cl; + ngx_list_part_t *part; + ngx_table_elt_t *header; len = sizeof(CRLF "0" CRLF CRLF) - 1; @@ -259,8 +261,6 @@ ngx_http_chunked_create_trailers(ngx_htt + header[i].value.len + sizeof(CRLF) - 1; } - ctx = ngx_http_get_module_ctx(r, ngx_http_chunked_filter_module); - cl = ngx_chain_get_free_buf(r->pool, &ctx->free); if (cl == NULL) { return NULL; @@ -273,14 +273,18 @@ ngx_http_chunked_create_trailers(ngx_htt b->memory = 1; b->last_buf = 1; - b->start = ngx_palloc(r->pool, len); - if (b->start == NULL) { + if (len == sizeof(CRLF "0" CRLF CRLF) - 1) { + b->pos = (u_char *) CRLF "0" CRLF CRLF; + b->last = b->pos + sizeof(CRLF "0" CRLF CRLF) - 1; + return cl; + } + + b->pos = ngx_palloc(r->pool, len); + if (b->pos == NULL) { return NULL; } - b->end = b->last + len; - b->pos = b->start; - b->last = b->start; + b->last = b->pos; *b->last++ = CR; *b->last++ = LF; *b->last++ = '0'; -- Maxim Dounin http://nginx.org/ _______________________________________________ nginx-devel mailing list [email protected] http://mailman.nginx.org/mailman/listinfo/nginx-devel
