[PATCH/v2] SPDY: Allow returning the full status line

[Valentin V. Bartenev]

On Friday 31 May 2013 08:52:41 Jim Radford wrote:
> On Fri, May 31, 2013 at 04:21:45AM +0400, Valentin V. Bartenev wrote:
> > On Friday 31 May 2013 02:24:50 Jim Radford wrote:
> > > This is a replacement to my previous patch which actaully includes the
> > > buffer length handling.
> > > 
> > > # HG changeset patch
> > > # User Jim Radford <radford at galvanix.com>
> > > # Date 1369952377 25200
> > > # Node ID 52d7b6082129c90275579fa3667cce3f537cbd09
> > > # Parent  00dbfac67e48a8fe20802287b6fca50950178b8b
> > > SPDY: Allow returning the full status line
> > 
> > [...]
> > 
> > Could you clarify a bit the purpose of this change?
> 
> When using nginx to proxy to http, the response status (code and text)
> are passed though unchanged if the request comes in via HTTP or HTTP
> over SSL; the text however is currently stripped when the request is
> made via SPDY.  For us this meant that enabling SPDY broke our
> application which expected to receive our custom status text.
> 
> While we could easily work around this, we think that it better for
> nginx to be request transport agnostic as much as possible.
> Applications that prefer a curt status may still provide one and it
> will be passed through unmolested.
> 

Ok, fair enough. See the review below:

> # HG changeset patch
> # User Jim Radford <radford at galvanix.com>
> # Date 1369952377 25200
> # Node ID 52d7b6082129c90275579fa3667cce3f537cbd09
> # Parent  00dbfac67e48a8fe20802287b6fca50950178b8b
> SPDY: Allow returning the full status line

You should not use capitalization after a colon, and also please
end the summary line with a period.

A small description of the problem would be nice to see here.

> diff -r 00dbfac67e48 -r 52d7b6082129 src/http/ngx_http_spdy_filter_module.c
> --- a/src/http/ngx_http_spdy_filter_module.c    Thu May 30 18:23:05 2013 +0400
> +++ b/src/http/ngx_http_spdy_filter_module.c    Thu May 30 15:19:37 2013 -0700
> @@ -162,7 +162,9 @@
>            + ngx_http_spdy_nv_nsize("version")
>            + ngx_http_spdy_nv_vsize("HTTP/1.1")
>            + ngx_http_spdy_nv_nsize("status")
> -          + ngx_http_spdy_nv_vsize("418");
> +          + (r->headers_out.status_line.len
> +             ? NGX_SPDY_NV_VLEN_SIZE + r->headers_out.status_line.len
> +             : ngx_http_spdy_nv_vsize("418"));
>  
>      clcf = ngx_http_get_module_loc_conf(r, ngx_http_core_module);
>  
> @@ -304,8 +306,14 @@
>      last = ngx_http_spdy_nv_write_val(last, "HTTP/1.1");
>  
>      last = ngx_http_spdy_nv_write_name(last, "status");
> -    last = ngx_spdy_frame_write_uint16(last, 3);
> -    last = ngx_sprintf(last, "%03ui", r->headers_out.status);

It turns too tightly. Please add an empty line here for better readability.

> +    if (r->headers_out.status_line.len) {
> +        last = ngx_http_spdy_nv_write_vlen(last, r->headers_out.status_line.len);
> +        last = ngx_cpymem(last, r->headers_out.status_line.data,
> +                          r->headers_out.status_line.len);
> +    } else {
> +        last = ngx_spdy_frame_write_uint16(last, 3);
> +        last = ngx_sprintf(last, "%03ui", r->headers_out.status);
> +    }
>  
>      count = 2;
>  
> 
> 

Also, please note changes in http://hg.nginx.org/nginx/rev/5776804fff04


  wbr, Valentin V. Bartenev