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

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

>
> 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 Fri Jul 06 2012 - 16:14:31 MDT