Tarantool development patches archive
 help / color / mirror / Atom feed
From: Vladimir Davydov <vdavydov.dev@gmail.com>
To: Vladislav Shpilevoy <v.shpilevoy@tarantool.org>
Cc: tarantool-patches@freelists.org
Subject: Re: [PATCH v2 03/11] sio: remove exceptions
Date: Sun, 9 Dec 2018 15:54:13 +0300	[thread overview]
Message-ID: <20181209125413.3zkcuchfltd4smph@esperanza> (raw)
In-Reply-To: <fd79be8d0a088041d5a33dca9af6a5c78691159d.1543958698.git.v.shpilevoy@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.

  reply	other threads:[~2018-12-09 12:54 UTC|newest]

Thread overview: 23+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-12-04 21:28 [PATCH v2 00/11] SWIM preparation Vladislav Shpilevoy
2018-12-04 21:28 ` [PATCH v2 01/11] sio: remove unused functions Vladislav Shpilevoy
2018-12-09 12:10   ` Vladimir Davydov
2018-12-04 21:28 ` [PATCH v2 10/11] evio: make code C compatible Vladislav Shpilevoy
2018-12-05  8:56   ` Vladimir Davydov
2018-12-05 20:07     ` [tarantool-patches] " Vladislav Shpilevoy
2018-12-04 21:28 ` [PATCH v2 11/11] evio: turn nto c Vladislav Shpilevoy
2018-12-04 21:28 ` [PATCH v2 02/11] sio: treat EADDRINUSE in sio_listen as error Vladislav Shpilevoy
2018-12-09 12:57   ` Vladimir Davydov
2018-12-10 15:36     ` [tarantool-patches] " Vladislav Shpilevoy
2018-12-04 21:28 ` [PATCH v2 03/11] sio: remove exceptions Vladislav Shpilevoy
2018-12-09 12:54   ` Vladimir Davydov [this message]
2018-12-10 15:37     ` [tarantool-patches] " Vladislav Shpilevoy
2018-12-11  8:44       ` Vladimir Davydov
2018-12-04 21:28 ` [PATCH v2 04/11] sio: make code compatible with C Vladislav Shpilevoy
2018-12-05  8:57   ` Vladimir Davydov
2018-12-05 20:07     ` [tarantool-patches] " Vladislav Shpilevoy
2018-12-04 21:28 ` [PATCH v2 05/11] sio: turn into C Vladislav Shpilevoy
2018-12-04 21:28 ` [PATCH v2 06/11] evio: make on_accept be nothrow Vladislav Shpilevoy
2018-12-04 21:28 ` [PATCH v2 07/11] coio: fix double close of a file descriptor Vladislav Shpilevoy
2018-12-04 21:28 ` [PATCH v2 08/11] evio: remove exceptions Vladislav Shpilevoy
2018-12-04 21:28 ` [PATCH v2 09/11] coio: fix file descriptor leak on accept Vladislav Shpilevoy
2018-12-11  8:47 ` [PATCH v2 00/11] SWIM preparation Vladimir Davydov

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20181209125413.3zkcuchfltd4smph@esperanza \
    --to=vdavydov.dev@gmail.com \
    --cc=tarantool-patches@freelists.org \
    --cc=v.shpilevoy@tarantool.org \
    --subject='Re: [PATCH v2 03/11] sio: remove exceptions' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox