Re: [PATCH] Error page format codes upgrade

On 11/06/2013 4:53 a.m., Alex Rousskov wrote:
> On 06/02/2013 05:08 AM, Amos Jeffries wrote:
>> On 7/04/2013 5:48 p.m., Amos Jeffries wrote:
>>> What this does is convert ErrorState object to using the generic
>>> libformat.la parser and macro expansions instead of its own rather
>>> limited custom ones for error pages and deny_info URL creation.
> Thank you for addressing many of my concerns.
>
>
>> + 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 also wonder if it would be a good idea to make that new ale parameter
> mandatory so that folks are reminded to supply ALE when their context
> has one.

Maybe. Im not sure we want to just pass it in without checking its
up-to-date and that is a bit difficult with some of the current code.

>
>> + // 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;
>> + }
> Please use ConnStateData::getCurrentContext() since we have that
> accessor and this access is outside ConnStateData.
>
> 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.

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

> Please also add a commit (and release notes?) comment about the old
> error pages changing if they contained unsupported macros, as we
> discussed earlier when looking at the

There is no longer any change. see below..

>
>> - case '%':
>> - p = "%";
>> - break;
>> -
>> default:
>> - mb.Printf("%%%c", token);
>> - do_quote = 0;
>> break;
>> }
> change.
>
>
>
>> if (type == LFT_NONE) {
>> - fatalf("Can't parse configuration token: '%s'\n", def);
>> + debugs(46, 3, "Can't parse configuration token: '" << StringArea(def,min(strlen(def), static_cast<size_t>(10))) << "'");
>> + // NP: unknown token found. Assume it should have been %% instead of %.
>> + if (def[0] == '%') {
>> + type = LFT_PERCENT;
>> + ++cur;
>> + }
> Why do we want to start hiding these errors now? Is hiding them related
> to this patch scope? Should we at least warn the admin that their page
> may be buggy (but, as you know, I personally prefer a fatal exit on
> configuration errors).

This is the bit of code which replaces the above "mb.Printf("%%%c",
token);" removal.

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

Amos
Received on Tue Jun 11 2013 - 02:53:15 MDT