GeoIPv6 patch

Maxim Dounin mdounin at mdounin.ru
Wed Jun 15 03:47:18 MSD 2011


Hello!

On Tue, Jun 14, 2011 at 07:52:10AM +0200, Gregor Kališnik wrote:

> Hi.
> 
> On Tuesday 14 of June 2011 05:38:59 you wrote:
> > Hello!
> > 
> > On Sat, Jun 11, 2011 at 08:19:52PM +0200, Gregor Kališnik wrote:
> > > Hi.
> 
> > You can't assume you have sockaddr_in6 unless compiled with ipv6.
> 
> If IPv6 is not compiled, then GeoIPv6 also won't be compiled.
> 
> > This should go to auto/lib/geoip/conf.  And probably should be
> > made shorter.
> > 
> > Naming this NGX_HAVE_GEOIP_IPV6 would be better IMHO.
> 
> I'm checking for all used variables. Is it sufficient to look just for one?

I believe it would be sufficient to just check for geoipv6_t type.

    ngx_feature_incs="#include <GeoIP.h>"
    ...
    ngx_feature_test="geoipv6_t  v6addr;
                      v6addr.s6_addr[0] = 1;"

> > How this may happen and what's the expected result?
> > 
> 
> The whole ngx_http_geoip_addr_v6 is now absent from no-ipv6 version.

Looks much better now, thanks.  Below some minor issues noted 
(mostly style nitpicking).

> 
> 
> Best regards,
> Gregor Kališnik

> diff -ru nginx-1.0.4/auto/lib/geoip/conf nginx-1.0.4-patched/auto/lib/geoip/conf
> --- nginx-1.0.4/auto/lib/geoip/conf	2009-07-20 09:10:43.000000000 +0200
> +++ nginx-1.0.4-patched/auto/lib/geoip/conf	2011-06-14 07:45:10.902629103 +0200
> @@ -65,6 +65,24 @@
>  if [ $ngx_found = yes ]; then
>      CORE_LIBS="$CORE_LIBS $ngx_feature_libs"
>  
> +    if [ $NGX_IPV6 = YES ]; then
> +        ngx_feature="GeoIP IPV6 support"
> +        ngx_feature_name="NGX_HAVE_GEOIP_IPV6"
> +        ngx_feature_run=no
> +        ngx_feature_incs='#include <stdio.h>
> +                        #include <GeoIP.h>'
> +        ngx_feature_path=
> +        ngx_feature_libs=
> +        ngx_feature_test='printf("%d", GEOIP_COUNTRY_EDITION_V6);
> +                        printf("%d", GEOIP_ISP_EDITION_V6);
> +                        printf("%d", GEOIP_ORG_EDITION_V6);
> +                        printf("%d", GEOIP_DOMAIN_EDITION_V6);
> +                        printf("%d", GEOIP_ASNUM_EDITION_V6);
> +                        printf("%d", GEOIP_CITY_EDITION_REV0_V6);
> +                        printf("%d", GEOIP_CITY_EDITION_REV1_V6);'

Please use double quotes and align content (see above).

> +        . auto/feature
> +    fi
> +
>  else
>  
>  cat << END
> diff -ru nginx-1.0.4/src/http/modules/ngx_http_geoip_module.c nginx-1.0.4-patched/src/http/modules/ngx_http_geoip_module.c
> --- nginx-1.0.4/src/http/modules/ngx_http_geoip_module.c	2011-05-16 15:50:58.000000000 +0200
> +++ nginx-1.0.4-patched/src/http/modules/ngx_http_geoip_module.c	2011-06-14 07:44:09.575777908 +0200
> @@ -11,11 +11,19 @@
>  #include <GeoIP.h>
>  #include <GeoIPCity.h>
>  
> +/* Index numbers for GeoIP functions */

Two blank lines between includes and defines, please.  And comment 
looks unneeded.

> +#define NGX_GEOIP_COUNTRY_CODE  0
> +#define NGX_GEOIP_COUNTRY_CODE3 1
> +#define NGX_GEOIP_COUNTRY_NAME  2
> +
>  
>  typedef struct {
>      GeoIP      *country;
>      GeoIP      *org;
>      GeoIP      *city;
> +    unsigned    country_ipv6:1;
> +    unsigned    org_ipv6:1;
> +    unsigned    city_ipv6:1;
>  } ngx_http_geoip_conf_t;
>  
>  
> @@ -27,6 +35,26 @@
>  
>  typedef const char *(*ngx_http_geoip_variable_handler_pt)(GeoIP *, u_long addr);
>  
> +
> +uintptr_t ngx_http_geoip_country_functions[] = {
> +    (uintptr_t) GeoIP_country_code_by_ipnum,
> +    (uintptr_t) GeoIP_country_code3_by_ipnum,
> +    (uintptr_t) GeoIP_country_name_by_ipnum,
> +};

I tend to think I wasn't right here suggesting (uintptr_t) cast.  
Using real type would be better (and we have it anyway).

> +
> +
> +#if (NGX_HAVE_GEOIP_IPV6)
> +typedef const char *(*ngx_http_geoip_variable_handler_v6_pt)(GeoIP *, geoipv6_t addr);
> +
> +
> +uintptr_t ngx_http_geoip_country_ipv6_functions[] = {
> +    (uintptr_t) GeoIP_country_code_by_ipnum_v6,
> +    (uintptr_t) GeoIP_country_code3_by_ipnum_v6,
> +    (uintptr_t) GeoIP_country_name_by_ipnum_v6,
> +};
> +#endif
> +
> +
>  static ngx_int_t ngx_http_geoip_country_variable(ngx_http_request_t *r,
>      ngx_http_variable_value_t *v, uintptr_t data);
>  static ngx_int_t ngx_http_geoip_org_variable(ngx_http_request_t *r,
> @@ -114,19 +142,19 @@
>  
>      { ngx_string("geoip_country_code"), NULL,
>        ngx_http_geoip_country_variable,
> -      (uintptr_t) GeoIP_country_code_by_ipnum, 0, 0 },
> +      NGX_GEOIP_COUNTRY_CODE, 0, 0 },
>  
>      { ngx_string("geoip_country_code3"), NULL,
>        ngx_http_geoip_country_variable,
> -      (uintptr_t) GeoIP_country_code3_by_ipnum, 0, 0 },
> +      NGX_GEOIP_COUNTRY_CODE3, 0, 0 },
>  
>      { ngx_string("geoip_country_name"), NULL,
>        ngx_http_geoip_country_variable,
> -      (uintptr_t) GeoIP_country_name_by_ipnum, 0, 0 },
> +      NGX_GEOIP_COUNTRY_NAME, 0, 0 },
>  
>      { ngx_string("geoip_org"), NULL,
>        ngx_http_geoip_org_variable,
> -      (uintptr_t) GeoIP_name_by_ipnum, 0, 0 },
> +      0, 0, 0 },
>  
>      { ngx_string("geoip_city_continent_code"), NULL,
>        ngx_http_geoip_city_variable,
> @@ -217,16 +245,57 @@
>      return INADDR_NONE;
>  }
>  
> +#if (NGX_HAVE_GEOIP_IPV6)
> +static geoipv6_t
> +ngx_http_geoip_addr_v6(ngx_http_request_t *r)
> +{
> +    struct sockaddr_in6  *sin;

Please use "sin6" for sockaddr_in6, and "sin" for sockaddr_in (and 
"saun" for sockaddr_un, but this doesn't matter here).

> +    /* For IPv4 to IPv6 mapping */

Comment seems obvious and unneeded.

> +    struct sockaddr_in   *sin4;
> +    struct in6_addr       v6addr;
> +    u_long                raw_v4addr;

What about "addr" for ipv4-one, and addr6 for ipv6-one?  This will 
be more consistent with ngx_http_geoip_addr() and other naming.

> +    u_int8_t              i;

Variable "i" looks unused, nginx should refuse to compile.

> +
> +    switch (r->connection->sockaddr->sa_family) {
> +
> +    case AF_INET:
> +        sin4 = (struct sockaddr_in *) r->connection->sockaddr;
> +        raw_v4addr = ntohl(sin4->sin_addr.s_addr);
> +        /* Clear first 8 bytes */

Comment is obvious and not consistent with code (you actually zero 
10 bytes).  You may instead add something like

           /* produce IPv4 mapped IPv6 address */

in the beginning of the "case AF_INET", this would be much more 
helpful.

> +        memset(&v6addr, 0, 10);

You shouldn't assume struct in6_addr contains *only* s6_addr.  Per 
POSIX[1] it's expected to contain *at least* s6_addr.  So please 
use

           ngx_memzero(&v6addr, sizeof(struct in6_addr));

[1] http://pubs.opengroup.org/onlinepubs/9699919799/basedefs/netinet_in.h.html

> +        v6addr.s6_addr[10] = 0xFF;
> +        v6addr.s6_addr[11] = 0xFF;
> +        v6addr.s6_addr[12] = raw_v4addr >> 24;
> +        v6addr.s6_addr[13] = raw_v4addr >> 16;
> +        v6addr.s6_addr[14] = raw_v4addr >> 8;
> +        v6addr.s6_addr[15] = raw_v4addr;
> +
> +        return v6addr;
> +
> +    case AF_INET6:
> +        sin = (struct sockaddr_in6 *) r->connection->sockaddr;
> +
> +        return sin->sin6_addr;

No need for blank line here.

> +
> +    }
> +
> +    return in6addr_any;
> +}
> +#endif
>  
>  static ngx_int_t

Two blank lines between functions, please.

>  ngx_http_geoip_country_variable(ngx_http_request_t *r,
>      ngx_http_variable_value_t *v, uintptr_t data)
>  {
> -    ngx_http_geoip_variable_handler_pt  handler =
> -        (ngx_http_geoip_variable_handler_pt) data;
> +#if (NGX_HAVE_GEOIP_IPV6)
> +    ngx_http_geoip_variable_handler_v6_pt handler_v6 =
> +        (ngx_http_geoip_variable_handler_v6_pt) ngx_http_geoip_country_ipv6_functions[data];
> +#endif
> +    ngx_http_geoip_variable_handler_pt    handler =
> +        (ngx_http_geoip_variable_handler_pt) ngx_http_geoip_country_functions[data];
>  
> -    const char             *val;
> -    ngx_http_geoip_conf_t  *gcf;
> +    const char                           *val;
> +    ngx_http_geoip_conf_t                *gcf;
>  
>      gcf = ngx_http_get_module_main_conf(r, ngx_http_geoip_module);
>  
> @@ -234,7 +303,15 @@
>          goto not_found;
>      }
>  
> +#if (NGX_HAVE_GEOIP_IPV6)
> +    if (gcf->country_ipv6) {
> +        val = handler_v6(gcf->country, ngx_http_geoip_addr_v6(r));
> +    } else {
> +        val = handler(gcf->country, ngx_http_geoip_addr(r));
> +    }
> +#else
>      val = handler(gcf->country, ngx_http_geoip_addr(r));
> +#endif

IMHO, something like this will be more readable:

#if (NGX_HAVE_GEOIP_IPV6)
    val = gcf->country_ipv6
              ? handler_v6(gcf->country, ngx_http_geoip_addr_v6(r))
              : handler(gcf->country, ngx_http_geoip_addr(r));
#else
    val = handler(gcf->country, ngx_http_geoip_addr(r));
#endif

Just a matter of taste though.

>  
>      if (val == NULL) {
>          goto not_found;
> @@ -260,9 +337,6 @@
>  ngx_http_geoip_org_variable(ngx_http_request_t *r,
>      ngx_http_variable_value_t *v, uintptr_t data)
>  {
> -    ngx_http_geoip_variable_handler_pt  handler =
> -        (ngx_http_geoip_variable_handler_pt) data;
> -
>      const char             *val;
>      ngx_http_geoip_conf_t  *gcf;
>  
> @@ -272,7 +346,15 @@
>          goto not_found;
>      }
>  
> -    val = handler(gcf->org, ngx_http_geoip_addr(r));
> +#if (NGX_HAVE_GEOIP_IPV6)
> +    if (gcf->org_ipv6) {
> +        val = GeoIP_name_by_ipnum_v6(gcf->org, ngx_http_geoip_addr_v6(r));
> +    } else {
> +        val = GeoIP_name_by_ipnum(gcf->org, ngx_http_geoip_addr(r));
> +    }
> +#else
> +    val = GeoIP_name_by_ipnum(gcf->org, ngx_http_geoip_addr(r));
> +#endif
>  
>      if (val == NULL) {
>          goto not_found;
> @@ -452,7 +534,15 @@
>      gcf = ngx_http_get_module_main_conf(r, ngx_http_geoip_module);
>  
>      if (gcf->city) {
> +#if (NGX_HAVE_GEOIP_IPV6)
> +        if (gcf->city_ipv6) {
> +            return GeoIP_record_by_ipnum_v6(gcf->city, ngx_http_geoip_addr_v6(r));
> +        } else {
> +            return GeoIP_record_by_ipnum(gcf->city, ngx_http_geoip_addr(r));
> +        }
> +#else
>          return GeoIP_record_by_ipnum(gcf->city, ngx_http_geoip_addr(r));
> +#endif
>      }
>  
>      return NULL;
> @@ -540,8 +630,16 @@
>      case GEOIP_PROXY_EDITION:
>      case GEOIP_NETSPEED_EDITION:
>  
> +        gcf->country_ipv6 = 0;
>          return NGX_CONF_OK;
>  
> +#if (NGX_HAVE_GEOIP_IPV6)
> +    case GEOIP_COUNTRY_EDITION_V6:
> +
> +        gcf->country_ipv6 = 1;
> +        return NGX_CONF_OK;
> +#endif
> +
>      default:
>          ngx_conf_log_error(NGX_LOG_EMERG, cf, 0,
>                             "invalid GeoIP database \"%V\" type:%d",
> @@ -591,7 +689,18 @@
>      case GEOIP_DOMAIN_EDITION:
>      case GEOIP_ASNUM_EDITION:
>  
> +        gcf->org_ipv6 = 0;
> +        return NGX_CONF_OK;
> +
> +#if (NGX_HAVE_GEOIP_IPV6)
> +    case GEOIP_ISP_EDITION_V6:
> +    case GEOIP_ORG_EDITION_V6:
> +    case GEOIP_DOMAIN_EDITION_V6:
> +    case GEOIP_ASNUM_EDITION_V6:
> +
> +        gcf->org_ipv6 = 1;
>          return NGX_CONF_OK;
> +#endif
>  
>      default:
>          ngx_conf_log_error(NGX_LOG_EMERG, cf, 0,
> @@ -640,7 +749,16 @@
>      case GEOIP_CITY_EDITION_REV0:
>      case GEOIP_CITY_EDITION_REV1:
>  
> +        gcf->city_ipv6 = 0;
> +        return NGX_CONF_OK;
> +
> +#if (NGX_HAVE_GEOIP_IPV6)
> +    case GEOIP_CITY_EDITION_REV0_V6:
> +    case GEOIP_CITY_EDITION_REV1_V6:
> +
> +        gcf->city_ipv6 = 1;
>          return NGX_CONF_OK;
> +#endif
>  
>      default:
>          ngx_conf_log_error(NGX_LOG_EMERG, cf, 0,


Maxim Dounin



More information about the nginx-devel mailing list