[PATCH] Use ngx_socket_errno where appropriate.

[Ruslan Ermilov]

Hi Piotr,

On Thu, Jan 30, 2014 at 04:16:04PM -0800, Piotr Sikora wrote:
> # HG changeset patch
> # User Piotr Sikora <piotr at cloudflare.com>
> # Date 1391126566 28800
> #      Thu Jan 30 16:02:46 2014 -0800
> # Node ID fe4a9b7ca5985b0bde46fa78bd96de80e03d988b
> # Parent  2e40188f83ef5bf1ae5afe0dd445689049f46a5c
> Use ngx_socket_errno where appropriate.
> 
> Signed-off-by: Piotr Sikora <piotr at cloudflare.com>
> 
> diff -r 2e40188f83ef -r fe4a9b7ca598 src/core/ngx_connection.c
> --- a/src/core/ngx_connection.c Thu Jan 30 19:13:12 2014 +0400
> +++ b/src/core/ngx_connection.c Thu Jan 30 16:02:46 2014 -0800
[...]
> @@ -272,9 +272,9 @@ ngx_set_inherited_sockets(ngx_cycle_t *c
> 
>          if (getsockopt(ls[i].fd, IPPROTO_TCP, TCP_DEFER_ACCEPT,
> &timeout, &olen)

Your MUA seems to be wrapping lines and thus patch is broken.

> diff -r 2e40188f83ef -r fe4a9b7ca598 src/event/modules/ngx_epoll_module.c
> --- a/src/event/modules/ngx_epoll_module.c Thu Jan 30 19:13:12 2014 +0400
> +++ b/src/event/modules/ngx_epoll_module.c Thu Jan 30 16:02:46 2014 -0800
> @@ -241,9 +241,9 @@ ngx_epoll_aio_init(ngx_cycle_t *cycle, n
> 
>      n = 1;
> 
>      if (ioctl(ngx_eventfd, FIONBIO, &n) == -1) {
> -        ngx_log_error(NGX_LOG_EMERG, cycle->log, ngx_errno,
> +        ngx_log_error(NGX_LOG_EMERG, cycle->log, ngx_socket_errno,
>                        "ioctl(eventfd, FIONBIO) failed");
>          goto failed;
>      }

While I agree that using ngx_socket_errno for ngx_nonblocking()
which is implemented through ioctl() on UNIX-likes is a right
thing to do, this doesn't look right here, where all of the
rest of this code manipulating with ngx_eventfd uses ngx_errno.

> diff -r 2e40188f83ef -r fe4a9b7ca598 src/os/unix/ngx_process.c
> --- a/src/os/unix/ngx_process.c Thu Jan 30 19:13:12 2014 +0400
> +++ b/src/os/unix/ngx_process.c Thu Jan 30 16:02:46 2014 -0800
[...]
>          on = 1;
>          if (ioctl(ngx_processes[s].channel[0], FIOASYNC, &on) == -1) {
> -            ngx_log_error(NGX_LOG_ALERT, cycle->log, ngx_errno,
> +            ngx_log_error(NGX_LOG_ALERT, cycle->log, ngx_socket_errno,
>                            "ioctl(FIOASYNC) failed while spawning
> \"%s\"", name);
>              ngx_close_channel(ngx_processes[s].channel, cycle->log);
>              return NGX_INVALID_PID;
>          }
> 
>          if (fcntl(ngx_processes[s].channel[0], F_SETOWN, ngx_pid) == -1) {
> -            ngx_log_error(NGX_LOG_ALERT, cycle->log, ngx_errno,
> +            ngx_log_error(NGX_LOG_ALERT, cycle->log, ngx_socket_errno,
>                            "fcntl(F_SETOWN) failed while spawning
> \"%s\"", name);
>              ngx_close_channel(ngx_processes[s].channel, cycle->log);
>              return NGX_INVALID_PID;
>          }
> 
>          if (fcntl(ngx_processes[s].channel[0], F_SETFD, FD_CLOEXEC) == -1) {
> -            ngx_log_error(NGX_LOG_ALERT, cycle->log, ngx_errno,
> +            ngx_log_error(NGX_LOG_ALERT, cycle->log, ngx_socket_errno,
>                            "fcntl(FD_CLOEXEC) failed while spawning \"%s\"",
>                             name);
>              ngx_close_channel(ngx_processes[s].channel, cycle->log);
>              return NGX_INVALID_PID;
>          }
> 
>          if (fcntl(ngx_processes[s].channel[1], F_SETFD, FD_CLOEXEC) == -1) {
> -            ngx_log_error(NGX_LOG_ALERT, cycle->log, ngx_errno,
> +            ngx_log_error(NGX_LOG_ALERT, cycle->log, ngx_socket_errno,
>                            "fcntl(FD_CLOEXEC) failed while spawning \"%s\"",
>                             name);
>              ngx_close_channel(ngx_processes[s].channel, cycle->log);
>              return NGX_INVALID_PID;

These seem questionable to me because the ioctl/fcntl calls are
known to report back in errno, and they're not wrapped into ngx_foo()
calls here.

My own preferences are as follows.

Use "ngx_errno" if we directly call ioctl/fcntl from the UNIX context.
I'd even prefer using "errno" here, but that doesn't seem to agree
with the current code.  Still, some code uses "errno" here, and that
perhaps should be fixed as well.

Use "ngx_socket_errno" for socket-related calls, including wrappers
such as ngx_nonblocking() known to apply to exclusively to sockets.