On 21 Nov 2011, at 8:04 PM, Stefan Fritsch wrote:
Looks reasonable. Some comments:
The error log handler log_remote_address for %a needs to fall back to
c->remote_ip if r is not specified. Otherwise one would need different
logformats for per-conn and per-request log messages. Also, I would
prefer %{r}a and %{c}a to force logging of r->remote_ip and c-
remote_ip. Then we don't need a new format letter and it would be
more consistent with the %L and %{c}L errorlog format.
What would we do with the error_log customisation in log.c? Currently
the letter 'd' is used there, while "%{c}a" is used in mod_log_config.
Ideally log.c should support the same %{c} option, but right now it
doesn't. Thoughts?
We may also want a CONN_REMOTE_ADDR or PHYS_REMOTE_ADDR variable in
ap_expr to still allow access to c->remote_addr.
I've added this in r1204990.
Do we need special handling of the REMOTE_HOST script variable?
Probably it does not make sense because we can't reliably do DNS
lookups for addresses received via X-Forwarded-For.
Right now mod_remoteip does sanity checking on the IP address
presented in the header, if you provide a bogus address it will be
ignored. I think this should probably be the expectation of a module
that fiddles with r->remote_ip rather than trying to sanity check it
across the server.
I think there may be some confusion of addresses if mod_remoteip is
used for CONNECT requests. But I am OK with ignoring that
complication. It should always be possible to use the connection log
ids to correlate the different messages.
IMHO, commit to trunk and we can fix the remaining issues there.
Done in r1204968.
For symmetry, should we do the same for local_ip / local_addr? One of
the things this allows us to do is provide a mechanism for a load
balancer to reveal the frontend port being used, which helps
mod_proxy_ajp for example to report the right frontend port to the
application behind it (something that's always been painful in
javaland). The patch should be significantly simpler.
Regards,
Graham
--
