Re: [PATCH] [mk3] deny_info 3xx code support

On 20/11/10 07:03, Alex Rousskov wrote:
> On 11/19/2010 07:19 AM, Amos Jeffries wrote:
>> Small improvement on the earlier patch.

I should say the earlier patch is still needed for 3.1 to permit safe
CONNECT redirects. This improved version is for 3.2+.

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

mk2 patch only applies to the redirection form of deny_info. If the code
is not 3xx the browser will not fetch the desired content to display the
page.

I'm not going to implement a background fetch which is required to use
any other status there. I'm not even certain we have a clean way to do
so from random points in the code yet.

mk3 patch (attached) adds 4xx and 5xx codes to the ERR_* named template
form of deny_info. 3xx is dodgy on these since we can't provide a
Location: header. Though I'm open to allowing it if we need to cater for
someones actual use-case.

I don't think there is a case for allowing 1xx and 2xx is there?

>
>> + 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.

Done. mk3 patch drops the else condition and initializes to 302 instead.

>
> Thank you,
>
> Alex.
> P.S. Please consider posting diffs with context depth set to 20. They
> are often easier to review.

Thanks for the reminder. I've been meaning to ask how you set bzr to
produce those?

Amos

-- 
Please be using
   Current Stable Squid 2.7.STABLE9 or 3.1.9
   Beta testers wanted for 3.2.0.3