On 12/06/2013 9:46 a.m., Alex Rousskov wrote:
On 06/10/2013 08:44 PM, Amos Jeffries wrote:
+ ErrorState(err_type type, Http::StatusCode, HttpRequest *
request, AccessLogEntry *ale = NULL);
I do not see ErrorState constructor being called with an ALE object (the
last parameter) anywhere. FwdState, for example, stores ALE but does not
pass it to ErrorState. Should not it? Any other contexts where we would
be better off passing readily available ALE instead of relying on three
levels of volatile and obscure pointers to lead us to it?
The problem is whether ALE is filled out at the time it is passed to
ErrorState. A lot of the code in server-side is depending on the ALE
being filled out by logging, long after the error has been generated. I
would rather use this slightly messy code that fails over to generating
an ALE filled out with all the required details than suddenly make all
the errors output by Squid have missing information.
I appreciate the problem. Perhaps add a "Make sure ALE is completely
filled out if you pass it here" comment to the constructor declaration,
to explain why the parameter is currently unused? Or remove the
parameter completely (because nobody uses it).
We may need a function that fills out or _updates_ ALE from currently
available details to solve this problem, but that work is probably
outside your project scope.
Was just thinking that while I sent this last patch off.
+ // if request exists, we might have access to the transaction
ALE object
+ if (ale_ == NULL && request != NULL &&
request->clientConnectionManager.valid() &&
+ request->clientConnectionManager->currentobject != NULL) {
+ ClientSocketContext::Pointer c =
request->clientConnectionManager->currentobject;
+ ale_ = c->http->al;
+ }
During pipelining, are we certain that currentobject will point to the
request we are creating the error page for??
No. But that is no more certain when using the accessor in the current
pipelining design.
My question was about incorrect ClientSocketContext object being used
for error page generation, regardless of the accessor that led to that
object. So, after this change, if pipelining is enabled, then generated
error pages may contain completely bogus information, right? Was that
true before the change? Or is this a problem with the current error page
generation code as well?
There are a few comments in places using getCurrentContext() to the
effect that it sometimes supplies the wrong context in pipelines.
I'm not quite sure exactly, but IFF it is possible for async operations
about request #1 in a pipeline to complete after a response to that
request has already been sent, it pops the context off teh queue and may
result in a second response (ie error page) being generated with details
pulled from context of request #2 in the pipeline.
If it is an old problem, then you are not responsible for fixing it. If
your code adds it, then you are responsible for avoiding it. That is why
I asked...
The code comments are *very* old as in, some existed before the code was
loaded into CVS. So it may or may not be a real edge case any more.
I am inclined to bet on it not being a problem, some of the other code
is already making that bet and not having any issues yet traced to the
pipeline.
/* - IP stuff */
- str.Printf("ClientIP: %s\r\n", src_addr.NtoA(ntoabuf,MAX_IPSTRLEN));
+// char ntoabuf[MAX_IPSTRLEN];
+//XXX str.Printf("ClientIP: %s\r\n", src_addr.NtoA(ntoabuf,MAX_IPSTRLEN));
Is this change intentional?
Yes with the src_addr removal we have no way to identify the client IP
in that code anymore.
It is not clear what that XXX implies. Add a comment?
Do you think we need to display the client IP there anymore? I'm not
sure we do.
If we use fatal() here anyone loading a template with future defined
codes or typos will crash Squid every time that error is supposed to be
displayed.
Can Squid validate error templates at startup and quit if templates are
invalid? That would avoid runtime crashes without hiding potentially
serious problems from the admin.
That would be a part of the internal cache feature which is omitted from
this patch.
I don't think it is a serious problem. Just some technical token
lettering is displayed intead of potentially useful info. All templates
should be tested before use anyway to check that the desired output is
displayed at the desired time.
The assumption that they meanst %% is not great, but we do
have to choose between hiding the unknown token and displaying it
un-expanded and since libformat tokens are variable in length we don't
really have the hide option without potentially screwing something else
up as well. This way at least it will show up during trivial testing by
the admin.
What trivial test would expose these problems if Squid does not complain
about them? I cannot think of any (not to mention that most admins would
not know they should run those tests, even if they are trivial).
The trivial test of pretending to be a user and loading a forbidden URL
or whatever the use-case they are altering the templates for is.
Not that I'm disagreeing with you on the idea. Just that it is a design
decision that toggles between small internal conversion patch (current)
and major new feature addition (TODO).
PS. The one thing I tell translators if they are trying to figure out
how to handle maros in the grammar is that they are usually representing
some noun, or a full sentence of text. So the context where they appear
should be written in a way that is legible even if the token is
completely unknown to the reader. Macros added by us into the templates
have this principle, macros used by others self-written templates IMO we
can expect to at least have been tried loading to see what the display
looks like before going public.
Amos
