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
_______________________________________________
nginx-devel mailing list
nginx-devel@nginx.org
http://nginx.org/mailman/listinfo/nginx-devel
