Re: [PATCH] deny_info 3xx code support

On 11/19/2010 07:19 AM, Amos Jeffries wrote:
> Small improvement on the earlier patch.
>
> This adds:
>
> * automatic selection of 307 response status for non-GET/HEAD HTTP/1.1
> requests.
> * support for custom 3xx codes (only 300..399 allowed) to be configured
> on the deny_info URL.
> For example deny_info 303:http://example.com/ POST
>
> While retaining the 302 status for all HTTP/1.0 clients in case they do
> not support the HTTP/1.1 status.
>
> I've omitted automatic selection of 303 status, since it is slightly
> unclear whether it applies to *all* PUT/POST. Easily added at a later
> stage anyways.
>
>
> Testing in production shows that Firefox supports 307 redirection. The
> other popular browsers treat it like 302 still.

Why restrict custom codes to the 3xx range? I have heard requests for
other codes with deny_info. I realize we are adding Location header
unconditionally, but it should not hurt if the status code is not 3xx.

> + http_status status;
> + // Use configured 3xx reply status if set.

Please initialize status to some default value like
HTTP_MOVED_TEMPORARILY. The current code is not buggy, but it is safer
to keep the variable initialized, IMO.

Thank you,

Alex.
P.S. Please consider posting diffs with context depth set to 20. They
are often easier to review.
Received on Fri Nov 19 2010 - 18:03:41 MST