Hello!

On Thu, Dec 04, 2014 at 09:07:57PM +0000, Steven Hartland wrote:

> # HG changeset patch
> # User Steven Hartland <steven.hartland@multiplay.co.uk>
> # Date 1417727204 0
> # Thu Dec 04 21:06:44 2014 +0000
> # Node ID 05d3973ece9af030d0312932938fc3d1f2f139dd
> # Parent 1573fc7875fa09ee55763ce7ddc4e98d61e1deaf
> Allow Partial Content responses to satisfy Range requests

I would suggest something like:

Range filter: support for 206 Partial Content responses.

Some explanation on how to use these changes may be also helpful,
as out of the box nginx won't be able to do anything good with it.

In general I think that this change is interesting, but there are
some rough edges. See below for various comments.

>
> diff -r 1573fc7875fa -r 05d3973ece9a src/http/modules/ngx_http_range_filter_module.c
> --- a/src/http/modules/ngx_http_range_filter_module.c Wed Nov 26 18:35:37 2014 -0800
> +++ b/src/http/modules/ngx_http_range_filter_module.c Thu Dec 04 21:06:44 2014 +0000
> @@ -54,6 +54,7 @@
>
> typedef struct {
> off_t offset;
> + off_t content_length;
> ngx_str_t boundary_header;
> ngx_array_t ranges;
> } ngx_http_range_filter_ctx_t;
> @@ -65,7 +66,8 @@
> ngx_http_range_filter_ctx_t *ctx);
> static ngx_int_t ngx_http_range_multipart_header(ngx_http_request_t *r,
> ngx_http_range_filter_ctx_t *ctx);
> -static ngx_int_t ngx_http_range_not_satisfiable(ngx_http_request_t *r);
> +static ngx_int_t ngx_http_range_not_satisfiable(ngx_http_request_t *r,
> + ngx_http_range_filter_ctx_t *ctx);
> static ngx_int_t ngx_http_range_test_overlapped(ngx_http_request_t *r,
> ngx_http_range_filter_ctx_t *ctx, ngx_chain_t *in);
> static ngx_int_t ngx_http_range_singlepart_body(ngx_http_request_t *r,
> @@ -76,6 +78,9 @@
> static ngx_int_t ngx_http_range_header_filter_init(ngx_conf_t *cf);
> static ngx_int_t ngx_http_range_body_filter_init(ngx_conf_t *cf);
>
> +static ngx_int_t ngx_http_content_range_parse(ngx_http_request_t *r,
> + ngx_http_range_filter_ctx_t *ctx);
> +

Functions in the range filter use "ngx_http_range_*" prefix, so
there should be a better name.

Also, both the declaration and function body should be placed
eslewhere, see below.

>
> static ngx_http_module_t ngx_http_range_header_filter_module_ctx = {
> NULL, /* preconfiguration */
> @@ -153,8 +158,8 @@
> ngx_http_range_filter_ctx_t *ctx;
>
> if (r->http_version < NGX_HTTP_VERSION_10
> - || r->headers_out.status != NGX_HTTP_OK
> - || r != r->main
> + || (r->headers_out.status != NGX_HTTP_OK
> + && r->headers_out.status != NGX_HTTP_PARTIAL_CONTENT)
> || r->headers_out.content_length_n == -1
> || !r->allow_ranges)
> {

Style, should be:

@@ -153,8 +158,8 @@ ngx_http_range_header_filter(ngx_http_re
ngx_http_range_filter_ctx_t *ctx;

if (r->http_version < NGX_HTTP_VERSION_10
- || r->headers_out.status != NGX_HTTP_OK
- || r != r->main
+ || (r->headers_out.status != NGX_HTTP_OK
+ && r->headers_out.status != NGX_HTTP_PARTIAL_CONTENT)
|| r->headers_out.content_length_n == -1
|| !r->allow_ranges)
{

> @@ -230,26 +235,31 @@
>
> ranges = r->single_range ? 1 : clcf->max_ranges;
>
> - switch (ngx_http_range_parse(r, ctx, ranges)) {
> + switch (ngx_http_content_range_parse(r, ctx)) {
> + case NGX_OK:

If there are only two possible results of
ngx_http_content_range_parse(), there is no real reason to use
switch here. Just an if with "goto next_filter" should do.

> + switch (ngx_http_range_parse(r, ctx, ranges)) {
> + case NGX_OK:
> + ngx_http_set_ctx(r, ctx, ngx_http_range_body_filter_module);
>
> - case NGX_OK:
> - ngx_http_set_ctx(r, ctx, ngx_http_range_body_filter_module);
> + r->headers_out.status = NGX_HTTP_PARTIAL_CONTENT;
> + r->headers_out.status_line.len = 0;
>
> - r->headers_out.status = NGX_HTTP_PARTIAL_CONTENT;
> - r->headers_out.status_line.len = 0;
> + if (ctx->ranges.nelts == 1) {
> + return ngx_http_range_singlepart_header(r, ctx);
> + }
>
> - if (ctx->ranges.nelts == 1) {
> - return ngx_http_range_singlepart_header(r, ctx);
> + return ngx_http_range_multipart_header(r, ctx);
> +
> + case NGX_HTTP_RANGE_NOT_SATISFIABLE:
> + return ngx_http_range_not_satisfiable(r, ctx);
> +
> + case NGX_ERROR:
> + return NGX_ERROR;
> +
> + default: /* NGX_DECLINED */
> + break;
> }
> -
> - return ngx_http_range_multipart_header(r, ctx);
> -
> - case NGX_HTTP_RANGE_NOT_SATISFIABLE:
> - return ngx_http_range_not_satisfiable(r);
> -
> - case NGX_ERROR:
> - return NGX_ERROR;
> -
> + break;
> default: /* NGX_DECLINED */
> break;
> }
> @@ -274,13 +284,12 @@

Please put

[diff]
showfunc=1

into your ~/.hgrc, it'll make patches more readable. Thanks.

> ngx_uint_t ranges)
> {
> u_char *p;
> - off_t start, end, size, content_length;
> + off_t start, end, size;
> ngx_uint_t suffix;
> ngx_http_range_t *range;
>
> p = r->headers_in.range->value.data + 6;
> size = 0;
> - content_length = r->headers_out.content_length_n;

Just using ctx->content_length here may be a better option.

>
> for ( ;; ) {
> start = 0;
> @@ -298,6 +307,10 @@
> start = start * 10 + *p++ - '0';
> }
>
> + if (start < ctx->offset) {
> + return NGX_HTTP_RANGE_NOT_SATISFIABLE;
> + }
> +

That's wrong return code.

Additionally, this probably needs some logging, as this situation
indicates a problem elsewhere: a 206 Partial Content response was
used to answer a request for a range not covered by the response.

> while (*p == ' ') { p++; }
>
> if (*p++ != '-') {
> @@ -307,7 +320,7 @@
> while (*p == ' ') { p++; }
>
> if (*p == ',' || *p == '\0') {
> - end = content_length;
> + end = ctx->content_length;
> goto found;
> }
>
> @@ -331,12 +344,12 @@
> }
>
> if (suffix) {
> - start = content_length - end;
> - end = content_length - 1;
> + start = ctx->content_length - end;
> + end = ctx->content_length - 1;
> }
>
> - if (end >= content_length) {
> - end = content_length;
> + if (end >= ctx->content_length) {
> + end = ctx->content_length;
>
> } else {
> end++;
> @@ -369,7 +382,7 @@
> return NGX_HTTP_RANGE_NOT_SATISFIABLE;
> }
>
> - if (size > content_length) {
> + if (size > ctx->content_length) {
> return NGX_DECLINED;
> }
>
> @@ -384,16 +397,18 @@
> ngx_table_elt_t *content_range;
> ngx_http_range_t *range;
>
> - content_range = ngx_list_push(&r->headers_out.headers);
> - if (content_range == NULL) {
> - return NGX_ERROR;
> + if (r->headers_out.content_range == NULL) {
> + content_range = ngx_list_push(&r->headers_out.headers);
> + if (content_range == NULL) {
> + return NGX_ERROR;
> + }
> + r->headers_out.content_range = content_range;
> + content_range->hash = 1;
> + ngx_str_set(&content_range->key, "Content-Range");
> + } else {
> + content_range = r->headers_out.content_range;
> }
>
> - r->headers_out.content_range = content_range;
> -
> - content_range->hash = 1;
> - ngx_str_set(&content_range->key, "Content-Range");
> -
> content_range->value.data = ngx_pnalloc(r->pool,
> sizeof("bytes -/") - 1 + 3 * NGX_OFF_T_LEN);
> if (content_range->value.data == NULL) {

It might be easier to just remove an old header if it's here, like
this:

@@ -389,6 +389,10 @@ ngx_http_range_singlepart_header(ngx_htt
return NGX_ERROR;
}

+ if (r->headers_out.content_range) {
+ r->headers_out.content_range->hash = 0;
+ }
+
r->headers_out.content_range = content_range;

content_range->hash = 1;

Additionally, it looks like appropriate modifications to
ngx_http_range_multipart_header() are missing. This will result
in unneeded/confusing Content-Range header in multipart/ranges
responses produced from 206 responses.

> @@ -407,7 +422,7 @@
> content_range->value.len = ngx_sprintf(content_range->value.data,
> "bytes %O-%O/%O",
> range->start, range->end - 1,
> - r->headers_out.content_length_n)
> + ctx->content_length)
> - content_range->value.data;
>
> r->headers_out.content_length_n = range->end - range->start;
> @@ -546,22 +561,25 @@
>
>
> static ngx_int_t
> -ngx_http_range_not_satisfiable(ngx_http_request_t *r)
> +ngx_http_range_not_satisfiable(ngx_http_request_t *r,
> + ngx_http_range_filter_ctx_t *ctx)
> {
> ngx_table_elt_t *content_range;
>
> r->headers_out.status = NGX_HTTP_RANGE_NOT_SATISFIABLE;
>
> - content_range = ngx_list_push(&r->headers_out.headers);
> - if (content_range == NULL) {
> - return NGX_ERROR;
> + if (r->headers_out.content_range == NULL) {
> + content_range = ngx_list_push(&r->headers_out.headers);
> + if (content_range == NULL) {
> + return NGX_ERROR;
> + }
> + r->headers_out.content_range = content_range;
> + content_range->hash = 1;
> + ngx_str_set(&content_range->key, "Content-Range");
> + } else {
> + content_range = r->headers_out.content_range;
> }
>
> - r->headers_out.content_range = content_range;
> -
> - content_range->hash = 1;
> - ngx_str_set(&content_range->key, "Content-Range");
> -
> content_range->value.data = ngx_pnalloc(r->pool,
> sizeof("bytes */") - 1 + NGX_OFF_T_LEN);
> if (content_range->value.data == NULL) {

Same here.


> @@ -570,7 +588,7 @@
>
> content_range->value.len = ngx_sprintf(content_range->value.data,
> "bytes */%O",
> - r->headers_out.content_length_n)
> + ctx->content_length)
> - content_range->value.data;
>
> ngx_http_clear_content_length(r);
> @@ -888,3 +906,76 @@
>
> return NGX_OK;
> }
> +
> +
> +static ngx_int_t
> +ngx_http_content_range_parse(ngx_http_request_t *r,
> + ngx_http_range_filter_ctx_t *ctx)
> +{

This should be placed elsewhere. Normally we functions in the
order of execution. That is, as this function is called from
ngx_http_range_header_filter(), it should be placed after
ngx_http_range_header_filter(). And as it's called just before
ngx_http_range_parse(), it should be placed somewhere above
ngx_http_range_parse().

> + u_char *p;
> + off_t start, end, len;

Style, should be:

off_t start, end, len;
u_char *p;

> +
> + ctx->offset = 0;
> + ctx->content_length = r->headers_out.content_length_n;
> +
> + if (r->headers_out.status != NGX_HTTP_PARTIAL_CONTENT) {
> + return NGX_OK;
> + }

Style, should be 4 spaces.

> +
> + if (r->headers_out.content_range == NULL
> + || r->headers_out.content_range->value.len == 0) {

Style, "{" should be on it's own line.

> + return NGX_HTTP_RANGE_NOT_SATISFIABLE;
> + }

NGX_HTTP_RANGE_NOT_SATISFIABLE is wrong and confusing here. It's
not checked/used by the calling code, but nevertheless shouldn't
be here.

The same applies to other uses of NGX_HTTP_RANGE_NOT_SATISFIABLE
in this function.

> +
> + if (r->headers_out.content_range->value.len < 7
> + || ngx_strncasecmp(r->headers_out.content_range->value.data,
> + (u_char *) "bytes ", 6) != 0) {
> + return NGX_DECLINED;
> + }

Style, "{" should be on it's own line.

> +
> + start = 0;
> + end = 0;
> + len = 0;
> +
> + p = r->headers_out.content_range->value.data + 6;
> +
> + while (*p == ' ') { p++; }
> +
> + if (*p < '0' || *p > '9') {
> + return NGX_HTTP_RANGE_NOT_SATISFIABLE;
> + }
> +
> + while (*p >= '0' && *p <= '9') {
> + start = start * 10 + *p++ - '0';
> + }
> +
> + if (*p++ != '-') {
> + return NGX_HTTP_RANGE_NOT_SATISFIABLE;
> + }
> +
> + while (*p >= '0' && *p <= '9') {
> + end = end * 10 + *p++ - '0';
> + }
> +
> + if (*p++ != '/') {
> + return NGX_HTTP_RANGE_NOT_SATISFIABLE;
> + }
> +
> + if (*p < '0' || *p > '9') {
> + return NGX_HTTP_RANGE_NOT_SATISFIABLE;
> + }
> +
> + while (*p >= '0' && *p <= '9') {
> + len = len * 10 + *p++ - '0';
> + }
> +
> + if (*p != '\0') {
> + return NGX_HTTP_RANGE_NOT_SATISFIABLE;
> + }
> +
> + ctx->offset = start;
> + ctx->content_length = len;
> +
> + return NGX_OK;
> +}
> +
> diff -r 1573fc7875fa -r 05d3973ece9a src/http/ngx_http_upstream.c
> --- a/src/http/ngx_http_upstream.c Wed Nov 26 18:35:37 2014 -0800
> +++ b/src/http/ngx_http_upstream.c Thu Dec 04 21:06:44 2014 +0000
> @@ -292,6 +292,11 @@
> ngx_http_upstream_copy_content_encoding, 0, 0 },
> #endif
>
> + { ngx_string("Content-Range"),
> + ngx_http_upstream_ignore_header_line, 0,
> + ngx_http_upstream_copy_allow_ranges,
> + offsetof(ngx_http_headers_out_t, content_range), 1 },
> +

Placing this right after Accept-Ranges may be better.

> { ngx_null_string, NULL, 0, NULL, 0, 0 }
> };
>
> @@ -4516,37 +4521,26 @@
> ngx_http_upstream_copy_allow_ranges(ngx_http_request_t *r,
> ngx_table_elt_t *h, ngx_uint_t offset)
> {
> - ngx_table_elt_t *ho;
> -
> if (r->upstream->conf->force_ranges) {
> return NGX_OK;
> }
> -
> #if (NGX_HTTP_CACHE)
> -

Style, empty lines should be here.

> if (r->cached) {
> r->allow_ranges = 1;
> - return NGX_OK;
> + if (offsetof(ngx_http_headers_out_t, accept_ranges) == offset) {
> + return NGX_OK;
> + }
> }

It may be better idea to introduce a separate copy function
instead.

>
> if (r->upstream->cacheable) {
> r->allow_ranges = 1;
> r->single_range = 1;
> - return NGX_OK;
> - }
> -
> + if (offsetof(ngx_http_headers_out_t, accept_ranges) == offset) {
> + return NGX_OK;
> + }
> + }
> #endif
> -
> - ho = ngx_list_push(&r->headers_out.headers);
> - if (ho == NULL) {
> - return NGX_ERROR;
> - }
> -
> - *ho = *h;
> -
> - r->headers_out.accept_ranges = ho;
> -
> - return NGX_OK;
> + return ngx_http_upstream_copy_header_line(r, h, offset);

I don't think that ngx_http_upstream_copy_header_line() worth it.

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

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

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