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.
next prev parent 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