Re: [PATCH] Add request_header_add option and [request|reply]_header_* manglers fixes

On 7/07/2012 4:13 a.m., Tsantilas Christos wrote:
> On 07/06/2012 05:55 PM, Alex Rousskov wrote:
>> On 07/06/2012 08:41 AM, Tsantilas Christos wrote:
>>> On 07/01/2012 02:25 PM, Amos Jeffries wrote:
>>>> Thank you. In most places the problem is that AccessLogEntry is the
>>>> closest thing we have to a XactionData object for the whole HTTP
>>>> transaction. (I still have patches waiting around to polish up and
>>>> submit for audit creating that master transaction object).
>>>> AccessLogEntry is available from the ConnStateData / conn() almost
>>>> everywhere in the code. So it *is* actually available everywhere through
>>>> the transaction, just not filled out. We have a lot of code cleaning to
>>>> make it consistent, (but outside this patches scope).
>>> The AccessLogEntry is not available from ConnStateData, it is only
>>> available from ClientHttpRequest, and this is correct because an
>>> accessLogEntry is Http request related, not connection related.
>>>
>>>> For now it is available via "request->conn()->ale". Or a new temporary
>>>> one created in local scope for the function doing display output. It may
>>>> need filling out with pointers to things required (is SSL content, HTTP
>>>> request, client connection), the code before logging is very
>>>> inconsistent/non-existent.
>>> The only way to access AccessLogEntry is using the following path:
>>> request->conn()->getCurrentContext()->http->al
>>>
>>> I am not feeling comfortable using the request->conn() although it looks
>>> that works well in the current squid code. Moreover the
>>> request->conn()->getCurrentContext()->http path, I think does not
>>> guarantee that the ClientHttpRequest is the object we need.

yes. Collapsed forwarding was added in 2.6. I am constantly also getting
the impression that squid-3 contains many of the design changes leading
up to it.

>>>
>>> Sometimes I am felling the initial design was that a (server side)
>>> HttpRequest corresponds to more than one connections (more than one
>>> connections accessing the same object) and of course to more than one
>>> ClientHttpRequest objects.
>>>
>>> Also I believe that the request->conn()->getCurrentContext() may not
>>> return the ClientSocketContext corresponding to our HttpRequest.
>>> We can go from ClientSocketContext to HttpRequest
>>> (ClientSocketContext->http->request) but no the reverse.
>>>
>>> Any opinions here?
>> I agree that getting AccessLogEntry via getCurrentContext() is wrong,
>> even if it works in most cases. A pipelining connection may have many
>> requests associated with it and, at the time we log the transaction, our
>> request may no longer be "current".

Yes. Sorry I overlooked that indirection. Squid-2 has a few bugs in
record about the context miss-matching the server request, I am also in
favour of not adding more. Until the pipeline management is polished up
and bug fixed to make some guarantees, getCurrentContext() not safe.

That was just a suggestion to improve the use of ALE as a XactionData
style object. You are 100% free to create a new AccessLogEntry local
variable and fill it out with the details relevant to this code.

>>
>> Can we add a link from HttpRequest to the corresponding ClientHttpRequest?
> Probably yes. We are creating the HttpRequest inside
> clientProcessRequest function (client_side.cc file). We can link it here.
> We may not be able to link it in some cases where we are creating fake
> HttpRequest objects (httpsEstablish function)

If you wish to continue with the idea of ALE as the current best
XactionData object (I'm hoping you will but see above) a new one should
be created whenever Squid generates a unique request/transaction for
doing things. So even those can get logged properly.

>
>> Or is it better to add a link from HttpRequest to AccessLogEntry?
> We need to convert AccessLogEntry to cbdata class or use RefCounts to
> use link to AccessLogEntry.
>
>> Seriously malformed and special requests may not have HttpRequest
>> assciated with them, so I understand why ClientHttpRequest stores
>> AccessLogEntry and not HttpRequest, but we need to make a way for the
>> rest of the code to find AccessLogEntry without relying on
>> getCurrentContext(). And HttpRequest is available nearly everywhere.
> OK.
>
>
>>
>> HTH,
>>
>> Alex.
>>
Received on Sat Jul 07 2012 - 01:25:08 MDT