From: Vladimir Davydov <vdavydov.dev@gmail.com> To: Vladislav Shpilevoy <v.shpilevoy@tarantool.org> Cc: tarantool-patches@freelists.org Subject: Re: [tarantool-patches] Re: [PATCH 02/11] sio: remove unused functions, restyle code Date: Wed, 5 Dec 2018 11:41:03 +0300 [thread overview] Message-ID: <20181205084103.p2jkvxlohpbfiqtc@esperanza> (raw) In-Reply-To: <5861cffa-534b-fcb7-e000-c8a7b326cfd2@tarantool.org> On Wed, Dec 05, 2018 at 12:29:07AM +0300, Vladislav Shpilevoy wrote: > Hi! Thanks for the review. > > On 03/12/2018 15:28, Vladimir Davydov wrote: > > On Fri, Nov 30, 2018 at 06:39:34PM +0300, Vladislav Shpilevoy wrote: > > > -/** Read up to 'count' bytes from a socket. */ > > > ssize_t > > > sio_read(int fd, void *buf, size_t count) > > > { > > > ssize_t n = read(fd, buf, count); > > > - if (n < 0) { > > > - if (errno == EWOULDBLOCK) > > > - errno = EINTR; > > > - switch (errno) { > > > - case EAGAIN: > > > - case EINTR: > > > - break; > > > - /* > > > - * Happens typically when the client closes > > > - * socket on timeout without reading the previous > > > - * query's response completely. Treat the same as > > > - * EOF. > > > - */ > > > - case ECONNRESET: > > > - errno = 0; > > > - n = 0; > > > - break; > > > - default: > > > - tnt_raise(SocketError, sio_socketname(fd), > > > - "read(%zd)", count); > > > - } > > > + if (n >= 0) > > > + return n; > > > + if (errno == EWOULDBLOCK) > > > + errno = EINTR; > > > + switch (errno) { > > > + case EAGAIN: > > > + case EINTR: > > > + break; > > > + /* > > > + * Happens typically when the client closes socket on > > > + * timeout without reading the previous query's response > > > + * completely. Treat the same as EOF. > > > + */ > > > + case ECONNRESET: > > > + errno = 0; > > > + n = 0; > > > + break; > > > + default: > > > + tnt_raise(SocketError, sio_socketname(fd), "read(%zd)", count); > > > > If I had to write this function myself, I'd probably write it the same > > way as you intend to patch it, however it isn't worth polluting the > > history now. Please revert this change and all other changes that exist > > solely to make the code conform to your taste. The only thing that's > > worth doing now is removing unused functions, because otherwise you'd > > have to remove exceptions from them, too. I wouldn't even touch the > > comments - they look fine to me, as a matter of fact. > > > > > -int sio_setfl(int fd, int flag, int on); > > > +/** > > > + * Set a file descriptor flag. A wrapper for > > > + * fcntl(F_SETFL, flag | fcntl(F_GETFL)), but sets diagnostics on > > > + * error. > > > + */ > > > +int > > > +sio_setfl(int fd, int flag); > > > > This makes the API kinda lopsided - you can set a flag, but you can't > > clear it. Please leave it as is. > > It is always 1 everywhere. Makes no sense to provide an ability I know, but it has nothing to do with your goal. There are a lot of places in the code that I don't like - malformed indentation here and there, grammatical mistakes and typos in comments (there are plenty of them), weird function prototypes and names, but I refrain from patching them just in case, because this pollutes the git history, wastes reviewer's time, and complicates cherry-pickying. > to unset and I can not imagine when we could need such 'feature'. > > But as you wish. Done. See v2 in a new thread. Too many changes to > paste them here. Thanks.
next prev parent reply other threads:[~2018-12-05 8:41 UTC|newest] Thread overview: 37+ messages / expand[flat|nested] mbox.gz Atom feed top 2018-11-30 15:39 [PATCH 00/11] SWIM preparation Vladislav Shpilevoy 2018-11-30 15:39 ` [PATCH 01/11] box: move info_handler interface into src/info Vladislav Shpilevoy 2018-12-03 11:05 ` Vladimir Davydov 2018-12-03 21:48 ` [tarantool-patches] " Vladislav Shpilevoy 2018-12-03 20:41 ` [tarantool-patches] " Konstantin Osipov 2018-12-03 21:48 ` [tarantool-patches] " Vladislav Shpilevoy 2018-12-04 8:52 ` Vladimir Davydov 2018-11-30 15:39 ` [PATCH 10/11] evio: remove exceptions Vladislav Shpilevoy 2018-11-30 15:39 ` [PATCH 11/11] evio: turn into C Vladislav Shpilevoy 2018-11-30 15:39 ` [PATCH 02/11] sio: remove unused functions, restyle code Vladislav Shpilevoy 2018-12-03 12:28 ` Vladimir Davydov 2018-12-04 21:29 ` [tarantool-patches] " Vladislav Shpilevoy 2018-12-05 8:41 ` Vladimir Davydov [this message] 2018-11-30 15:39 ` [PATCH 03/11] sio: remove exceptions Vladislav Shpilevoy 2018-12-03 12:36 ` Vladimir Davydov 2018-12-04 21:29 ` [tarantool-patches] " Vladislav Shpilevoy 2018-12-05 8:42 ` Vladimir Davydov 2018-11-30 15:39 ` [PATCH 04/11] sio: fix passing negative size_t to sio_add_to_iov Vladislav Shpilevoy 2018-12-03 13:50 ` Vladimir Davydov 2018-12-04 21:29 ` Vladislav Shpilevoy 2018-12-05 8:48 ` Vladimir Davydov 2018-11-30 15:39 ` [PATCH 05/11] sio: turn into C Vladislav Shpilevoy 2018-11-30 15:39 ` [PATCH 06/11] evio: make on_accept be nothrow Vladislav Shpilevoy 2018-12-03 14:58 ` Vladimir Davydov 2018-12-04 21:29 ` [tarantool-patches] " Vladislav Shpilevoy 2018-12-05 8:52 ` Vladimir Davydov 2018-11-30 15:39 ` [PATCH 07/11] coio: fix file descriptor leak on accept Vladislav Shpilevoy 2018-12-03 16:16 ` Vladimir Davydov 2018-12-04 21:29 ` [tarantool-patches] " Vladislav Shpilevoy 2018-11-30 15:39 ` [PATCH 08/11] coio: fix double close of a file descriptor Vladislav Shpilevoy 2018-12-03 16:19 ` Vladimir Davydov 2018-12-04 21:29 ` [tarantool-patches] " Vladislav Shpilevoy 2018-11-30 15:39 ` [PATCH 09/11] evio: refactoring Vladislav Shpilevoy 2018-12-03 16:37 ` Vladimir Davydov 2018-12-04 21:29 ` [tarantool-patches] " Vladislav Shpilevoy 2018-12-03 9:47 ` [PATCH 00/11] SWIM preparation Vladimir Davydov 2018-12-03 10:27 ` [tarantool-patches] " Vladislav Shpilevoy
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=20181205084103.p2jkvxlohpbfiqtc@esperanza \ --to=vdavydov.dev@gmail.com \ --cc=tarantool-patches@freelists.org \ --cc=v.shpilevoy@tarantool.org \ --subject='Re: [tarantool-patches] Re: [PATCH 02/11] sio: remove unused functions, restyle code' \ /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