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

[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 <piotrsikora at google.com>
> > > > # 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 <piotrsikora at google.com>
> > > > 
> > > [..]
> > > > +
> > > > +        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\"",
> > > > +                          &header[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\"",
> > > > +                          &header[i].key, &header[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\"",
                           &header[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\"",
                           &header[i].key, &header[i].value);
             return NULL;

-- 
Maxim Dounin
http://nginx.org/