From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Date: Wed, 5 Dec 2018 11:42:41 +0300 From: Vladimir Davydov Subject: Re: [tarantool-patches] Re: [PATCH 03/11] sio: remove exceptions Message-ID: <20181205084241.4oe53lf7cqhk7f66@esperanza> References: <14e50e3a3872a1898d5df006fb61442c1ab2e556.1543590433.git.v.shpilevoy@tarantool.org> <20181203123642.hpo5nhkndt7qftcl@esperanza> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: To: Vladislav Shpilevoy Cc: tarantool-patches@freelists.org List-ID: On Wed, Dec 05, 2018 at 12:29:10AM +0300, Vladislav Shpilevoy wrote: > > > > /** > > > * 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. OK, I see. I'll think what we can do about it. > > 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.