You are viewing a plain text version of this content. The canonical link for it is here.
Posted to cvs@httpd.apache.org by mi...@apache.org on 2009/10/03 16:54:00 UTC
svn commit: r821333 - in /httpd/httpd/trunk: CHANGES
docs/manual/mod/mod_cache.xml modules/cache/cache_util.c
Author: minfrin
Date: Sat Oct 3 14:54:00 2009
New Revision: 821333
URL: http://svn.apache.org/viewvc?rev=821333&view=rev
Log:
mod_cache: Fix uri_meets_conditions() so that CacheEnable will
match by scheme, or by a wildcarded hostname.
PR: 40169
Submitted by: Ryan Pendergast <rpender us.ibm.com>
Reviewed by: Graham Leggett
Modified:
httpd/httpd/trunk/CHANGES
httpd/httpd/trunk/docs/manual/mod/mod_cache.xml
httpd/httpd/trunk/modules/cache/cache_util.c
Modified: httpd/httpd/trunk/CHANGES
URL: http://svn.apache.org/viewvc/httpd/httpd/trunk/CHANGES?rev=821333&r1=821332&r2=821333&view=diff
==============================================================================
--- httpd/httpd/trunk/CHANGES [utf-8] (original)
+++ httpd/httpd/trunk/CHANGES [utf-8] Sat Oct 3 14:54:00 2009
@@ -10,6 +10,10 @@
mod_proxy_ftp: NULL pointer dereference on error paths.
[Stefan Fritsch <sf fritsch.de>, Joe Orton]
+ *) mod_cache: Fix uri_meets_conditions() so that CacheEnable will
+ match by scheme, or by a wildcarded hostname. PR 40169
+ [Ryan Pendergast <rpender us.ibm.com>, Graham Leggett]
+
*) suxec: Allow to log an error if exec fails by setting FD_CLOEXEC
on the log file instead of closing it. PR 10744. [Nicolas Rachinsky]
Modified: httpd/httpd/trunk/docs/manual/mod/mod_cache.xml
URL: http://svn.apache.org/viewvc/httpd/httpd/trunk/docs/manual/mod/mod_cache.xml?rev=821333&r1=821332&r2=821333&view=diff
==============================================================================
--- httpd/httpd/trunk/docs/manual/mod/mod_cache.xml (original)
+++ httpd/httpd/trunk/docs/manual/mod/mod_cache.xml Sat Oct 3 14:54:00 2009
@@ -262,6 +262,17 @@
CacheEnable disk http://www.apache.org/<br />
</example>
+ <p>A hostname starting with a <strong>"*"</strong> matches all hostnames with
+ that suffix. A hostname starting with <strong>"."</strong> matches all
+ hostnames containing the domain components that follow.</p>
+
+ <example>
+ # Match www.apache.org, and fooapache.org<br />
+ CacheEnable disk http://*apache.org/<br />
+ # Match www.apache.org, but not fooapache.org<br />
+ CacheEnable disk http://.apache.org/<br />
+ </example>
+
<p> The <code>no-cache</code> environment variable can be set to
disable caching on a finer grained set of resources in versions
2.2.12 and later.</p>
Modified: httpd/httpd/trunk/modules/cache/cache_util.c
URL: http://svn.apache.org/viewvc/httpd/httpd/trunk/modules/cache/cache_util.c?rev=821333&r1=821332&r2=821333&view=diff
==============================================================================
--- httpd/httpd/trunk/modules/cache/cache_util.c (original)
+++ httpd/httpd/trunk/modules/cache/cache_util.c Sat Oct 3 14:54:00 2009
@@ -26,42 +26,79 @@
/* Determine if "url" matches the hostname, scheme and port and path
* in "filter". All but the path comparisons are case-insensitive.
+ * Note: the 's' parameters is not used currently, but needed for
+ * logging during debugging.
*/
-static int uri_meets_conditions(apr_uri_t filter, int pathlen, apr_uri_t url)
-{
- /* Compare the hostnames */
- if(filter.hostname) {
- if (!url.hostname) {
+static int uri_meets_conditions(const server_rec * const s,
+ const apr_uri_t filter, const int pathlen, const apr_uri_t url) {
+ (void) s;
+
+ /* Scheme, hostname port and local part. The filter URI and the
+ * URI we test may have the following shapes:
+ * /<path>
+ * <scheme>[:://<hostname>[:<port>][/<path>]]
+ * That is, if there is no scheme then there must be only the path,
+ * and we check only the path; if there is a scheme, we check the
+ * scheme for equality, and then if present we match the hostname,
+ * and then if present match the port, and finally the path if any.
+ *
+ * Note that this means that "/<path>" only matches local paths,
+ * and to match proxied paths one *must* specify the scheme.
+ */
+
+ /* Is the filter is just for a local path or a proxy URI? */
+ if (!filter.scheme) {
+ if (url.scheme || url.hostname) {
return 0;
}
- else if (strcasecmp(filter.hostname, url.hostname)) {
+ } else {
+ /* The URI scheme must be present and identical except for case. */
+ if (!url.scheme || strcasecmp(filter.scheme, url.scheme)) {
return 0;
}
- }
- /* Compare the schemes */
- if(filter.scheme) {
- if (!url.scheme) {
- return 0;
- }
- else if (strcasecmp(filter.scheme, url.scheme)) {
- return 0;
+ /* If the filter hostname is null or empty it matches any hostname,
+ * if it begins with a "*" it matches the _end_ of the URI hostname
+ * excluding the "*", if it begins with a "." it matches the _end_
+ * of the URI * hostname including the ".", otherwise it must match
+ * the URI hostname exactly. */
+
+ if (filter.hostname && filter.hostname[0]) {
+ if (filter.hostname[0] == '.') {
+ const size_t fhostlen = strlen(filter.hostname);
+ const size_t uhostlen = url.hostname ? strlen(url.hostname) : 0;
+
+ if (fhostlen > uhostlen || strcasecmp(filter.hostname,
+ url.hostname + uhostlen - fhostlen)) {
+ return 0;
+ }
+ } else if (filter.hostname[0] == '*') {
+ const size_t fhostlen = strlen(filter.hostname + 1);
+ const size_t uhostlen = url.hostname ? strlen(url.hostname) : 0;
+
+ if (fhostlen > uhostlen || strcasecmp(filter.hostname + 1,
+ url.hostname + uhostlen - fhostlen)) {
+ return 0;
+ }
+ } else if (!url.hostname || strcasecmp(filter.hostname, url.hostname)) {
+ return 0;
+ }
}
- }
- /* Compare the ports */
- if(filter.port_str) {
- if (url.port_str && filter.port != url.port) {
- return 0;
- }
- /* NOTE: ap_port_of_scheme will return 0 if given NULL input */
- else if (filter.port != apr_uri_port_of_scheme(url.scheme)) {
- return 0;
- }
- }
- else if(url.port_str && filter.scheme) {
- if (apr_uri_port_of_scheme(filter.scheme) == url.port) {
- return 0;
+ /* If the filter port is empty it matches any URL port.
+ * If the filter or URL port are missing, or the URL port is
+ * empty, they default to the port for their scheme. */
+
+ if (!(filter.port_str && !filter.port_str[0])) {
+ /* NOTE: ap_port_of_scheme will return 0 if given NULL input */
+ const unsigned fport = filter.port_str ? filter.port
+ : apr_uri_port_of_scheme(filter.scheme);
+ const unsigned uport = (url.port_str && url.port_str[0])
+ ? url.port : apr_uri_port_of_scheme(url.scheme);
+
+ if (fport != uport) {
+ return 0;
+ }
}
}
@@ -71,8 +108,7 @@
if (!url.path) {
if (*filter.path == '/' && pathlen == 1) {
return 1;
- }
- else {
+ } else {
return 0;
}
}
@@ -94,7 +130,7 @@
for (i = 0; i < conf->cacheenable->nelts; i++) {
struct cache_enable *ent =
(struct cache_enable *)conf->cacheenable->elts;
- if (uri_meets_conditions(ent[i].url, ent[i].pathlen, uri)) {
+ if (uri_meets_conditions(r->server,ent[i].url, ent[i].pathlen, uri)) {
/* Fetch from global config and add to the list. */
cache_provider *provider;
provider = ap_lookup_provider(CACHE_PROVIDER_GROUP, ent[i].type,
@@ -131,7 +167,7 @@
for (i = 0; i < conf->cachedisable->nelts; i++) {
struct cache_disable *ent =
(struct cache_disable *)conf->cachedisable->elts;
- if (uri_meets_conditions(ent[i].url, ent[i].pathlen, uri)) {
+ if (uri_meets_conditions(r->server,ent[i].url, ent[i].pathlen, uri)) {
/* Stop searching now. */
return NULL;
}
Re: svn commit: r821333 - in /httpd/httpd/trunk: CHANGES
docs/manual/mod/mod_cache.xml modules/cache/cache_util.c
Posted by Jeff Trawick <tr...@gmail.com>.
On Sun, Oct 4, 2009 at 9:22 AM, Graham Leggett <mi...@sharp.fm> wrote:
> Jeff Trawick wrote:
>
> > My gut instinct when I see something odd is that I'd like to know what
> > that was for.
>
> First off, I am not in a position to tell you why it was done like that,
> that came from the original contributor, so I don't know why you were
> asking me. Although having looked at it it is pretty obvious why.
>
You committed the code, so questions will be addressed to you based on the
the general expectation that you reviewed and understood it. It isn't
appropriate for you to expect me to track down the original contributor and
ask them.
> The original author had added ap_log_error() into the code, and when
> they were done they removed them again, but forgot to remove the
> server_rec they had wired through that made ap_log_error() possible.
>
> And it's pretty obvious how it could be missed by me. The comment at the
> top clearly explained why the server_rec had been added. I expected to
> see ap_log_error() further down, but instead got distracted for a few
> hours by the fact the patch had completely rewritten the function and in
> its original form didn't fully work.
>
> And that was fine too, because you picked it up, creating the
> opportunity for it to be fixed.
>
> > Yeah, I'm pretty sure it is wrong and needs to be
> > removed, but I wondered why as well. I suppose I could have said "I
> > don't see any purpose for this, so delete."
>
> Just flag it to be looked at further, as in "you missed a spot".
>
I think that is a very extreme position. I don't believe that anyone here
is going to say "you missed a spot" in lieu of something more specific about
a code construct (except when making some boilerplate change and missing 1
of n occurrences).
>
> > What I think is that you are not comfortable having a civil conversation
> > about your commits.
> >
> > Would "Please delete this unnecessary code" have been better received
> > than explaining why I thought it was preferable to leave that minimal
> > debug support out?
>
> It would have, yes.
>
> Rhetorical questions come across as "how could you have missed that
> moron" even though that may not have been the original intention.
>
How about just answering questions straight-up and seeing how that goes?
I'm not trying to say "how could you have missed that" with any of my
comments.
Re: svn commit: r821333 - in /httpd/httpd/trunk: CHANGES docs/manual/mod/mod_cache.xml
modules/cache/cache_util.c
Posted by Graham Leggett <mi...@sharp.fm>.
Jeff Trawick wrote:
> My gut instinct when I see something odd is that I'd like to know what
> that was for.
First off, I am not in a position to tell you why it was done like that,
that came from the original contributor, so I don't know why you were
asking me. Although having looked at it it is pretty obvious why.
The original author had added ap_log_error() into the code, and when
they were done they removed them again, but forgot to remove the
server_rec they had wired through that made ap_log_error() possible.
And it's pretty obvious how it could be missed by me. The comment at the
top clearly explained why the server_rec had been added. I expected to
see ap_log_error() further down, but instead got distracted for a few
hours by the fact the patch had completely rewritten the function and in
its original form didn't fully work.
And that was fine too, because you picked it up, creating the
opportunity for it to be fixed.
> Yeah, I'm pretty sure it is wrong and needs to be
> removed, but I wondered why as well. I suppose I could have said "I
> don't see any purpose for this, so delete."
Just flag it to be looked at further, as in "you missed a spot".
> What I think is that you are not comfortable having a civil conversation
> about your commits.
>
> Would "Please delete this unnecessary code" have been better received
> than explaining why I thought it was preferable to leave that minimal
> debug support out?
It would have, yes.
Rhetorical questions come across as "how could you have missed that
moron" even though that may not have been the original intention.
> It's late, I'm tired and I'm off to bed. The style can be fixed
> tomorrow.
>
>
> It will be appreciated.
Fixed in r821538.
Regards,
Graham
--
Re: svn commit: r821333 - in /httpd/httpd/trunk: CHANGES
docs/manual/mod/mod_cache.xml modules/cache/cache_util.c
Posted by Jeff Trawick <tr...@gmail.com>.
On Sat, Oct 3, 2009 at 8:44 PM, Graham Leggett <mi...@sharp.fm> wrote:
> Jeff Trawick wrote:
>
> + *) mod_cache: Fix uri_meets_conditions() so that CacheEnable will
>> + match by scheme, or by a wildcarded hostname. PR 40169
>> + [Ryan Pendergast <rpender us.ibm.com <http://us.ibm.com>>,
>> Graham Leggett]
>> +
>>
>>
>> I guess it is "Submitted:" by both you and Ryan? (Commit log doesn't
>> match CHANGES?)
>>
>> This is curious because this patch looks most like one attached to the
>> issue by Peter Grandi. The last patch attached by Ryan is a one-liner, and
>> now marked obsolete.
>>
>
> I mixed up the names, thanks for the catch.
>
> The patch is different enough that if I was the original contributor, I'd
> say "oi, that isn't my code any more, and the person who reviewed it changed
> it, so don't blame me for their bugs".
>
> CHANGES doesn't make distinctions about who did what to code, it just lists
> the people responsible, usually in the order of contribution.
>
> The commit log makes a clearer distinction that the code was originally
> submitted, and then reviewed and modified by me. They do not contradict one
> another.
IMO the "modified by me" part is lost.
>
>
> some aspects of this patch change the style or handle certain issues that
>> are ignored everywhere else
>>
>
> That's because the entire afternoon I devoted to this patch was spent
> painstakingly testing each path through the code, to make 100% sure that it
> worked both for the forward and reverse proxy case (the original patch
> didn't work in the reverse proxy case).
>
> The original patch didn't follow the style of mod_cache at all, and I
> changed the majority of it to match. Obviously I didn't catch all of it, but
> then I prioritised whether it worked or not over and above where a brace was
> placed.
>
> we have unused parameters all over the place (e.g., with hook functions);
>> why we need to do "(void) s;" here?
>>
>
> Why are you asking why?
>
> If it's wrong, highlight it, we fix it, we move on.
My gut instinct when I see something odd is that I'd like to know what that
was for. Yeah, I'm pretty sure it is wrong and needs to be removed, but I
wondered why as well. I suppose I could have said "I don't see any purpose
for this, so delete."
>
> also, since no debug provision utilizing s was committed, the developer
>> has to add code anyway when wanting to log something; can't they just add s
>> at that time instead of leaving it in the code? it should take all of 15
>> seconds to do that
>>
>
> Did you think I just took the patch as it was and applied it blindly to
> httpd-trunk and committed it?
>
>
What I think is that you are not comfortable having a civil conversation
about your commits.
Would "Please delete this unnecessary code" have been better received than
explaining why I thought it was preferable to leave that minimal debug
support out?
> The fact that you can only find issues with the style, and not with the
> functionality indicates that the afternoon's work was a success.
>
Good for you for moving an outside patch forward; that's great, of course.
>
> It's late, I'm tired and I'm off to bed. The style can be fixed tomorrow.
>
It will be appreciated.
Re: svn commit: r821333 - in /httpd/httpd/trunk: CHANGES docs/manual/mod/mod_cache.xml
modules/cache/cache_util.c
Posted by Graham Leggett <mi...@sharp.fm>.
Jeff Trawick wrote:
> + *) mod_cache: Fix uri_meets_conditions() so that CacheEnable will
> + match by scheme, or by a wildcarded hostname. PR 40169
> + [Ryan Pendergast <rpender us.ibm.com <http://us.ibm.com>>,
> Graham Leggett]
> +
>
>
> I guess it is "Submitted:" by both you and Ryan? (Commit log doesn't
> match CHANGES?)
>
> This is curious because this patch looks most like one attached to the
> issue by Peter Grandi. The last patch attached by Ryan is a one-liner,
> and now marked obsolete.
I mixed up the names, thanks for the catch.
The patch is different enough that if I was the original contributor,
I'd say "oi, that isn't my code any more, and the person who reviewed it
changed it, so don't blame me for their bugs".
CHANGES doesn't make distinctions about who did what to code, it just
lists the people responsible, usually in the order of contribution.
The commit log makes a clearer distinction that the code was originally
submitted, and then reviewed and modified by me. They do not contradict
one another.
> some aspects of this patch change the style or handle certain issues
> that are ignored everywhere else
That's because the entire afternoon I devoted to this patch was spent
painstakingly testing each path through the code, to make 100% sure that
it worked both for the forward and reverse proxy case (the original
patch didn't work in the reverse proxy case).
The original patch didn't follow the style of mod_cache at all, and I
changed the majority of it to match. Obviously I didn't catch all of it,
but then I prioritised whether it worked or not over and above where a
brace was placed.
> we have unused parameters all over the place (e.g., with hook
> functions); why we need to do "(void) s;" here?
Why are you asking why?
If it's wrong, highlight it, we fix it, we move on.
> also, since no debug provision utilizing s was committed, the developer
> has to add code anyway when wanting to log something; can't they just
> add s at that time instead of leaving it in the code? it should take
> all of 15 seconds to do that
Did you think I just took the patch as it was and applied it blindly to
httpd-trunk and committed it?
The fact that you can only find issues with the style, and not with the
functionality indicates that the afternoon's work was a success.
It's late, I'm tired and I'm off to bed. The style can be fixed tomorrow.
Regards,
Graham
--
Re: svn commit: r821333 - in /httpd/httpd/trunk: CHANGES
docs/manual/mod/mod_cache.xml modules/cache/cache_util.c
Posted by Jeff Trawick <tr...@gmail.com>.
On Sat, Oct 3, 2009 at 10:54 AM, <mi...@apache.org> wrote:
> Author: minfrin
> Date: Sat Oct 3 14:54:00 2009
> New Revision: 821333
>
> URL: http://svn.apache.org/viewvc?rev=821333&view=rev
> Log:
> mod_cache: Fix uri_meets_conditions() so that CacheEnable will
> match by scheme, or by a wildcarded hostname.
> PR: 40169
> Submitted by: Ryan Pendergast <rpender us.ibm.com>
> Reviewed by: Graham Leggett
>
> Modified:
> httpd/httpd/trunk/CHANGES
> httpd/httpd/trunk/docs/manual/mod/mod_cache.xml
> httpd/httpd/trunk/modules/cache/cache_util.c
>
> Modified: httpd/httpd/trunk/CHANGES
> URL:
> http://svn.apache.org/viewvc/httpd/httpd/trunk/CHANGES?rev=821333&r1=821332&r2=821333&view=diff
>
> ==============================================================================
> --- httpd/httpd/trunk/CHANGES [utf-8] (original)
> +++ httpd/httpd/trunk/CHANGES [utf-8] Sat Oct 3 14:54:00 2009
> @@ -10,6 +10,10 @@
> mod_proxy_ftp: NULL pointer dereference on error paths.
> [Stefan Fritsch <sf fritsch.de>, Joe Orton]
>
> + *) mod_cache: Fix uri_meets_conditions() so that CacheEnable will
> + match by scheme, or by a wildcarded hostname. PR 40169
> + [Ryan Pendergast <rpender us.ibm.com>, Graham Leggett]
> +
>
I guess it is "Submitted:" by both you and Ryan? (Commit log doesn't match
CHANGES?)
This is curious because this patch looks most like one attached to the issue
by Peter Grandi. The last patch attached by Ryan is a one-liner, and now
marked obsolete.
> Modified: httpd/httpd/trunk/modules/cache/cache_util.c
> URL:
> http://svn.apache.org/viewvc/httpd/httpd/trunk/modules/cache/cache_util.c?rev=821333&r1=821332&r2=821333&view=diff
>
> ==============================================================================
> --- httpd/httpd/trunk/modules/cache/cache_util.c (original)
> +++ httpd/httpd/trunk/modules/cache/cache_util.c Sat Oct 3 14:54:00 2009
>
some aspects of this patch change the style or handle certain issues that
are ignored everywhere else
> @@ -26,42 +26,79 @@
>
> /* Determine if "url" matches the hostname, scheme and port and path
> * in "filter". All but the path comparisons are case-insensitive.
> + * Note: the 's' parameters is not used currently, but needed for
> + * logging during debugging.
> */
> -static int uri_meets_conditions(apr_uri_t filter, int pathlen, apr_uri_t
> url)
> -{
> - /* Compare the hostnames */
> - if(filter.hostname) {
> - if (!url.hostname) {
> +static int uri_meets_conditions(const server_rec * const s,
> + const apr_uri_t filter, const int pathlen, const apr_uri_t url) {
> + (void) s;
>
we have unused parameters all over the place (e.g., with hook functions);
why we need to do "(void) s;" here?
also, since no debug provision utilizing s was committed, the developer has
to add code anyway when wanting to log something; can't they just add s at
that time instead of leaving it in the code? it should take all of 15
seconds to do that
+ /* Is the filter is just for a local path or a proxy URI? */
> + if (!filter.scheme) {
> + if (url.scheme || url.hostname) {
> return 0;
> }
> - else if (strcasecmp(filter.hostname, url.hostname)) {
> + } else {
>
I guess "} else {" is okay, even though 90% of the code doesn't use that
construct.
@@ -71,8 +108,7 @@
if (!url.path) {
> if (*filter.path == '/' && pathlen == 1) {
> return 1;
> - }
> - else {
> + } else {
>
but it is even more odd to see that the commit modifies existing code to
follow the rarely used style of the new code
@@ -94,7 +130,7 @@
> for (i = 0; i < conf->cacheenable->nelts; i++) {
> struct cache_enable *ent =
> (struct cache_enable
> *)conf->cacheenable->elts;
> - if (uri_meets_conditions(ent[i].url, ent[i].pathlen, uri)) {
> + if (uri_meets_conditions(r->server,ent[i].url, ent[i].pathlen,
> uri)) {
>
more odd style