Hello!

On Wed, Jun 15, 2011 at 12:43:49PM +0200, Gregor Kališnik wrote:

> Hi.
>
> On Wednesday 15 of June 2011 03:47:18 you wrote:
> > Hello!
> >
> > On Tue, Jun 14, 2011 at 07:52:10AM +0200, Gregor Kališnik wrote:
> > > Hi.
> > >
>
> >
> > 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;"
> >
>
> I've checked GeoIP.h from geoip-1.4.6 and geoip-1.4.7. Both have geoipv6_t.
> But the 1.4.6 version doesn't support GeoIP City IPv6 version and check for
> geoipv6_t would break compilation when using geoip-1.4.6.

Ah, ok, I see now. IPv6 support appeared in libgeoip 1.4.5, but
it was rather limited. So the test for (at least one) constant
appeared in 1.4.7 is indeed required.

> Should I break GeoIP IPv6 code into two parts? First checking for geoipv6_t
> and second checking for GeoCity related define/type?

No, I think this would be an overkill.

> > 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.
> >
>
> Hope I got the identitation right :) .

I actually mean "?" to be 4 spaces after "gcf" in previous line.
Much like in

core/ngx_open_file_cache.c:797: p = (ngx_strcmp(file->name, file_temp->name) < 0)
core/ngx_open_file_cache.c:798: ? &temp->left : &temp->right;

>
>
> 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-15 12:22:34.085253223 +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);'
> + . auto/feature

Please include attached patch for configure part (on top of
yours).

It cleans up some minor things in original code (missing
ngx_feature_incs, extra trailing "/" in ngx_feature_path for
NetBSD port, missing ngx_feature_path for FreeBSD port, missing
CORE_INCS modification) and modifies your code to not set
"ngx_feature_path" and "ngx_feature_libs". This allows correct
detection of ipv6 support if library was found on non-default
path.

> + 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-15 12:28:44.455416386 +0200
> @@ -12,10 +12,18 @@
> #include <GeoIPCity.h>
>
>
> +#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);
>
> +
> +ngx_http_geoip_variable_handler_pt ngx_http_geoip_country_functions[] = {
> + (ngx_http_geoip_variable_handler_pt) GeoIP_country_code_by_ipnum,
> + (ngx_http_geoip_variable_handler_pt) GeoIP_country_code3_by_ipnum,
> + (ngx_http_geoip_variable_handler_pt) GeoIP_country_name_by_ipnum,

I believe cast shouldn't be needed here.

> +};
> +
> +
> +#if (NGX_HAVE_GEOIP_IPV6)
> +typedef const char *(*ngx_http_geoip_variable_handler_v6_pt)(GeoIP *, geoipv6_t addr);
> +
> +
> +ngx_http_geoip_variable_handler_v6_pt ngx_http_geoip_country_ipv6_functions[] = {
> + (ngx_http_geoip_variable_handler_v6_pt) GeoIP_country_code_by_ipnum_v6,
> + (ngx_http_geoip_variable_handler_v6_pt) GeoIP_country_code3_by_ipnum_v6,
> + (ngx_http_geoip_variable_handler_v6_pt) GeoIP_country_name_by_ipnum_v6,

And here.

> +};
> +#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,55 @@
> return INADDR_NONE;
> }
>

Two empty lines between functions, please.

> +#if (NGX_HAVE_GEOIP_IPV6)
> +static geoipv6_t
> +ngx_http_geoip_addr_v6(ngx_http_request_t *r)
> +{
> + struct sockaddr_in6 *sin6;
> + struct sockaddr_in *sin;
> + struct in6_addr addr6;
> + u_long addr;
> +
> + switch (r->connection->sockaddr->sa_family) {
> +
> + case AF_INET:
> + /* Produce IPv4 mapped IPv6 address */
> + sin = (struct sockaddr_in *) r->connection->sockaddr;
> + addr = ntohl(sin->sin_addr.s_addr);
> +
> + ngx_memzero(&addr6, sizeof(struct in6_addr));
> + addr6.s6_addr[10] = 0xFF;
> + addr6.s6_addr[11] = 0xFF;
> + addr6.s6_addr[12] = addr >> 24;
> + addr6.s6_addr[13] = addr >> 16;
> + addr6.s6_addr[14] = addr >> 8;
> + addr6.s6_addr[15] = addr;
> + return addr6;
> +
> + case AF_INET6:
> + sin6 = (struct sockaddr_in6 *) r->connection->sockaddr;
> + return sin6->sin6_addr;
> +
> + }
> +
> + return in6addr_any;
> +}
> +#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;
> +#if (NGX_HAVE_GEOIP_IPV6)
> + ngx_http_geoip_variable_handler_v6_pt handler_v6 =
> + ngx_http_geoip_country_ipv6_functions[data];
> +#endif
> + ngx_http_geoip_variable_handler_pt handler =
> + 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 +301,13 @@
> goto not_found;
> }
>
> +#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
>
> if (val == NULL) {
> goto not_found;
> @@ -260,9 +333,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 +342,13 @@
> goto not_found;
> }
>
> - val = handler(gcf->org, ngx_http_geoip_addr(r));
> +#if (NGX_HAVE_GEOIP_IPV6)
> + val = gcf->country_ipv6
> + ? GeoIP_name_by_ipnum_v6(gcf->country, ngx_http_geoip_addr_v6(r))
> + : GeoIP_name_by_ipnum(gcf->country, 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 +528,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

The same form here.

> }
>
> return NULL;
> @@ -540,8 +624,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,8 +683,19 @@
> 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,
> "invalid GeoIP database \"%V\" type:%d",
> @@ -640,7 +743,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,

Overall, looks good for me except some minor style nits.

Maxim Dounin

_______________________________________________
nginx-devel mailing list
nginx-devel@nginx.org
http://nginx.org/mailman/listinfo/nginx-devel