GeoIPv6 patch

Maxim Dounin mdounin at mdounin.ru
Tue Jun 14 05:38:59 MSD 2011


Hello!

On Sat, Jun 11, 2011 at 08:19:52PM +0200, Gregor Kališnik wrote:

> Hi.
> 
> On Saturday 11 of June 2011 16:54:18 you wrote:
> > Unrelated whitespace breakage.  There are multiple other style
> > issues, too.
> 
> There souldn't be any now in this patch.
> 
> 
> > I don't really like this aproach.  Result looks to cluttered with
> > #if's and hard to read.  Probably it's good idea to try to produce
> > something better.
> 
> Put one #if .. #endif block at the begining. Not sure if it's good enough 
> (does inherit compile errors, but makes two identical pathways)...
> 
> > Using inet_ntop/sprintf/inet_pton to map ipv4 binary address to
> > ipv4-mapped ipv6 binary address is cool.  :)
> > 
> > What about just setting appropriate bytes?
> 
> Now using binary operators :).
> 
> 
> > It's probably good idea to centralize all these checks at
> > configuration stage.
> > 
> > Please also note that nginx style is to use 4 spaces for
> > indentation, and always use {}.
> 
> I added three variables to configure struct (is_<type>_ipv6).
> 
> 
> > Some configure checks for ipv6 support in libgeoip may also be
> > needed as I have no idea if recent libgeoip versions are widely
> > availabe on some known-to-be-slow Linux'es.  Not sure, but it at
> > least deserves some investigation.
> > 
> > Maxim Dounin
> 
> Added a check into auto/unix. Check for availability of database type names.
> 
> Best regards,
> Gregor Kališnik

> diff -ru nginx-1.0.4/auto/unix nginx-1.0.4-patched/auto/unix
> --- nginx-1.0.4/auto/unix	2011-05-31 10:25:10.000000000 +0200
> +++ nginx-1.0.4-patched/auto/unix	2011-06-11 20:07:09.783912047 +0200
> @@ -440,6 +440,25 @@
>  fi
>  
>  
> +if [ $NGX_IPV6 = YES -a $NGX_GEOIP = YES ]; then
> +    ngx_feature="GEOIPV6"
> +    ngx_feature_name="NGX_HAVE_GEOIPV6"
> +    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);'
> +    . auto/feature
> +fi

This should go to auto/lib/geoip/conf.  And probably should be 
made shorter.

Naming this NGX_HAVE_GEOIP_IPV6 would be better IMHO.

> +
> +
>  ngx_feature="setproctitle()"
>  ngx_feature_name="NGX_HAVE_SETPROCTITLE"
>  ngx_feature_run=no
> 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-11 19:50:58.097700814 +0200
> @@ -12,10 +12,23 @@
>  #include <GeoIPCity.h>
>  
>  
> +#if (!NGX_HAVE_GEOIPV6)
> +#define geoipv6_t u_long
> +#define GeoIP_country_code_by_ipnum_v6 GeoIP_country_code_by_ipnum
> +#define GeoIP_country_code3_by_ipnum_v6 GeoIP_country_code3_by_ipnum
> +#define GeoIP_country_name_by_ipnum_v6 GeoIP_country_name_by_ipnum
> +#define GeoIP_name_by_ipnum_v6 GeoIP_name_by_ipnum
> +#define GeoIP_record_by_ipnum_v6 GeoIP_record_by_ipnum
> +#endif
> +
> +
>  typedef struct {
>      GeoIP      *country;
>      GeoIP      *org;
>      GeoIP      *city;
> +    u_int8_t    is_country_ipv6;
> +    u_int8_t    is_org_ipv6;
> +    u_int8_t    is_city_ipv6;

Something like this would be better:

       unsigned    country_ipv6:1;
       unsigned    org_ipv6:1;
       unsigned    city_ipv6:1;

>  } ngx_http_geoip_conf_t;
>  
>  
> @@ -26,6 +39,33 @@
>  
>  
>  typedef const char *(*ngx_http_geoip_variable_handler_pt)(GeoIP *, u_long addr);
> +typedef const char *(*ngx_http_geoip_variable_handler_v6_pt)(GeoIP *, geoipv6_t addr);
> +
> +/* Functions for IPv4 and IPv6 country or org lookup */
> +typedef struct {
> +    ngx_http_geoip_variable_handler_pt v4;
> +    ngx_http_geoip_variable_handler_v6_pt v6;
> +} ngx_http_geoip_lookup_function;
> +
> +ngx_http_geoip_lookup_function ngx_http_geoip_country_code_by_ipnum = {
> +    (ngx_http_geoip_variable_handler_pt) GeoIP_country_code_by_ipnum,
> +    (ngx_http_geoip_variable_handler_v6_pt) GeoIP_country_code_by_ipnum_v6
> +};
> +
> +ngx_http_geoip_lookup_function ngx_http_geoip_country_code3_by_ipnum = {
> +    (ngx_http_geoip_variable_handler_pt) GeoIP_country_code3_by_ipnum,
> +    (ngx_http_geoip_variable_handler_v6_pt) GeoIP_country_code3_by_ipnum_v6
> +};
> +
> +ngx_http_geoip_lookup_function ngx_http_geoip_country_name_by_ipnum = {
> +    (ngx_http_geoip_variable_handler_pt) GeoIP_country_name_by_ipnum,
> +    (ngx_http_geoip_variable_handler_v6_pt) GeoIP_country_name_by_ipnum_v6
> +};
> +
> +ngx_http_geoip_lookup_function ngx_http_geoip_name_by_ipnum = {
> +    (ngx_http_geoip_variable_handler_pt) GeoIP_name_by_ipnum,
> +    (ngx_http_geoip_variable_handler_v6_pt) GeoIP_name_by_ipnum_v6
> +};
>  
>  static ngx_int_t ngx_http_geoip_country_variable(ngx_http_request_t *r,
>      ngx_http_variable_value_t *v, uintptr_t data);
> @@ -114,19 +154,19 @@
>  
>      { ngx_string("geoip_country_code"), NULL,
>        ngx_http_geoip_country_variable,
> -      (uintptr_t) GeoIP_country_code_by_ipnum, 0, 0 },
> +      (uintptr_t) &ngx_http_geoip_country_code_by_ipnum, 0, 0 },
>  
>      { ngx_string("geoip_country_code3"), NULL,
>        ngx_http_geoip_country_variable,
> -      (uintptr_t) GeoIP_country_code3_by_ipnum, 0, 0 },
> +      (uintptr_t) &ngx_http_geoip_country_code3_by_ipnum, 0, 0 },
>  
>      { ngx_string("geoip_country_name"), NULL,
>        ngx_http_geoip_country_variable,
> -      (uintptr_t) GeoIP_country_name_by_ipnum, 0, 0 },
> +      (uintptr_t) &ngx_http_geoip_country_name_by_ipnum, 0, 0 },
>  
>      { ngx_string("geoip_org"), NULL,
>        ngx_http_geoip_org_variable,
> -      (uintptr_t) GeoIP_name_by_ipnum, 0, 0 },
> +      (uintptr_t) &ngx_http_geoip_name_by_ipnum, 0, 0 },

Still looks overcomplicated for me.  Probably indexed arrays with 
pointers would be better, something like

uintptr_t ngx_http_geoip_country[] = {
    GeoIP_country_code_by_ipnum,
    GeoIP_country_code3_by_ipnum,
    GeoIP_country_name_by_ipnum,
};

#if (NGX_HAVE_GEOIP_IPV6)

uintptr_t ngx_http_geoip_country_ipv6[] = {
    GeoIP_country_code_by_ipnum_v6,
    GeoIP_country_code3_by_ipnum_v6,
    GeoIP_country_name_by_ipnum_v6,
};

#endif

and passing index as variable data.

In case of ngx_http_geoip_org_variable() it should probably just 
go into function itself.
    
>  
>      { ngx_string("geoip_city_continent_code"), NULL,
>        ngx_http_geoip_city_variable,
> @@ -217,13 +257,56 @@
>      return INADDR_NONE;
>  }
>  
> +static geoipv6_t
> +ngx_http_geoip_addr_v6(ngx_http_request_t *r)
> +{
> +#if (NGX_HAVE_GEOIPV6)
> +    struct sockaddr_in6  *sin;

You can't assume you have sockaddr_in6 unless compiled with ipv6.

> +    /* For IPv4 to IPv6 mapping */
> +    struct sockaddr_in   *sin4;
> +    struct in6_addr      v6address;
> +    u_long               raw_v4address;

Just "addr" should be fine here.  And please note that variable 
names should be aligned, not including "*".

> +    u_int8_t             i;
> +
> +    switch (r->connection->sockaddr->sa_family) {
> +
> +    case AF_INET:
> +        sin4 = (struct sockaddr_in *) r->connection->sockaddr;
> +        raw_v4address = ntohl(sin4->sin_addr.s_addr);
> +        /* Clear first 8 bytes */
> +        memset(&v6address, 0, 10);
> +        /* Set 2 bytes from 80th bit onward */
> +        memset(v6address.s6_addr + 10, 0xFF, 2);
> +        /* Fill up 4 last bytes with IPv4 address */
> +        for (i = 0; i < 4; i++) {
> +            /* We have 16 bytes in IPv6 address */
> +            v6address.s6_addr[15 - i] = raw_v4address;
> +            /* Shift for 8 bits */
> +            raw_v4address >>= 8;
> +        }

Just byte setting should be fine here.  See no reason to use 
memset() and cycles.

> +
> +        return v6address;
> +
> +    case AF_INET6:
> +        sin = (struct sockaddr_in6 *) r->connection->sockaddr;
> +
> +        return sin->sin6_addr;
> +
> +    }
> +
> +    return in6addr_any;
> +#else
> +    /* Non IPv6 GeoIP - redirect to IPv4 version */
> +    return ngx_http_geoip_addr(r);

How this may happen and what's the expected result?

> +#endif
> +}
>  
>  static ngx_int_t
>  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;
> +    ngx_http_geoip_lookup_function *handler =
> +        (ngx_http_geoip_lookup_function*) data;
>  
>      const char             *val;
>      ngx_http_geoip_conf_t  *gcf;
> @@ -234,7 +317,11 @@
>          goto not_found;
>      }
>  
> -    val = handler(gcf->country, ngx_http_geoip_addr(r));
> +    if (gcf->is_country_ipv6) {
> +        val = handler->v6(gcf->country, ngx_http_geoip_addr_v6(r));
> +    } else {
> +        val = handler->v4(gcf->country, ngx_http_geoip_addr(r));
> +    }
>  
>      if (val == NULL) {
>          goto not_found;
> @@ -260,8 +347,8 @@
>  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;
> +    ngx_http_geoip_lookup_function *handler =
> +        (ngx_http_geoip_lookup_function*) data;
>  
>      const char             *val;
>      ngx_http_geoip_conf_t  *gcf;
> @@ -272,7 +359,11 @@
>          goto not_found;
>      }
>  
> -    val = handler(gcf->org, ngx_http_geoip_addr(r));
> +    if (gcf->is_org_ipv6) {
> +        val = handler->v6(gcf->org, ngx_http_geoip_addr_v6(r));
> +    } else {
> +        val = handler->v4(gcf->org, ngx_http_geoip_addr(r));
> +    }
>  
>      if (val == NULL) {
>          goto not_found;
> @@ -452,7 +543,11 @@
>      gcf = ngx_http_get_module_main_conf(r, ngx_http_geoip_module);
>  
>      if (gcf->city) {
> -        return GeoIP_record_by_ipnum(gcf->city, ngx_http_geoip_addr(r));
> +        if (gcf->is_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));
> +        }
>      }
>  
>      return NULL;
> @@ -540,8 +635,16 @@
>      case GEOIP_PROXY_EDITION:
>      case GEOIP_NETSPEED_EDITION:
>  
> +        gcf->is_country_ipv6 = 0;
>          return NGX_CONF_OK;
>  
> +#if (NGX_HAVE_GEOIPV6)
> +    case GEOIP_COUNTRY_EDITION_V6:
> +
> +        gcf->is_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 +694,18 @@
>      case GEOIP_DOMAIN_EDITION:
>      case GEOIP_ASNUM_EDITION:
>  
> +        gcf->is_org_ipv6 = 0;
> +        return NGX_CONF_OK;
> +
> +#if (NGX_HAVE_GEOIPV6)
> +    case GEOIP_ISP_EDITION_V6:
> +    case GEOIP_ORG_EDITION_V6:
> +    case GEOIP_DOMAIN_EDITION_V6:
> +    case GEOIP_ASNUM_EDITION_V6:
> +
> +        gcf->is_org_ipv6 = 1;
>          return NGX_CONF_OK;
> +#endif
>  
>      default:
>          ngx_conf_log_error(NGX_LOG_EMERG, cf, 0,
> @@ -640,8 +754,17 @@
>      case GEOIP_CITY_EDITION_REV0:
>      case GEOIP_CITY_EDITION_REV1:
>  
> +        gcf->is_city_ipv6 = 0;
>          return NGX_CONF_OK;
>  
> +#if (NGX_HAVE_GEOIPV6)
> +    case GEOIP_CITY_EDITION_REV0_V6:
> +    case GEOIP_CITY_EDITION_REV1_V6:
> +
> +        gcf->is_city_ipv6 = 1;
> +        return NGX_CONF_OK;
> +#endif
> +
>      default:
>          ngx_conf_log_error(NGX_LOG_EMERG, cf, 0,
>                             "invalid GeoIP City database \"%V\" type:%d",

Maxim Dounin



More information about the nginx-devel mailing list