httpd-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Jeff Trawick <traw...@gmail.com>
Subject Re: svn commit: r821333 - in /httpd/httpd/trunk: CHANGES docs/manual/mod/mod_cache.xml modules/cache/cache_util.c
Date Sat, 03 Oct 2009 16:20:01 GMT
On Sat, Oct 3, 2009 at 10:54 AM, <minfrin@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

Mime
View raw message