subject:"\[PATCH 2 of 3\] HTTP\/2\: added support for trailers in HTTP responses"

Re: [PATCH 2 of 3] HTTP/2: added support for trailers in HTTP responses

2017-06-17 Thread Piotr Sikora via nginx-devel

Hey Valentin,

> I've overlooked this while doing previous review, but it looks strange.
>
> Why do you use NGX_LOG_WARN for trailers headers?  It results in
> finalizing request with an error (in case of HTTP/2 it means RST_STREAM).
>
> For main headers the NGX_LOG_CRIT level is used.  It looks too serious,
> but the WARN level is too low.

Good catch, thanks!

This was left-over from the initial version, which skipped too long
trailers, instead of resetting stream.

Best regards,
Piotr Sikora
___
nginx-devel mailing list
nginx-devel@nginx.org
http://mailman.nginx.org/mailman/listinfo/nginx-devel

Re: [PATCH 2 of 3] HTTP/2: added support for trailers in HTTP responses

2017-06-17 Thread Piotr Sikora via nginx-devel

Hey Maxim,

> Series committed with the above change.
> Thanks to all involved.

Thanks! :)

Best regards,
Piotr Sikora
___
nginx-devel mailing list
nginx-devel@nginx.org
http://mailman.nginx.org/mailman/listinfo/nginx-devel

Re: [PATCH 2 of 3] HTTP/2: added support for trailers in HTTP responses

2017-06-15 Thread Maxim Dounin

Hello!

On Wed, Jun 14, 2017 at 08:33:34PM +0300, Maxim Dounin wrote:

> Hello!
> 
> On Wed, Jun 14, 2017 at 08:06:02PM +0300, Valentin V. Bartenev wrote:
> 
> > On Wednesday 14 June 2017 19:54:47 Maxim Dounin wrote:
> > > Hello!
> > > 
> > > On Wed, Jun 14, 2017 at 06:12:25PM +0300, Valentin V. Bartenev wrote:
> > > 
> > > > On Tuesday 13 June 2017 05:19:54 Piotr Sikora via nginx-devel wrote:
> > > > > # HG changeset patch
> > > > > # User Piotr Sikora 
> > > > > # Date 1490351854 25200
> > > > > #  Fri Mar 24 03:37:34 2017 -0700
> > > > > # Node ID 73f67e06ab103e0368d1810c6f8cac5c70c4e246
> > > > > # Parent  07a5d26b49f04425ff54cc998f885aa987b7823f
> > > > > HTTP/2: added support for trailers in HTTP responses.
> > > > > 
> > > > > Signed-off-by: Piotr Sikora 
> > > > > 
> > > > [..]
> > > > > +
> > > > > +if (header[i].key.len > NGX_HTTP_V2_MAX_FIELD) {
> > > > > +ngx_log_error(NGX_LOG_WARN, r->connection->log, 0,
> > > > > +  "too long response trailer name: \"%V\"",
> > > > > +  [i].key);
> > > > > +return NULL;
> > > > > +}
> > > > > +
> > > > > +if (header[i].value.len > NGX_HTTP_V2_MAX_FIELD) {
> > > > > +ngx_log_error(NGX_LOG_WARN, r->connection->log, 0,
> > > > > +  "too long response trailer value: \"%V: 
> > > > > %V\"",
> > > > > +  [i].key, [i].value);
> > > > > +return NULL;
> > > > > +}
> > > > [..]
> > > > 
> > > > I've overlooked this while doing previous review, but it looks strange.
> > > > 
> > > > Why do you use NGX_LOG_WARN for trailers headers?  It results in
> > > > finalizing request with an error (in case of HTTP/2 it means 
> > > > RST_STREAM).
> > > > 
> > > > For main headers the NGX_LOG_CRIT level is used.  It looks too serious,
> > > > but the WARN level is too low.
> > > > 
> > > > It seems the right log level for both cases is NGX_LOG_ERR.
> > > > 
> > > > So I'm going to commit the patch below:
> > > 
> > > [...]
> > > 
> > > Given that the limit is about 2 megabytes, I don't think that this 
> > > can be triggered in practice by a backend.  As such, NGX_LOG_CRIT 
> > > looks logical, as the only practically possible reason for the 
> > > test to fail is a bug somewhere.
> > > 
> > 
> > Fair enough, I agree.
> > 
> > Anyway, that's should be the same for trailers (i.e. CRIT instead of WARN).
> 
> Sure.
> 
> The other patches looks good to me, so I'm going to commit the 
> series with the following additional change to this patch unless 
> there are objections:
> 
> --- a/src/http/v2/ngx_http_v2_filter_module.c
> +++ b/src/http/v2/ngx_http_v2_filter_module.c
> @@ -672,14 +672,14 @@ ngx_http_v2_create_trailers_frame(ngx_ht
>  }
>  
>  if (header[i].key.len > NGX_HTTP_V2_MAX_FIELD) {
> -ngx_log_error(NGX_LOG_WARN, r->connection->log, 0,
> +ngx_log_error(NGX_LOG_CRIT, r->connection->log, 0,
>"too long response trailer name: \"%V\"",
>[i].key);
>  return NULL;
>  }
>  
>  if (header[i].value.len > NGX_HTTP_V2_MAX_FIELD) {
> -ngx_log_error(NGX_LOG_WARN, r->connection->log, 0,
> +ngx_log_error(NGX_LOG_CRIT, r->connection->log, 0,
>"too long response trailer value: \"%V: %V\"",
>[i].key, [i].value);
>  return NULL;

Series committed with the above change.
Thanks to all involved.

-- 
Maxim Dounin
http://nginx.org/
___
nginx-devel mailing list
nginx-devel@nginx.org
http://mailman.nginx.org/mailman/listinfo/nginx-devel

Re: [PATCH 2 of 3] HTTP/2: added support for trailers in HTTP responses

2017-06-14 Thread Maxim Dounin

Hello!

On Wed, Jun 14, 2017 at 08:06:02PM +0300, Valentin V. Bartenev wrote:

> On Wednesday 14 June 2017 19:54:47 Maxim Dounin wrote:
> > Hello!
> > 
> > On Wed, Jun 14, 2017 at 06:12:25PM +0300, Valentin V. Bartenev wrote:
> > 
> > > On Tuesday 13 June 2017 05:19:54 Piotr Sikora via nginx-devel wrote:
> > > > # HG changeset patch
> > > > # User Piotr Sikora 
> > > > # Date 1490351854 25200
> > > > #  Fri Mar 24 03:37:34 2017 -0700
> > > > # Node ID 73f67e06ab103e0368d1810c6f8cac5c70c4e246
> > > > # Parent  07a5d26b49f04425ff54cc998f885aa987b7823f
> > > > HTTP/2: added support for trailers in HTTP responses.
> > > > 
> > > > Signed-off-by: Piotr Sikora 
> > > > 
> > > [..]
> > > > +
> > > > +if (header[i].key.len > NGX_HTTP_V2_MAX_FIELD) {
> > > > +ngx_log_error(NGX_LOG_WARN, r->connection->log, 0,
> > > > +  "too long response trailer name: \"%V\"",
> > > > +  [i].key);
> > > > +return NULL;
> > > > +}
> > > > +
> > > > +if (header[i].value.len > NGX_HTTP_V2_MAX_FIELD) {
> > > > +ngx_log_error(NGX_LOG_WARN, r->connection->log, 0,
> > > > +  "too long response trailer value: \"%V: 
> > > > %V\"",
> > > > +  [i].key, [i].value);
> > > > +return NULL;
> > > > +}
> > > [..]
> > > 
> > > I've overlooked this while doing previous review, but it looks strange.
> > > 
> > > Why do you use NGX_LOG_WARN for trailers headers?  It results in
> > > finalizing request with an error (in case of HTTP/2 it means RST_STREAM).
> > > 
> > > For main headers the NGX_LOG_CRIT level is used.  It looks too serious,
> > > but the WARN level is too low.
> > > 
> > > It seems the right log level for both cases is NGX_LOG_ERR.
> > > 
> > > So I'm going to commit the patch below:
> > 
> > [...]
> > 
> > Given that the limit is about 2 megabytes, I don't think that this 
> > can be triggered in practice by a backend.  As such, NGX_LOG_CRIT 
> > looks logical, as the only practically possible reason for the 
> > test to fail is a bug somewhere.
> > 
> 
> Fair enough, I agree.
> 
> Anyway, that's should be the same for trailers (i.e. CRIT instead of WARN).

Sure.

The other patches looks good to me, so I'm going to commit the 
series with the following additional change to this patch unless 
there are objections:

--- a/src/http/v2/ngx_http_v2_filter_module.c
+++ b/src/http/v2/ngx_http_v2_filter_module.c
@@ -672,14 +672,14 @@ ngx_http_v2_create_trailers_frame(ngx_ht
 }
 
 if (header[i].key.len > NGX_HTTP_V2_MAX_FIELD) {
-ngx_log_error(NGX_LOG_WARN, r->connection->log, 0,
+ngx_log_error(NGX_LOG_CRIT, r->connection->log, 0,
   "too long response trailer name: \"%V\"",
   [i].key);
 return NULL;
 }
 
 if (header[i].value.len > NGX_HTTP_V2_MAX_FIELD) {
-ngx_log_error(NGX_LOG_WARN, r->connection->log, 0,
+ngx_log_error(NGX_LOG_CRIT, r->connection->log, 0,
   "too long response trailer value: \"%V: %V\"",
   [i].key, [i].value);
 return NULL;

-- 
Maxim Dounin
http://nginx.org/
___
nginx-devel mailing list
nginx-devel@nginx.org
http://mailman.nginx.org/mailman/listinfo/nginx-devel

Re: [PATCH 2 of 3] HTTP/2: added support for trailers in HTTP responses

2017-06-14 Thread Valentin V. Bartenev

On Wednesday 14 June 2017 19:54:47 Maxim Dounin wrote:
> Hello!
> 
> On Wed, Jun 14, 2017 at 06:12:25PM +0300, Valentin V. Bartenev wrote:
> 
> > On Tuesday 13 June 2017 05:19:54 Piotr Sikora via nginx-devel wrote:
> > > # HG changeset patch
> > > # User Piotr Sikora 
> > > # Date 1490351854 25200
> > > #  Fri Mar 24 03:37:34 2017 -0700
> > > # Node ID 73f67e06ab103e0368d1810c6f8cac5c70c4e246
> > > # Parent  07a5d26b49f04425ff54cc998f885aa987b7823f
> > > HTTP/2: added support for trailers in HTTP responses.
> > > 
> > > Signed-off-by: Piotr Sikora 
> > > 
> > [..]
> > > +
> > > +if (header[i].key.len > NGX_HTTP_V2_MAX_FIELD) {
> > > +ngx_log_error(NGX_LOG_WARN, r->connection->log, 0,
> > > +  "too long response trailer name: \"%V\"",
> > > +  [i].key);
> > > +return NULL;
> > > +}
> > > +
> > > +if (header[i].value.len > NGX_HTTP_V2_MAX_FIELD) {
> > > +ngx_log_error(NGX_LOG_WARN, r->connection->log, 0,
> > > +  "too long response trailer value: \"%V: %V\"",
> > > +  [i].key, [i].value);
> > > +return NULL;
> > > +}
> > [..]
> > 
> > I've overlooked this while doing previous review, but it looks strange.
> > 
> > Why do you use NGX_LOG_WARN for trailers headers?  It results in
> > finalizing request with an error (in case of HTTP/2 it means RST_STREAM).
> > 
> > For main headers the NGX_LOG_CRIT level is used.  It looks too serious,
> > but the WARN level is too low.
> > 
> > It seems the right log level for both cases is NGX_LOG_ERR.
> > 
> > So I'm going to commit the patch below:
> 
> [...]
> 
> Given that the limit is about 2 megabytes, I don't think that this 
> can be triggered in practice by a backend.  As such, NGX_LOG_CRIT 
> looks logical, as the only practically possible reason for the 
> test to fail is a bug somewhere.
> 

Fair enough, I agree.

Anyway, that's should be the same for trailers (i.e. CRIT instead of WARN).

  wbr, Valentin V. Bartenev

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

Re: [PATCH 2 of 3] HTTP/2: added support for trailers in HTTP responses

2017-06-14 Thread Maxim Dounin

Hello!

On Wed, Jun 14, 2017 at 06:12:25PM +0300, Valentin V. Bartenev wrote:

> On Tuesday 13 June 2017 05:19:54 Piotr Sikora via nginx-devel wrote:
> > # HG changeset patch
> > # User Piotr Sikora 
> > # Date 1490351854 25200
> > #  Fri Mar 24 03:37:34 2017 -0700
> > # Node ID 73f67e06ab103e0368d1810c6f8cac5c70c4e246
> > # Parent  07a5d26b49f04425ff54cc998f885aa987b7823f
> > HTTP/2: added support for trailers in HTTP responses.
> > 
> > Signed-off-by: Piotr Sikora 
> > 
> [..]
> > +
> > +if (header[i].key.len > NGX_HTTP_V2_MAX_FIELD) {
> > +ngx_log_error(NGX_LOG_WARN, r->connection->log, 0,
> > +  "too long response trailer name: \"%V\"",
> > +  [i].key);
> > +return NULL;
> > +}
> > +
> > +if (header[i].value.len > NGX_HTTP_V2_MAX_FIELD) {
> > +ngx_log_error(NGX_LOG_WARN, r->connection->log, 0,
> > +  "too long response trailer value: \"%V: %V\"",
> > +  [i].key, [i].value);
> > +return NULL;
> > +}
> [..]
> 
> I've overlooked this while doing previous review, but it looks strange.
> 
> Why do you use NGX_LOG_WARN for trailers headers?  It results in
> finalizing request with an error (in case of HTTP/2 it means RST_STREAM).
> 
> For main headers the NGX_LOG_CRIT level is used.  It looks too serious,
> but the WARN level is too low.
> 
> It seems the right log level for both cases is NGX_LOG_ERR.
> 
> So I'm going to commit the patch below:

[...]

Given that the limit is about 2 megabytes, I don't think that this 
can be triggered in practice by a backend.  As such, NGX_LOG_CRIT 
looks logical, as the only practically possible reason for the 
test to fail is a bug somewhere.

-- 
Maxim Dounin
http://nginx.org/
___
nginx-devel mailing list
nginx-devel@nginx.org
http://mailman.nginx.org/mailman/listinfo/nginx-devel

Re: [PATCH 2 of 3] HTTP/2: added support for trailers in HTTP responses

2017-06-14 Thread Valentin V. Bartenev

On Tuesday 13 June 2017 05:19:54 Piotr Sikora via nginx-devel wrote:
> # HG changeset patch
> # User Piotr Sikora 
> # Date 1490351854 25200
> #  Fri Mar 24 03:37:34 2017 -0700
> # Node ID 73f67e06ab103e0368d1810c6f8cac5c70c4e246
> # Parent  07a5d26b49f04425ff54cc998f885aa987b7823f
> HTTP/2: added support for trailers in HTTP responses.
> 
> Signed-off-by: Piotr Sikora 
> 
[..]
> +
> +if (header[i].key.len > NGX_HTTP_V2_MAX_FIELD) {
> +ngx_log_error(NGX_LOG_WARN, r->connection->log, 0,
> +  "too long response trailer name: \"%V\"",
> +  [i].key);
> +return NULL;
> +}
> +
> +if (header[i].value.len > NGX_HTTP_V2_MAX_FIELD) {
> +ngx_log_error(NGX_LOG_WARN, r->connection->log, 0,
> +  "too long response trailer value: \"%V: %V\"",
> +  [i].key, [i].value);
> +return NULL;
> +}
[..]

I've overlooked this while doing previous review, but it looks strange.

Why do you use NGX_LOG_WARN for trailers headers?  It results in
finalizing request with an error (in case of HTTP/2 it means RST_STREAM).

For main headers the NGX_LOG_CRIT level is used.  It looks too serious,
but the WARN level is too low.

It seems the right log level for both cases is NGX_LOG_ERR.

So I'm going to commit the patch below:

# HG changeset patch
# User Valentin Bartenev 
# Date 1497453034 -10800
#  Wed Jun 14 18:10:34 2017 +0300
# Node ID 460ed2f4e160bc6b0953456d1218381cdfc7e40b
# Parent  3c55863e6887ee764265fd43a878b3f793e0a7b9
HTTP/2: adjusted log level for too long response headers.

It's likely an upstream fault and the CRIT log level looks too high.

diff -r 3c55863e6887 -r 460ed2f4e160 src/http/v2/ngx_http_v2_filter_module.c
--- a/src/http/v2/ngx_http_v2_filter_module.c   Tue Jun 13 17:01:08 2017 +0300
+++ b/src/http/v2/ngx_http_v2_filter_module.c   Wed Jun 14 18:10:34 2017 +0300
@@ -385,14 +385,14 @@ ngx_http_v2_header_filter(ngx_http_reque
 }
 
 if (header[i].key.len > NGX_HTTP_V2_MAX_FIELD) {
-ngx_log_error(NGX_LOG_CRIT, fc->log, 0,
+ngx_log_error(NGX_LOG_ERR, fc->log, 0,
   "too long response header name: \"%V\"",
   [i].key);
 return NGX_ERROR;
 }
 
 if (header[i].value.len > NGX_HTTP_V2_MAX_FIELD) {
-ngx_log_error(NGX_LOG_CRIT, fc->log, 0,
+ngx_log_error(NGX_LOG_ERR, fc->log, 0,
   "too long response header value: \"%V: %V\"",
   [i].key, [i].value);
 return NGX_ERROR;

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

Re: [PATCH 2 of 3] HTTP/2: added support for trailers in HTTP responses

2017-06-13 Thread Piotr Sikora via nginx-devel

Hey Valentin,

> 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.

NGX_HTTP_V2_NO_TRAILERS feels a bit weird, but whatever works for you is fine.

> There's no reason to check "trailers" on each iteration.
> I think you can put it inside the "if (in == NULL)" condition.

Good catch, thanks!

> Please consider the changes below.

Applied (with removed empty lines between error message and "return NULL").

Thanks!

Best regards,
Piotr Sikora
___
nginx-devel mailing list
nginx-devel@nginx.org
http://mailman.nginx.org/mailman/listinfo/nginx-devel

[PATCH 2 of 3] HTTP/2: added support for trailers in HTTP responses

2017-06-13 Thread Piotr Sikora via nginx-devel

# HG changeset patch
# User Piotr Sikora 
# Date 1490351854 25200
#  Fri Mar 24 03:37:34 2017 -0700
# Node ID 73f67e06ab103e0368d1810c6f8cac5c70c4e246
# Parent  07a5d26b49f04425ff54cc998f885aa987b7823f
HTTP/2: added support for trailers in HTTP responses.

Signed-off-by: Piotr Sikora 

diff -r 07a5d26b49f0 -r 73f67e06ab10 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_INDEX59
 
+#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,
 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,118 @@ 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_tlen, tmp_len;
+ngx_uint_ti;
+ngx_list_part_t  *part;
+ngx_table_elt_t  *header;
+
+len = 0;
+tmp_len = 0;
+
+part = >headers_out.trailers.part;
+header = part->elts;
+
+for (i = 0; /* void */; i++) {
+
+if (i >= part->nelts) {
+if (part->next == NULL) {
+break;
+}
+
+part = part->next;
+header = part->elts;
+i = 0;
+}
+
+if (header[i].hash == 0) {
+continue;
+}
+
+if (header[i].key.len > NGX_HTTP_V2_MAX_FIELD) {
+ngx_log_error(NGX_LOG_WARN, r->connection->log, 0,
+  "too long response trailer name: \"%V\"",
+  [i].key);
+return NULL;
+}
+
+if (header[i].value.len > NGX_HTTP_V2_MAX_FIELD) {
+ngx_log_error(NGX_LOG_WARN, r->connection->log, 0,
+  "too long response trailer value: \"%V: %V\"",
+  [i].key, [i].value);
+return NULL;
+}
+
+len += 1 + NGX_HTTP_V2_INT_OCTETS + header[i].key.len
+ + NGX_HTTP_V2_INT_OCTETS + header[i].value.len;
+
+if (header[i].key.len > tmp_len) {
+tmp_len = header[i].key.len;
+}
+
+if (header[i].value.len > tmp_len) {
+tmp_len = header[i].value.len;
+}
+}
+
+if (len == 0) {
+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 NULL;
+}
+
+start = pos;
+
+part = >headers_out.trailers.part;
+header = part->elts;
+
+for (i = 0; /* void */; i++) {
+
+if (i >= part->nelts) {
+if (part->next == NULL) {
+break;
+}
+
+part = part->next;
+header = part->elts;
+i = 0;
+}
+
+if (header[i].hash == 0) {
+continue;
+}
+
+#if (NGX_DEBUG)
+if (r->connection->log->log_level & NGX_LOG_DEBUG_HTTP) {
+ngx_strlow(tmp, header[i].key.data, header[i].key.len);
+
+ngx_log_debug3(NGX_LOG_DEBUG_HTTP, r->connection->log, 0,
+   "http2 output trailer: \"%*s: %V\"",
+   header[i].key.len, tmp, [i].value);
+}
+#endif
+
+*pos++ = 0;
+
+pos = ngx_http_v2_write_name(pos, header[i].key.data,
+ header[i].key.len, tmp);
+
+pos = ngx_http_v2_write_value(pos, header[i].value.data,
+  header[i].value.len, tmp);
+}
+
+return ngx_http_v2_create_headers_frame(r, start, pos, 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)
@@ -686,7 +802,7 @@ ngx_http_v2_write_int(u_char *pos, ngx_u
 
 static ngx_http_v2_out_frame_t *
 ngx_http_v2_create_headers_frame(ngx_http_request_t *r, u_char *pos,
-u_char *end)
+u_char *end, ngx_uint_t fin)

Re: [PATCH 2 of 3] HTTP/2: added support for trailers in HTTP responses

2017-06-07 Thread Valentin V. Bartenev

On Friday 02 June 2017 20:33:46 Piotr Sikora via nginx-devel wrote:
> # HG changeset patch
> # User Piotr Sikora 
> # 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 
> 
> 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_INDEX59
>  
> +#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_INDEX59
 
-#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_tlen, tmp_len;
-ngx_uint_ti;
-ngx_list_part_t  *part;
-ngx_table_elt_t  *header;
-ngx_http_v2_out_frame_t  *frame;
+u_char   *pos, *start, *tmp;
+size_tlen,

Re: [PATCH 2 of 3] HTTP/2: added support for trailers in HTTP responses

2017-06-06 Thread Maxim Konovalov

On 05/06/2017 21:00, Maxim Dounin wrote:
> Hello!
> 
> On Fri, Jun 02, 2017 at 08:33:46PM -0700, Piotr Sikora via nginx-devel wrote:
> 
>> # HG changeset patch
>> # User Piotr Sikora 
>> # 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 
> 
> I've asked Valentin to look into this part.  Hopefully he'll be 
> able to do so in a couple of days.
> 
> [...]
> 
To be precise and to avoid confusion: Valentin is on the conf today
so expect his feedback this week.

-- 
Maxim Konovalov
___
nginx-devel mailing list
nginx-devel@nginx.org
http://mailman.nginx.org/mailman/listinfo/nginx-devel

Re: [PATCH 2 of 3] HTTP/2: added support for trailers in HTTP responses

2017-06-05 Thread Maxim Dounin

Hello!

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

> # HG changeset patch
> # User Piotr Sikora 
> # 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 

I've asked Valentin to look into this part.  Hopefully he'll be 
able to do so in a couple of days.

[...]

-- 
Maxim Dounin
http://nginx.org/
___
nginx-devel mailing list
nginx-devel@nginx.org
http://mailman.nginx.org/mailman/listinfo/nginx-devel

[PATCH 2 of 3] HTTP/2: added support for trailers in HTTP responses

2017-06-02 Thread Piotr Sikora via nginx-devel

# HG changeset patch
# User Piotr Sikora 
# 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 

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_INDEX59
 
+#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)
+{
+u_char   *pos, *start, *tmp;
+size_tlen, tmp_len;
+ngx_uint_ti;
+ngx_list_part_t  *part;
+ngx_table_elt_t  *header;
+ngx_http_v2_out_frame_t  *frame;
+
+len = 0;
+tmp_len = 0;
+
+part = >headers_out.trailers.part;
+header = part->elts;
+
+for (i = 0; /* void */; i++) {
+
+if (i >= part->nelts) {
+if (part->next == NULL) {
+break;
+}
+
+part = part->next;
+header = part->elts;
+i = 0;
+}
+
+if (header[i].hash == 0) {
+continue;
+}
+
+if (header[i].key.len > NGX_HTTP_V2_MAX_FIELD) {
+ngx_log_error(NGX_LOG_WARN, r->connection->log, 0,
+  "too long response trailer name: \"%V\"",
+  [i].key);
+
+return NGX_HTTP_V2_FRAME_ERROR;
+}
+
+if (header[i].value.len > NGX_HTTP_V2_MAX_FIELD) {
+ngx_log_error(NGX_LOG_WARN, r->connection->log, 0,
+  "too long response trailer value: \"%V: %V\"",
+  [i].key, [i].value);
+
+return NGX_HTTP_V2_FRAME_ERROR;
+}
+
+len += 1 + NGX_HTTP_V2_INT_OCTETS + header[i].key.len
+ + NGX_HTTP_V2_INT_OCTETS + header[i].value.len;
+
+if (header[i].key.len > tmp_len) {
+tmp_len = header[i].key.len;
+}
+
+if (header[i].value.len > tmp_len) {
+tmp_len = header[i].value.len;
+}
+}
+
+if (len == 0) {
+return NULL;
+}
+
+tmp = ngx_palloc(r->pool, tmp_len);
+pos = ngx_pnalloc(r->pool, len);
+
+if (pos == NULL || tmp == NULL) {
+return NGX_HTTP_V2_FRAME_ERROR;
+}
+
+start = pos;
+
+part = >headers_out.trailers.part;
+header = part->elts;
+
+for (i = 0; /* void */; i++) {
+
+if (i >= part->nelts) {
+if (part->next == NULL) {
+break;
+}
+
+part = part->next;
+header = part->elts;
+i = 0;
+}
+
+if (header[i].hash == 0) {
+continue;
+}
+
+#if (NGX_DEBUG)
+if (r->connection->log->log_level & NGX_LOG_DEBUG_HTTP) {
+ngx_strlow(tmp, header[i].key.data, header[i].key.len);
+
+ngx_log_debug3(NGX_LOG_DEBUG_HTTP, r->connection->log, 0,
+   "http2 output trailer: \"%*s: %V\"",
+   header[i].key.len, tmp, [i].value);
+}
+#endif
+
+*pos++ = 0;
+
+pos = ngx_http_v2_write_name(pos, header[i].key.data,
+ header[i].key.len, tmp);
+
+pos = ngx_http_v2_write_value(pos, header[i].value.data,
+  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;
+}
+
+
 static u_char *
 ngx_http_v2_string_encode(u_char *dst, u_char *src, size_t len, u_char *tmp,
 ngx_uint_t lower)
@@

[PATCH 2 of 3] HTTP/2: added support for trailers in HTTP responses

2017-06-02 Thread Piotr Sikora via nginx-devel

# HG changeset patch
# User Piotr Sikora 
# Date 1493191954 25200
#  Wed Apr 26 00:32:34 2017 -0700
# Node ID e84aa49c5bc7a3250d4844b581e4bf3ed42db5f5
# Parent  b0a910ad494158427ba102bdac71ce01d0667f72
HTTP/2: added support for trailers in HTTP responses.

Signed-off-by: Piotr Sikora 

diff -r b0a910ad4941 -r e84aa49c5bc7 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_INDEX59
 
+#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)
+{
+u_char   *pos, *start, *tmp;
+size_tlen, tmp_len;
+ngx_uint_ti;
+ngx_list_part_t  *part;
+ngx_table_elt_t  *header;
+ngx_http_v2_out_frame_t  *frame;
+
+len = 0;
+tmp_len = 0;
+
+part = >headers_out.trailers.part;
+header = part->elts;
+
+for (i = 0; /* void */; i++) {
+
+if (i >= part->nelts) {
+if (part->next == NULL) {
+break;
+}
+
+part = part->next;
+header = part->elts;
+i = 0;
+}
+
+if (header[i].hash == 0) {
+continue;
+}
+
+if (header[i].key.len > NGX_HTTP_V2_MAX_FIELD) {
+ngx_log_error(NGX_LOG_WARN, r->connection->log, 0,
+  "too long response trailer name: \"%V\"",
+  [i].key);
+
+return NGX_HTTP_V2_FRAME_ERROR;
+}
+
+if (header[i].value.len > NGX_HTTP_V2_MAX_FIELD) {
+ngx_log_error(NGX_LOG_WARN, r->connection->log, 0,
+  "too long response trailer value: \"%V: %V\"",
+  [i].key, [i].value);
+
+return NGX_HTTP_V2_FRAME_ERROR;
+}
+
+len += 1 + NGX_HTTP_V2_INT_OCTETS + header[i].key.len
+ + NGX_HTTP_V2_INT_OCTETS + header[i].value.len;
+
+if (header[i].key.len > tmp_len) {
+tmp_len = header[i].key.len;
+}
+
+if (header[i].value.len > tmp_len) {
+tmp_len = header[i].value.len;
+}
+}
+
+if (len == 0) {
+return NULL;
+}
+
+tmp = ngx_palloc(r->pool, tmp_len);
+pos = ngx_pnalloc(r->pool, len);
+
+if (pos == NULL || tmp == NULL) {
+return NGX_HTTP_V2_FRAME_ERROR;
+}
+
+start = pos;
+
+part = >headers_out.trailers.part;
+header = part->elts;
+
+for (i = 0; /* void */; i++) {
+
+if (i >= part->nelts) {
+if (part->next == NULL) {
+break;
+}
+
+part = part->next;
+header = part->elts;
+i = 0;
+}
+
+if (header[i].hash == 0) {
+continue;
+}
+
+#if (NGX_DEBUG)
+if (r->connection->log->log_level & NGX_LOG_DEBUG_HTTP) {
+ngx_strlow(tmp, header[i].key.data, header[i].key.len);
+
+ngx_log_debug3(NGX_LOG_DEBUG_HTTP, r->connection->log, 0,
+   "http2 output trailer: \"%*s: %V\"",
+   header[i].key.len, tmp, [i].value);
+}
+#endif
+
+*pos++ = 0;
+
+pos = ngx_http_v2_write_name(pos, header[i].key.data,
+ header[i].key.len, tmp);
+
+pos = ngx_http_v2_write_value(pos, header[i].value.data,
+  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;
+}
+
+
 static u_char *
 ngx_http_v2_string_encode(u_char *dst, u_char *src, size_t len, u_char *tmp,
 ngx_uint_t lower)
@@

Re: [PATCH 2 of 3] HTTP/2: added support for trailers in HTTP responses

Re: [PATCH 2 of 3] HTTP/2: added support for trailers in HTTP responses

Re: [PATCH 2 of 3] HTTP/2: added support for trailers in HTTP responses

Re: [PATCH 2 of 3] HTTP/2: added support for trailers in HTTP responses

Re: [PATCH 2 of 3] HTTP/2: added support for trailers in HTTP responses

Re: [PATCH 2 of 3] HTTP/2: added support for trailers in HTTP responses

Re: [PATCH 2 of 3] HTTP/2: added support for trailers in HTTP responses

Re: [PATCH 2 of 3] HTTP/2: added support for trailers in HTTP responses

[PATCH 2 of 3] HTTP/2: added support for trailers in HTTP responses

Re: [PATCH 2 of 3] HTTP/2: added support for trailers in HTTP responses

Re: [PATCH 2 of 3] HTTP/2: added support for trailers in HTTP responses

Re: [PATCH 2 of 3] HTTP/2: added support for trailers in HTTP responses

[PATCH 2 of 3] HTTP/2: added support for trailers in HTTP responses

[PATCH 2 of 3] HTTP/2: added support for trailers in HTTP responses

14 matches

Site Navigation

Mail list logo

Footer information