Hello!
On Fri, Oct 23, 2009 at 03:12:33PM +0200, Hongli Lai wrote:
> Hi.
>
> The attached patch allows Nginx to listen on Unix domain sockets, like
> this:
>
> server {
> listen unix:/tmp/nginx.sock;
> server_name foobar.com;
> root /webapps/foobar;
> }
>
> We use it to improve proxying performance; Unix domain sockets are
> faster than TCP sockets.
>
> I'd like to see this patch getting merged upstream. Could a maintainer
> please review this patch?
I'm not really maintainer, but here is the review.
[...]
> diff --git a/src/core/ngx_inet.c b/src/core/ngx_inet.c
> index 4c18036..59998be 100644
> --- a/src/core/ngx_inet.c
> +++ b/src/core/ngx_inet.c
> @@ -68,7 +68,9 @@ ngx_sock_ntop(struct sockaddr *sa, u_char *text, size_t len, ngx_uint_t port)
> size_t n;
> struct sockaddr_in6 *sin6;
> #endif
> -
> + struct sockaddr_un *sun;
This should be protected by #if (NGX_HAVE_UNIX_DOMAIN).
> + size_t path_len;
Using 'plen' here is just enough to be understood. And actually
it may be a good idea to use just one 'n' shared with INET6 case.
> +
Whitespace damage here.
> switch (sa->sa_family) {
>
> case AF_INET:
> @@ -108,6 +110,17 @@ ngx_sock_ntop(struct sockaddr *sa, u_char *text, size_t len, ngx_uint_t port)
> return n;
> #endif
>
> + case AF_UNIX:
> +
> + sun = (struct sockaddr_un *) sa;
This should be protected by #if (NGX_HAVE_UNIX_DOMAIN).
> + path_len = strlen(sun->sun_path);
> + if (path_len == 0) {
> + return ngx_snprintf(text, len, "(unknown)") - text;
> + } else {
> + ngx_copy(text, (const u_char *) sun->sun_path, path_len);
Using const is wrong here, as it's not in prototype at least in
some cases.
And it looks like you just trashed memory if buffer pointed by
text isn't big enough, no?
Note well that buffers used in the code for ngx_sock_ntop() result
are sized against NGX_SOCKADDR_STRLEN, which is big enough to hold
ipv6 address, but may be smaller than needed for unix sockets.
> + return path_len;
> + }
> +
> default:
> return 0;
> }
> diff --git a/src/http/ngx_http_request.c b/src/http/ngx_http_request.c
> index 4930b50..5e3493e 100644
> --- a/src/http/ngx_http_request.c
> +++ b/src/http/ngx_http_request.c
> @@ -2426,7 +2426,11 @@ ngx_http_set_keepalive(ngx_http_request_t *r)
>
> if (tcp_nodelay
> && clcf->tcp_nodelay
> - && c->tcp_nodelay == NGX_TCP_NODELAY_UNSET)
> + && c->tcp_nodelay == NGX_TCP_NODELAY_UNSET
> + #if (NGX_HAVE_UNIX_DOMAIN)
> + && c->sockaddr->sa_family != AF_UNIX
> + #endif
Please keep preprocessor directive starting at position 0.
And note that ngx_tcp_nopush() / ngx_tcp_push() invocations
probably should be protected too.
Maxim Dounin
