httpd-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Ruediger Pluem <rpl...@apache.org>
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 19:56:03 GMT

On 03.10.2009 16:54, 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/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.

It is not even used for logging. Why is it present?
Please remove it or actually do logging.

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

What is this?

> +
> +    /* 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 {

Style.

> +        /* 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] == '*') {

Style.

> +                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)) {

Style.

> +                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);

IMHO we cannot be sure that url.scheme != NULL here.

> +
> +            if (fport != uport) {
> +                return 0;
> +            }
>          }
>      }
>  
> @@ -71,8 +108,7 @@
>      if (!url.path) {
>          if (*filter.path == '/' && pathlen == 1) {
>              return 1;
> -        }
> -        else {
> +        } else {

Please fix your editor to avoid these kind of whitespace changes
that as in case even violate our style guide.

Regards

RĂ¼diger


Mime
View raw message