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

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