Hello!

On Fri, Jun 02, 2017 at 08:33:45PM -0700, Piotr Sikora via nginx-devel wrote:

> # HG changeset patch
> # User Piotr Sikora <piotrsikora@google.com>
> # 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 <piotrsikora@google.com>

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
nginx-devel@nginx.org
http://mailman.nginx.org/mailman/listinfo/nginx-devel