On Friday 02 June 2017 20:33:46 Piotr Sikora via nginx-devel wrote:
> # HG changeset patch
> # User Piotr Sikora <piotrsik...@google.com>
> # Date 1493191954 25200
> # Wed Apr 26 00:32:34 2017 -0700
> # Node ID 8d74ff6c2015180f5c1f399f492214d7d0a52b3f
> # Parent 41c09a2fd90410e25ad8515793bd48028001c954
> HTTP/2: added support for trailers in HTTP responses.
>
> Signed-off-by: Piotr Sikora <piotrsik...@google.com>
>
> diff -r 41c09a2fd904 -r 8d74ff6c2015 src/http/v2/ngx_http_v2_filter_module.c
> --- a/src/http/v2/ngx_http_v2_filter_module.c
> +++ b/src/http/v2/ngx_http_v2_filter_module.c
> @@ -50,13 +50,17 @@
> #define NGX_HTTP_V2_SERVER_INDEX 54
> #define NGX_HTTP_V2_VARY_INDEX 59
>
> +#define NGX_HTTP_V2_FRAME_ERROR (ngx_http_v2_out_frame_t *) -1
> +
>
> static u_char *ngx_http_v2_string_encode(u_char *dst, u_char *src, size_t
> len,
> u_char *tmp, ngx_uint_t lower);
> static u_char *ngx_http_v2_write_int(u_char *pos, ngx_uint_t prefix,
> ngx_uint_t value);
> static ngx_http_v2_out_frame_t *ngx_http_v2_create_headers_frame(
> - ngx_http_request_t *r, u_char *pos, u_char *end);
> + ngx_http_request_t *r, u_char *pos, u_char *end, ngx_uint_t fin);
> +static ngx_http_v2_out_frame_t *ngx_http_v2_create_trailers_frame(
> + ngx_http_request_t *r);
>
> static ngx_chain_t *ngx_http_v2_send_chain(ngx_connection_t *fc,
> ngx_chain_t *in, off_t limit);
> @@ -612,7 +616,7 @@ ngx_http_v2_header_filter(ngx_http_reque
> header[i].value.len, tmp);
> }
>
> - frame = ngx_http_v2_create_headers_frame(r, start, pos);
> + frame = ngx_http_v2_create_headers_frame(r, start, pos, r->header_only);
> if (frame == NULL) {
> return NGX_ERROR;
> }
> @@ -636,6 +640,126 @@ ngx_http_v2_header_filter(ngx_http_reque
> }
>
>
> +static ngx_http_v2_out_frame_t *
> +ngx_http_v2_create_trailers_frame(ngx_http_request_t *r)
> +{
[..]
> +
> + frame = ngx_http_v2_create_headers_frame(r, start, pos, 1);
> + if (frame == NULL) {
> + return NGX_HTTP_V2_FRAME_ERROR;
> + }
> +
> + return frame;
> +}
It's better to keep return values consistent with
ngx_http_v2_create_headers_frame() and introduce
NGX_HTTP_V2_NO_TRAILERS instead of NGX_HTTP_V2_FRAME_ERROR.
[..]
> @@ -934,17 +1060,36 @@ ngx_http_v2_send_chain(ngx_connection_t
> size -= rest;
> }
>
> - frame = ngx_http_v2_filter_get_data_frame(stream, frame_size, out,
> cl);
> - if (frame == NULL) {
> - return NGX_CHAIN_ERROR;
> + if (cl->buf->last_buf) {
> + trailers = ngx_http_v2_create_trailers_frame(r);
> + if (trailers == NGX_HTTP_V2_FRAME_ERROR) {
> + return NGX_CHAIN_ERROR;
> + }
> +
> + if (trailers) {
> + cl->buf->last_buf = 0;
> + }
> }
>
> - ngx_http_v2_queue_frame(h2c, frame);
> + if (frame_size || cl->buf->last_buf) {
> + frame = ngx_http_v2_filter_get_data_frame(stream, frame_size,
> out,
> + cl);
> + if (frame == NULL) {
> + return NGX_CHAIN_ERROR;
> + }
>
> - h2c->send_window -= frame_size;
> + ngx_http_v2_queue_frame(h2c, frame);
>
> - stream->send_window -= frame_size;
> - stream->queued++;
> + h2c->send_window -= frame_size;
> +
> + stream->send_window -= frame_size;
> + stream->queued++;
> + }
> +
> + if (trailers) {
> + ngx_http_v2_queue_frame(h2c, trailers);
> + stream->queued++;
> + }
>
> if (in == NULL) {
> break;
There's no reason to check "trailers" on each iteration.
I think you can put it inside the "if (in == NULL)" condition.
Please consider the changes below.
wbr, Valentin V. Bartenev
diff -r 0e2f2f8b5c9b src/http/v2/ngx_http_v2_filter_module.c
--- a/src/http/v2/ngx_http_v2_filter_module.c Wed Apr 26 00:32:34 2017 -0700
+++ b/src/http/v2/ngx_http_v2_filter_module.c Wed Jun 07 22:10:04 2017 +0300
@@ -50,7 +50,7 @@
#define NGX_HTTP_V2_SERVER_INDEX 54
#define NGX_HTTP_V2_VARY_INDEX 59
-#define NGX_HTTP_V2_FRAME_ERROR (ngx_http_v2_out_frame_t *) -1
+#define NGX_HTTP_V2_NO_TRAILERS (ngx_http_v2_out_frame_t *) -1
static u_char *ngx_http_v2_string_encode(u_char *dst, u_char *src, size_t len,
@@ -643,12 +643,11 @@ ngx_http_v2_header_filter(ngx_http_reque
static ngx_http_v2_out_frame_t *
ngx_http_v2_create_trailers_frame(ngx_http_request_t *r)
{
- u_char *pos, *start, *tmp;
- size_t len, tmp_len;
- ngx_uint_t i;
- ngx_list_part_t *part;
- ngx_table_elt_t *header;
- ngx_http_v2_out_frame_t *frame;
+ u_char *pos, *start, *tmp;
+ size_t len, tmp_len;
+ ngx_uint_t i;
+ ngx_list_part_t *part;
+ ngx_table_elt_t *header;
len = 0;
tmp_len = 0;
@@ -677,7 +676,7 @@ ngx_http_v2_create_trailers_frame(ngx_ht
"too long response trailer name: \"%V\"",
&header[i].key);
- return NGX_HTTP_V2_FRAME_ERROR;
+ return NULL;
}
if (header[i].value.len > NGX_HTTP_V2_MAX_FIELD) {
@@ -685,7 +684,7 @@ ngx_http_v2_create_trailers_frame(ngx_ht
"too long response trailer value: \"%V: %V\"",
&header[i].key, &header[i].value);
- return NGX_HTTP_V2_FRAME_ERROR;
+ return NULL;
}
len += 1 + NGX_HTTP_V2_INT_OCTETS + header[i].key.len
@@ -701,14 +700,14 @@ ngx_http_v2_create_trailers_frame(ngx_ht
}
if (len == 0) {
- return NULL;
+ return NGX_HTTP_V2_NO_TRAILERS;
}
tmp = ngx_palloc(r->pool, tmp_len);
pos = ngx_pnalloc(r->pool, len);
if (pos == NULL || tmp == NULL) {
- return NGX_HTTP_V2_FRAME_ERROR;
+ return NULL;
}
start = pos;
@@ -751,12 +750,7 @@ ngx_http_v2_create_trailers_frame(ngx_ht
header[i].value.len, tmp);
}
- frame = ngx_http_v2_create_headers_frame(r, start, pos, 1);
- if (frame == NULL) {
- return NGX_HTTP_V2_FRAME_ERROR;
- }
-
- return frame;
+ return ngx_http_v2_create_headers_frame(r, start, pos, 1);
}
@@ -996,7 +990,7 @@ ngx_http_v2_send_chain(ngx_connection_t
frame_size = (h2lcf->chunk_size < h2c->frame_size)
? h2lcf->chunk_size : h2c->frame_size;
- trailers = NULL;
+ trailers = NGX_HTTP_V2_NO_TRAILERS;
#if (NGX_SUPPRESS_WARN)
cl = NULL;
@@ -1062,18 +1056,18 @@ ngx_http_v2_send_chain(ngx_connection_t
if (cl->buf->last_buf) {
trailers = ngx_http_v2_create_trailers_frame(r);
- if (trailers == NGX_HTTP_V2_FRAME_ERROR) {
+ if (trailers == NULL) {
return NGX_CHAIN_ERROR;
}
- if (trailers) {
+ if (trailers != NGX_HTTP_V2_NO_TRAILERS) {
cl->buf->last_buf = 0;
}
}
if (frame_size || cl->buf->last_buf) {
- frame = ngx_http_v2_filter_get_data_frame(stream, frame_size, out,
- cl);
+ frame = ngx_http_v2_filter_get_data_frame(stream, frame_size,
+ out, cl);
if (frame == NULL) {
return NGX_CHAIN_ERROR;
}
@@ -1086,12 +1080,13 @@ ngx_http_v2_send_chain(ngx_connection_t
stream->queued++;
}
- if (trailers) {
- ngx_http_v2_queue_frame(h2c, trailers);
- stream->queued++;
- }
+ if (in == NULL) {
- if (in == NULL) {
+ if (trailers != NGX_HTTP_V2_NO_TRAILERS) {
+ ngx_http_v2_queue_frame(h2c, trailers);
+ stream->queued++;
+ }
+
break;
}
_______________________________________________
nginx-devel mailing list
nginx-devel@nginx.org
http://mailman.nginx.org/mailman/listinfo/nginx-devel
