[PATCH] parse URL to ipv6

[Maxim Dounin]

Hello!

On Tue, Mar 01, 2011 at 08:22:59PM +0800, Speed First wrote:

> Two files are affected "ngx_inet.c" and "ngx_inet.h". I remove the original
> "ngx_parse_inet_url" and "ngx_parse_inet6_url" because they have many
> duplicated code and "ngx_pare_inet_url" this name won't describe what it
> does comprehensively (it may generate IPv6 too).
> 
> So I use "ngx_parse_url" to split the url to "host", "port", "uri" and
> create a function "ngx_parse_host" to convert host to IP address. Besides, I
> also change "ngx_inet_resolve_host" to make it accept IPv6.
> 
> At last I add a function "ngx_inet_sock_addr" to convert "ipv4:port" and
> "[ipv6]:port" to sockaddr_in and sockaddr_in6.
> 
> The following test has been done to verify the functionality of url parse
> (Here ip = ipv4 and ipv6).
> 
> 1. only port   ==> only accept IPv4
> 2. *:port ==> same as 1
> 3. [::]:port ==> accept both ipv4 and ipv6 if not set ipv6only=on
> 4. ip:port
> 5. ip:port/uri
> 6. ip:port/uri?arg
> 7: ip/uri?arg
> 8. text:port   (text url can be resolved to 1 IPv6 addr)
> 9. text:port   (text url can be resolved to many addr, some are IPv4 and
> some IPv6. The url->sockaddr always be the first IP address)

Some comments in no particular order:

1. Patch is damaged.  If unsure how to configure your mail client 
   to not damage patches - please use attachments instead.

2. There are multiple style issues and unrelated changes, you may 
   want to cleanup them.

3. Result looks inconsistent: it uses separate function to 
   parse unix domain urls, and joined one to parse ipv4/ipv6.
   There should be clear point where address family separation is 
   done.

4. Allowing bare IPv6 addresses without [] is bad idea, you can't 
   distingush ipv6 address without port "[::1:80]" from one with
   port "[::1]:80" if written without [], i.e. as "::1:80".  It's 
   good idea to follow rfc3986 instead.

5. There are systems without getaddrinfo() or with broken one.  It 
   should be only used when compiled with ipv6.

6. Introduction of ngx_inet_sock_addr() is clearly separate and at 
   least should be kept as separate patch.  It is not clear why 
   you ever need it though, using ngx_parse_url() with correct 
   flags should do the trick.

Some more comments inline.

[...]

> @@ -446,7 +445,6 @@
>      }
>  }
> 
> -
>  ngx_int_t
>  ngx_parse_addr(ngx_pool_t *pool, ngx_addr_t *addr, u_char *text, size_t

Unrelated change (and style violation).  Same applies to other 
places as well.

[...]

> +#if (NGX_HAVE_INET6)
> +    if (host[0] == '[') {

As a result of the changes it no longer returns error "the INET6 
sockets are not supported on this platform" when ipv6 isn't 
compiled in, but produces some obscure error instead.

[...]

>      if (uri) {
>          if (u->listen || !u->uri_part) {
> -            u->err = "invalid host";
> +            u->err = "invalid url to listen";

This is certainly incorrect message change.  Not only listen 
sockets may have "u->uri_part" not set.

[...]

> +ngx_int_t
> +ngx_inet_sock_addr (u_char * p, size_t len, struct sockaddr * sockaddr)
> +{

Style: there should be no space between function name and "(".  
Same applies to other places as well.

[...]

Maxim Dounin