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

[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 <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;

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

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