Re: [PATCH] Error page format codes upgrade

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?

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.

> + // 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??

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

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

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

Thank you,

Alex.
Received on Mon Jun 10 2013 - 16:54:08 MDT