Hello!

On Wed, Mar 02, 2011 at 12:55:53AM -0800, Speed First wrote:

Just a side note: you may want to avoid starting new thread with
each new message. That is, try hitting "reply" button.

[...]

> 4. seems there is a bug in old file's line 829. Only when uri is NULL, len =
> last - port.

You are wrong assuming anybody remember line numbers (and this is
why asked for "-p" switch to diff, btw).

And yes, it's bug (and was recently discussed here). Consider
making fix a separate patch.

> 5. now "ngx_inet6_parse_url" will finally invoke new added
> "ngx_inet6_resolve_host", just like what "ngx_inet_parse_url" does. I think
> it should be done but the old code doesn't do that. Please confirm this is
> valid.

Resolve shouldn't be needed here as ngx_inet6_parse_url() is only
invoked for ipv6-literals. That is, only parse is required, not
resolve.

> 6. I try to remove duplicated code as much as possible, so the
> "ngx_inet_resolve_host" is almost rewritten. Now, it will first handle the
> u->host as it's an IP; if not, it will try to resolve them.
>
> 7. In "ngx_inet_parse_url", it firstly assume it's AF_INET and allocate the
> memory. I think it's not suitable in the new implementation, but I didn't
> touch this part of code.
>
> 8. All the fix are tested as before (with and without "--with-ipv6"), all
> test are passed.
>
> 9. I make this implementation based on 0.9.3. I compare the ngx_inet.c in
> official 0.9.3 and 0.9.5, and find there is a difference:
>
> - for (i = 0; i < u->naddrs; i++) {
> + for (i = 0; h->h_addr_list[i] != NULL; i++) {
>
> Since my implementation use "getaddrinfo", h_addr_list[i] will be invalid
> when "--with-ipv6" is set.

In 0.9.5 code is "for (i = 0; i < u->naddrs; i++) {", which is
correct. You may want to merge with 0.9.5 anyway though.

Some generic note: You may consider splitting your patch into
series of smaller ones to make them easier for review.

Some notes are below (mostly random ones, as I have no time for
more detailed review).

>
> Thanks.

> --- nginx-0.9.3/src/core/ngx_inet.c 2009-12-07 07:13:46.000000000 -0800
> +++ /home/speedfirst/p4/zimbra/main/ThirdParty/nginx/nginx-0.9-zimbra/src/core/ngx_inet.c 2011-03-02 00:22:29.958260165 -0800
> @@ -11,6 +11,8 @@
> static ngx_int_t ngx_parse_unix_domain_url(ngx_pool_t *pool, ngx_url_t *u);
> static ngx_int_t ngx_parse_inet_url(ngx_pool_t *pool, ngx_url_t *u);
> static ngx_int_t ngx_parse_inet6_url(ngx_pool_t *pool, ngx_url_t *u);
> +static ngx_int_t ngx_resolve_hostname(ngx_pool_t *pool, ngx_url_t *u);
> +static ngx_int_t ngx_inet6_resolve_host(ngx_pool_t *pool, ngx_url_t *u);
>
>
> in_addr_t
> @@ -612,10 +614,9 @@
> static ngx_int_t
> ngx_parse_inet_url(ngx_pool_t *pool, ngx_url_t *u)
> {
> - u_char *p, *host, *port, *last, *uri, *args;
> + u_char *host, *port, *last, *uri, *args;
> size_t len;
> ngx_int_t n;
> - struct hostent *h;
> struct sockaddr_in *sin;
>
> u->socklen = sizeof(struct sockaddr_in);
> @@ -677,7 +678,6 @@
> }
>
> u->port = (in_port_t) n;
> - sin->sin_port = htons((in_port_t) n);
>
> u->port_text.len = len;
> u->port_text.data = port;
> @@ -734,27 +734,15 @@
> return NGX_OK;
> }
>
> + if (u->no_port) {
> + u->port = u->default_port;
> + }
> +
> if (len) {
> sin->sin_addr.s_addr = ngx_inet_addr(host, len);
>
> if (sin->sin_addr.s_addr == INADDR_NONE) {
> - p = ngx_alloc(++len, pool->log);
> - if (p == NULL) {
> - return NGX_ERROR;
> - }
> -
> - (void) ngx_cpystrn(p, host, len);
> -
> - h = gethostbyname((const char *) p);
> -
> - ngx_free(p);
> -
> - if (h == NULL || h->h_addr_list[0] == NULL) {
> - u->err = "host not found";
> - return NGX_ERROR;
> - }
> -
> - sin->sin_addr.s_addr = *(in_addr_t *) (h->h_addr_list[0]);
> + return ngx_resolve_hostname(pool, u);
> }
>
> if (sin->sin_addr.s_addr == INADDR_ANY) {
> @@ -766,10 +754,7 @@
> u->wildcard = 1;
> }
>
> - if (u->no_port) {
> - u->port = u->default_port;
> - sin->sin_port = htons(u->default_port);
> - }
> + sin->sin_port = htons(u->port);

This change is completely unrelated and not really needed. You
may want to avoid cluttering patch with such changes.

>
> if (u->listen) {
> return NGX_OK;
> @@ -782,6 +767,111 @@
> return NGX_OK;
> }
>
> +static ngx_int_t
> +ngx_resolve_hostname(ngx_pool_t *pool, ngx_url_t *u) {
> + u_char *p;
> + ngx_uint_t family;
> + in_addr_t in_addr;
> + struct sockaddr_in *sin;
> +
> +#if (NGX_HAVE_INET6)
> + struct addrinfo hints, *addrinfo;
> + struct in6_addr in6_addr;
> + struct sockaddr_in6 *sin6;
> + int n;
> +#else
> + struct hostent *h;
> +#endif
> +
> + /* resolve the IP address through host name.
> + * only the first IP address will be used. */

Style problem here.

> + p = ngx_alloc(u->host.len + 1, pool->log);
> +
> + if (p == NULL) {
> + return NGX_ERROR;
> + }
> +
> + ngx_cpystrn(p, u->host.data, u->host.len + 1);
> +
> +#if (NGX_HAVE_INET6)
> +
> + ngx_memzero (&hints, sizeof (struct addrinfo));

Style problem here.

> +
> + if (u->listen) {
> + hints.ai_flags = AI_PASSIVE;
> + } else {
> + hints.ai_flags = AI_CANONNAME;
> + }
> +
> + hints.ai_protocol = IPPROTO_TCP;
> +
> + n = getaddrinfo((const char *) p,
> + NULL, &hints, &addrinfo);
> +
> + if (n != NGX_OK) {
> + u->err = "host not found";
> + return NGX_ERROR;
> + }
> +
> + if (addrinfo->ai_family == AF_INET) {
> + family = AF_INET;
> + in_addr = ((struct sockaddr_in *) addrinfo->ai_addr)->sin_addr.s_addr;
> +
> + } else { /* AF_INET6 */
> + family = AF_INET6;
> + in6_addr = ((struct sockaddr_in6 *) addrinfo->ai_addr)->sin6_addr;
> +
> + }
> +#else
> + h = gethostbyname((const char *) p);
> +
> + if (h == NULL || h->h_addr_list[0] == NULL) {
> + u->err = "host not found";
> + return NGX_ERROR;
> + }
> +
> + in_addr = *(in_addr_t *) (h->h_addr_list[0]);
> +#endif
> +
> + ngx_free(p);
> +
> + switch (family) {
> +
> +#if (NGX_HAVE_INET6)
> + case AF_INET6:
> + sin6 = (struct sockaddr_in6 *) u->sockaddr;
> + sin6->sin6_family = AF_INET6;
> + sin6->sin6_port = htons(u->port);
> + u->family = AF_INET6;
> + u->socklen = sizeof (struct sockaddr_in6);
> + ngx_memcpy(sin6->sin6_addr.s6_addr, in6_addr.s6_addr, 16);
> +
> + if (IN6_IS_ADDR_UNSPECIFIED(&sin6->sin6_addr)) {
> + u->wildcard = 1;
> + }
> + break;
> +#endif
> +
> + default: /* AF_INET */
> + sin = (struct sockaddr_in *) u->sockaddr;
> + sin->sin_family = AF_INET;
> + sin->sin_port = htons(u->port);
> + u->family = AF_INET;
> + u->socklen = sizeof (struct sockaddr_in);
> + sin->sin_addr.s_addr = in_addr;
> + if (sin->sin_addr.s_addr == INADDR_ANY) {
> + u->wildcard = 1;
> + }
> + break;
> + }
> +
> + if (u->listen) {
> + return NGX_OK;
> + }
> +
> + return ngx_inet_resolve_host(pool, u);

It's not clear why you are trying to do two different "resolve" in
a raw, each of them calling gethostbyname/getaddrinfo. Looks silly.

> +}
> +
>
> static ngx_int_t
> ngx_parse_inet6_url(ngx_pool_t *pool, ngx_url_t *u)
> @@ -826,7 +916,11 @@
> if (*port == ':') {
> port++;
>
> - len = last - port;
> + if (uri) {
> + len = uri - port;
> + } else {
> + len = last - port;
> + }
>
> if (len == 0) {
> u->err = "invalid port";

This hunk is good one. Please submit as a separate patch.

> @@ -881,7 +975,11 @@
> sin6->sin6_port = htons(u->default_port);
> }
>
> - return NGX_OK;
> + if (u->listen) {
> + return NGX_OK;
> + }
> +
> + return ngx_inet6_resolve_host(pool, u);

As already noted above - shouldn't be needed.

>
> #else
>
> @@ -901,107 +999,245 @@
> in_port_t port;
> in_addr_t in_addr;
> ngx_uint_t i;
> - struct hostent *h;
> struct sockaddr_in *sin;
> + struct sockaddr *sa;
>
> - /* AF_INET only */
> +#if (NGX_HAVE_INET6)
> + int family = -1;
> + struct addrinfo hints, *addrinfo, *item;
> + struct in6_addr in6_addr;
> + struct sockaddr_in6 *sin6;
> + int n;
> +
> +#else
> + struct hostent *h;
> +#endif
> +
> + if (*u->host.data == '[') {
> + return ngx_inet6_resolve_host(pool, u);
> + }
>
> port = htons(u->port);
>
> in_addr = ngx_inet_addr(u->host.data, u->host.len);
>
> - if (in_addr == INADDR_NONE) {
> - host = ngx_alloc(u->host.len + 1, pool->log);
> - if (host == NULL) {
> - return NGX_ERROR;
> + if (in_addr != INADDR_NONE) {
> +
> + u->addrs = ngx_pcalloc(pool, sizeof(ngx_addr_t));
> + if (u->addrs == NULL) {
> + return NGX_ERROR;
> }
>
> - (void) ngx_cpystrn(host, u->host.data, u->host.len + 1);
> + u->naddrs = 1;
>
> - h = gethostbyname((char *) host);
> + sin = ngx_pcalloc(pool, sizeof(struct sockaddr_in));
> + if (sin == NULL) {
> + return NGX_ERROR;
> + }
> + sin->sin_family = AF_INET;
> + sin->sin_port = port;
> + sin->sin_addr.s_addr = in_addr;
>
> - ngx_free(host);
> + u->addrs[0].sockaddr = (struct sockaddr *) sin;
> + u->addrs[0].socklen = sizeof(struct sockaddr_in);
>
> - if (h == NULL || h->h_addr_list[0] == NULL) {
> - u->err = "host not found";
> + p = ngx_pnalloc(pool, u->host.len + sizeof(":65535") - 1);
> + if (p == NULL) {
> return NGX_ERROR;
> }
>
> - if (u->one_addr == 0) {
> - for (i = 0; h->h_addr_list[i] != NULL; i++) { /* void */ }
> + u->addrs[0].name.len = ngx_sprintf(p, "%V:%d",
> + &u->host, ntohs(port)) - p;
> + u->addrs[0].name.data = p;
>
> - } else {
> - i = 1;
> - }
> + return NGX_OK;
> + }
> +
> + /* need to resolve host name */
> + host = ngx_alloc(u->host.len + 1, pool->log);
> + if (host == NULL) {
> + return NGX_ERROR;
> + }
>
> - /* MP: ngx_shared_palloc() */
> + (void) ngx_cpystrn(host, u->host.data, u->host.len + 1);
>
> - u->addrs = ngx_pcalloc(pool, i * sizeof(ngx_addr_t));
> - if (u->addrs == NULL) {
> - return NGX_ERROR;
> - }
> +#if (NGX_HAVE_INET6)
>
> - u->naddrs = i;
> + ngx_memzero (&hints, sizeof (struct addrinfo));
>
> - for (i = 0; h->h_addr_list[i] != NULL; i++) {
> + /* if the address is for listen, it won't enter this reslove function */
> + hints.ai_flags = AI_CANONNAME;
> + hints.ai_protocol = IPPROTO_TCP;
>
> - sin = ngx_pcalloc(pool, sizeof(struct sockaddr_in));
> - if (sin == NULL) {
> - return NGX_ERROR;
> - }
> + n = getaddrinfo((const char *) host,
> + NULL, &hints, &addrinfo);
>
> - sin->sin_family = AF_INET;
> - sin->sin_port = port;
> - sin->sin_addr.s_addr = *(in_addr_t *) (h->h_addr_list[i]);
> + if (n != NGX_OK) {
> + u->err = "host not found";
> + return NGX_ERROR;
> + }
>
> - u->addrs[i].sockaddr = (struct sockaddr *) sin;
> - u->addrs[i].socklen = sizeof(struct sockaddr_in);
> + if (u->one_addr == 0) {
> + item = addrinfo;
> + for (i = 0; item != NULL; i++, item = item->ai_next) { /* void */ }
>
> - len = NGX_INET_ADDRSTRLEN + sizeof(":65535") - 1;
> + } else {
> + i = 1;
> + }
>
> - p = ngx_pnalloc(pool, len);
> - if (p == NULL) {
> - return NGX_ERROR;
> - }
> +#else
>
> - len = ngx_sock_ntop((struct sockaddr *) sin, p, len, 1);
> + h = gethostbyname((char *) host);
>
> - u->addrs[i].name.len = len;
> - u->addrs[i].name.data = p;
> - }
> + if (h == NULL || h->h_addr_list[0] == NULL) {
> + u->err = "host not found";
> + return NGX_ERROR;
> + }
> +
> + if (u->one_addr == 0) {
> + for (i = 0; h->h_addr_list[i] != NULL; i++) { /* void */ }
>
> } else {
> + i = 1;
> + }
> +#endif
>
> - /* MP: ngx_shared_palloc() */
> + ngx_free (host);
>
> - u->addrs = ngx_pcalloc(pool, sizeof(ngx_addr_t));
> - if (u->addrs == NULL) {
> - return NGX_ERROR;
> + /* MP: ngx_shared_palloc() */
> +
> + u->addrs = ngx_pcalloc(pool, i * sizeof(ngx_addr_t));
> + if (u->addrs == NULL) {
> + return NGX_ERROR;
> + }
> +
> + u->naddrs = i;
> +
> + for (i = 0; i < u->naddrs; i++) {
> +
> +#if (NGX_HAVE_INET6)
> + if (addrinfo->ai_family == AF_INET) {
> + family = AF_INET;
> + sin = ngx_pcalloc(pool, sizeof(struct sockaddr_in));
> + if (sin == NULL) {
> + return NGX_ERROR;
> + }
> + in_addr = ((struct sockaddr_in *) addrinfo->ai_addr)->sin_addr.s_addr;
> + sin->sin_addr.s_addr = in_addr;
> + } else {
> + family = AF_INET6;
> + sin6 = ngx_pcalloc(pool, sizeof(struct sockaddr_in6));
> + if (sin6 == NULL) {
> + return NGX_ERROR;
> + }
> + in6_addr = ((struct sockaddr_in6 *) addrinfo->ai_addr)->sin6_addr;
> + ngx_memcpy(sin6->sin6_addr.s6_addr, in6_addr.s6_addr, 16);
> }
>
> + addrinfo = addrinfo->ai_next;
> +
> +#else
> sin = ngx_pcalloc(pool, sizeof(struct sockaddr_in));
> - if (sin == NULL) {
> + sin->sin_addr.s_addr = *(in_addr_t *) (h->h_addr_list[i]);
> +#endif
> +
> +#if (NGX_HAVE_INET6)
> + if (family == AF_INET6) {
> +
> + sin6->sin6_family = AF_INET6;
> + sin6->sin6_port = port;
> + sa = (struct sockaddr *)sin6;
> + u->addrs[i].sockaddr = sa;
> + u->addrs[i].socklen = sizeof(struct sockaddr_in6);
> + len = NGX_INET6_ADDRSTRLEN + sizeof(":65535") - 1;
> + }
> + else
> +#endif
> + {
> + sin->sin_family = AF_INET;
> + sin->sin_port = port;
> + sa = (struct sockaddr *)sin;
> + u->addrs[i].sockaddr = sa;
> + u->addrs[i].socklen = sizeof(struct sockaddr_in);
> + len = NGX_INET_ADDRSTRLEN + sizeof(":65535") - 1;
> + }
> +
> + p = ngx_pnalloc(pool, len);
> + if (p == NULL) {
> return NGX_ERROR;
> }
>
> - u->naddrs = 1;
> + len = ngx_sock_ntop(sa, p, len, port);
>
> - sin->sin_family = AF_INET;
> - sin->sin_port = port;
> - sin->sin_addr.s_addr = in_addr;
> + u->addrs[i].name.len = len;
> + u->addrs[i].name.data = p;
> + }
>
> - u->addrs[0].sockaddr = (struct sockaddr *) sin;
> - u->addrs[0].socklen = sizeof(struct sockaddr_in);
> + return NGX_OK;
> +}
>
> - p = ngx_pnalloc(pool, u->host.len + sizeof(":65535") - 1);
> - if (p == NULL) {
> +ngx_int_t
> +ngx_inet6_resolve_host(ngx_pool_t * pool, ngx_url_t * u)
> +{

"static" is missing here (on the other hand, the whole function
looks strange, see above)

> +#if (NGX_HAVE_INET6)
> + u_char *p;
> + struct in6_addr in6addr;
> + struct sockaddr_in6 *sin6;
> + ngx_str_t *host;
> +
> + host = &u->host;
> +
> + if (*(host->data) == '[') {
> + if(*(host->data + host->len - 1) == ']') {
> + host->data++;
> + host->len -= 2;
> + } else {
> + u->err = "invalid host";
> return NGX_ERROR;
> }
> + }
>
> - u->addrs[0].name.len = ngx_sprintf(p, "%V:%d",
> - &u->host, ntohs(port)) - p;
> - u->addrs[0].name.data = p;
> + if (ngx_inet6_addr(u->host.data,
> + u->host.len, (u_char *)&in6addr) == NGX_ERROR) {
> + u->err = "invalid host";
> + return NGX_ERROR;
> + }
> +
> + sin6 = ngx_pcalloc(pool, sizeof(struct sockaddr_in6));
> + if (sin6 == NULL) {
> + return NGX_ERROR;
> + }
> + sin6->sin6_family = AF_INET6;
> + sin6->sin6_port = htons(u->port);
> + ngx_memcpy(sin6->sin6_addr.s6_addr, in6addr.s6_addr, 16);
> +
> + u->addrs = ngx_pcalloc(pool, sizeof(ngx_addr_t));
> +
> + if(u->addrs == NULL) {
> + return NGX_ERROR;
> + }
> +
> + u->naddrs = 1;
> +
> + u->addrs[0].sockaddr = (struct sockaddr *) sin6;
> + u->addrs[0].socklen = sizeof(struct sockaddr_in6);
> +
> + p = ngx_pnalloc(pool, u->host.len + sizeof(":65535") - 1);
> + if (p == NULL) {
> + return NGX_ERROR;
> }
>
> + u->addrs[0].name.len = ngx_sprintf(p, "[%V]:%d",
> + &u->host, u->port) - p;
> + u->addrs[0].name.data = p;
> +
> return NGX_OK;
> +
> +#else
> +
> + u->err = "the INET6 sockets are not supported on this platform";
> +
> + return NGX_ERROR;
> +
> +#endif
> }

Maxim Dounin

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