Hello!
On Fri, Dec 05, 2014 at 07:38:18PM +0000, Steven Hartland wrote:
> First off thanks for reviewing, comments / questions inline below
>
> On 05/12/2014 16:00, Maxim Dounin wrote:
> >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.
> Looks like I lost the full comment when trying to make hg do a squash, its
> not as nice to work with as git,
Rebase and MQ extensions make "rewriting history" tasks easier in
Mercurial.
> which I guess is why Google are migrating
> their golang repo to github ;-)
Yes, we are also wonder how golang folks are going to maintain
their "new VCS every year" trend. It's not that easy unless they
will try some of them again, as they already used svn, p4, hg and
now git. ;)
> I'll restore this in the next patch but for now here's some details.
>
> The driver for the changes is an additional module we've been developing
> which based on this core enhancement can use predictably split range
> requests to satisfy other requests.
[...]
> More information on this can be seen on our blog here:
> http://blog.multiplay.co.uk/2014/04/lancache-dynamically-caching-game-installs-at-lans-using-nginx/
The question is mostly about "native" use, if possible. I
understand that your main goal is the module your are working on,
but I suspect that this change may be used with some minimal
configuration changes to improve range caching configs as in your
blog post.
>
> >In general I think that this change is interesting, but there are
> >some rough edges. See below for various comments.
> >
> >> 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.
> Sorry I'm not sure I follow you correctly, do you mean set the old
> content_length local from ctx->content_lenght keeping the diff minimal?
Yes, I think that preserving content_length local variable should
be a better option.
[...]
> >>@@ -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.
> I removed as it was inconsistent with the function above, could you confirm
> the details of this style requirement please so I know for the future?
There are no strict rules on how to put empty lines, but in
general preprocessor conditionals are separated by empty lines if
conditional content is complex enough (e.g., contains an empty
line). In the function about the content is trivial and so no
empty lines there.
[...]
> >> 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.
>
> I thought it would be wasteful to copy the function instead of making the
> small amendments to make it more generic and reuse
> ngx_http_upstream_copy_header_line instead of a hand rolled version of it.
>
> Its easy enough to make it independent if you think that's the better
> option?
I think that separate copy function would be better option here.
The code is going to be different enough to satisfy this.
--
Maxim Dounin
http://nginx.org/
_______________________________________________
nginx-devel mailing list
nginx-devel@nginx.org
http://mailman.nginx.org/mailman/listinfo/nginx-devel
