From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Subject: Re: [tarantool-patches] Re: [PATCH 03/11] sio: remove exceptions References: <14e50e3a3872a1898d5df006fb61442c1ab2e556.1543590433.git.v.shpilevoy@tarantool.org> <20181203123642.hpo5nhkndt7qftcl@esperanza> From: Vladislav Shpilevoy Message-ID: Date: Wed, 5 Dec 2018 00:29:10 +0300 MIME-Version: 1.0 In-Reply-To: <20181203123642.hpo5nhkndt7qftcl@esperanza> Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 7bit To: Vladimir Davydov Cc: tarantool-patches@freelists.org List-ID: >> /** >> * Receive a message on a socket. A wrapper for recvfrom(), but >> - * throws exception on error except EAGAIN, EINTR, EWOULDBLOCK. >> + * sets diagnostics on error except EAGAIN, EINTR, EWOULDBLOCK. >> + * @param fd Socket to receive from. >> + * @param buf Buffer to save message. >> + * @param len Size of @a buf. >> + * @param flags recvfrom() flags. >> + * @param[out] src_addr Source address. >> + * @param[in, out] addrlen Size of @a src_addr. >> + * @param[out] is_error_fatal Set to true, if an error occured and >> + * it was not EAGAIN, EINTR and EWOULDBLOCK. >> + * >> + * @param How many bytes are received, or -1 on error. >> */ >> ssize_t >> sio_recvfrom(int fd, void *buf, size_t len, int flags, >> - struct sockaddr *src_addr, socklen_t *addrlen); >> + struct sockaddr *src_addr, socklen_t *addrlen, >> + bool *is_error_fatal); > > This new API doesn't look good to me. Let's instead set diag and return > -1 on every error and introduce a helper to check if a particular error > code means that the socket would block (the name might need to be > refined):> > sio_wouldblock(errno) > > ? > The only purpose of the out flag was to hide from a user details of how exactly to check if an error is not critical: via errno, or via checking diag_is_empty(diag_get()) or via any other way. So a user wouldn't need to include errno or use complex constructions like with diag. But as you wish. Lets find another way. 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. Also, notion of 'critical' is different for various functions. I've grouped errors in 4 groups: EINPROGRESS - sio_connect EADDRINUSE - sio_bind, sio_listen EAGAIN - sio_accept, sio_write, EWOULDBLOCK sio_writev, sio_readn_ahead, EINTR sio_sendto, sio_recvfrom ECONNRESET - sio_read EAGAIN EWOULDBLOCK EINTR I can not implement one function for checking of any error via errno because * EADDRINUSE is not critical for bind/listen, but critical for connect; * EAGAIN, EINTR are not critical for IO, but critical for connect; * ECONNRESET is not critical for sio_read, but critical for sio_sendto. The list is not complete I think, and I do not want to fix some crutches in a single error checking function each time when something changed in any other sio function. I can not use the simplest way !diag_is_empty(diag_get()) as well because as I discovered, iproto thread does not clean diag in some places after processing an error. For example, iproto_connection_resume on error leaves an error in diag and the error lives in the thread until next error. The same in iproto_connection_on_input(), where catch() block does not clears diag. I could use diag if I fixed these places, but I do not want to rely on cleanness of external code. I decided to return from some functions a special code that can be checked by sio_is_error_fatal(). See v2 thread for a new version of the commit. I do not paste diff here, since in fact the patch is rewritten completely.