Re: [PATCH] log virgin HTTP request headers

[Alex Rousskov]

On 01/24/2010 08:20 PM, Amos Jeffries wrote:
> Tsantilas Christos wrote:
>> Hi all,
>>   First of all sorry for late answer.
>> I am sending a second version of the patch.
>> The new patch uses the format code "http::>ha" to log the adapted
>> headers. The already existing format code http::>h logs the original
>> request headers.
>> This patch still does not handle the use of the request_header_access.
>> Please read below.
>>
>>
>> Amos Jeffries wrote:
>>> Tsantilas Christos wrote:
>>>> Hi all,
>>>>  This patch adds a new format code which allow the user to log HTTP
>>>> request header or header fields before they are adapted.
>>>> The existing "http::>h" format code logs HTTP request headers after
>>>> adaptation.
>>>> The new format code is the "http::>hv".
>>>>
>>>> This is a Measurement Factory project.
>>>>
>>>> Regards,
>>>>     Christos
>>>>
>>>
>>> Um, I would think this makes more sense done the other way around.
>>>
>>> With the default >h displaying the virgin headers received from the
>>> client and some other code ( >ha ?) for the adapted headers. Be it 
>>
>> OK Amos this patch does it.
>>
>>> adaptation or headers_access doing the alteration.
>>
>> The request_header_access does not modify the request send by the
>> client but has to do with the http request will be send to the server
>> by squid.
>> Also currently does not have any effect to request headers logging.
>>
>> The request_header_access implemented inside  http.cc file. If we want
>> to consider it as request adaptation mechanism maybe we should move it
>> to client_side* code.
>>
>> Regards,
>>     Christos
>>
>>>
>>> Likewise on replies.
>>>
>>> Amos
> 
> Thats fine.   +1 from me.

* Should we add "virgin" to >h definition in src/cf.data.pre?


* I would polish src/cf.data.pre text a little:

  [http::]>ha   The adapted HTTP request headers.
                Optional header name argument as for >h


* In the first hunk of src/client_side.cc, I would use a single "#if
USE_ADAPTATION" block to set both aLogEntry->headers.adapted_request and
aLogEntry->headers.request instead of testing USE_ADAPTATION twice. If
this is not possible for some reason, ignore me.


* Please add comments to document the new adapted_request fields in the
header files. Something like

...  //< HTTP request headers after adaptation and redirection

may work well.


Please commit when you are comfortable with the code.

Thank you,

Alex.