From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <vdavydov.dev@gmail.com>
Date: Sun, 9 Dec 2018 15:54:13 +0300
From: Vladimir Davydov <vdavydov.dev@gmail.com>
Subject: Re: [PATCH v2 03/11] sio: remove exceptions
Message-ID: <20181209125413.3zkcuchfltd4smph@esperanza>
References: <cover.1543958698.git.v.shpilevoy@tarantool.org>
 <fd79be8d0a088041d5a33dca9af6a5c78691159d.1543958698.git.v.shpilevoy@tarantool.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <fd79be8d0a088041d5a33dca9af6a5c78691159d.1543958698.git.v.shpilevoy@tarantool.org>
To: Vladislav Shpilevoy <v.shpilevoy@tarantool.org>
Cc: tarantool-patches@freelists.org
List-ID: <tarantool-patches.dev.tarantool.org>

On Wed, Dec 05, 2018 at 12:28:50AM +0300, Vladislav Shpilevoy wrote:
> @@ -40,6 +40,14 @@
>  #include "trivia/util.h"
>  #include "exception.h"
>  
> +enum { SIO_ERROR_FATAL = -2 };
> +
> +bool
> +sio_is_error_fatal(int retcode)
> +{
> +	return retcode == SIO_ERROR_FATAL;
> +}
> +
>  /** Pretty print socket name and peer (for exceptions) */
>  const char *
>  sio_socketname(int fd)
> @@ -224,9 +234,11 @@ sio_accept(int fd, struct sockaddr *addr, socklen_t *addrlen)
>  {
>  	/* Accept a connection. */
>  	int newfd = accept(fd, addr, addrlen);
> -	if (newfd < 0 &&
> -	    (errno != EAGAIN && errno != EWOULDBLOCK && errno != EINTR))
> -		tnt_raise(SocketError, sio_socketname(fd), "accept");
> +	if (newfd < 0 && errno != EAGAIN && errno != EWOULDBLOCK &&
> +	    errno != EINTR) {
> +		diag_set(SocketError, sio_socketname(fd), "accept");
> +		return SIO_ERROR_FATAL;
> +	}
>  	return newfd;
>  }

This looks bad. It's not just my opinion. I consulted with Kostja
verbally and he agrees with me.

Let's please set diag on any error in all sio functions and let the
caller decide which ones are critical and which are not by checking
errno. To simplify the caller's job we can introduce a helper function
checking errno for cetain types of errors, say

  sio_wouldblock(errno): errno in EINTR, EAGAIN, EWOULDBLOCK

I've already suggested you to do this. Quoting your reply here:

> Any names having 'wouldblock', 'block' or describing other concrete
> behaviour can not be used. At least, sio_bind/listen can set EADDRINUSE,
> which is not about blocking.

Yeah, I agree, we should only use sio_wouldblock for EAGAIN, EINTR, and
EWOULDBLOCK. Other error codes should be checked explicitly.

> 
> Also, notion of 'critical' is different for various functions. I've
> grouped errors in 4 groups:
> 
> EINPROGRESS - sio_connect

Check (errno != EINPROGRESS) after calling sio_connect().

> 
> EADDRINUSE  - sio_bind, sio_listen

errno != EADDRINUSE

> 
> EAGAIN      - sio_accept, sio_write,
> EWOULDBLOCK   sio_writev, sio_readn_ahead,
> EINTR         sio_sendto, sio_recvfrom

sio_wouldblock(errno)

> 
> ECONNRESET  - sio_read
> EAGAIN
> EWOULDBLOCK
> EINTR

Actually, sio_read() never returns ECONNRESET - it treats it as EOF
(i.e. returns 0) and doesn't raise an error. So for sio_read(), the
caller can use sio_wouldblock() as well.