From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <vdavydov.dev@gmail.com>
Date: Mon, 3 Dec 2018 15:28:19 +0300
From: Vladimir Davydov <vdavydov.dev@gmail.com>
Subject: Re: [PATCH 02/11] sio: remove unused functions, restyle code
Message-ID: <20181203122819.uvooq6w7auzgjrj3@esperanza>
References: <cover.1543590433.git.v.shpilevoy@tarantool.org>
 <773c52aa9e5bb53fb7ff4245cc5613bde18e8948.1543590433.git.v.shpilevoy@tarantool.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <773c52aa9e5bb53fb7ff4245cc5613bde18e8948.1543590433.git.v.shpilevoy@tarantool.org>
To: Vladislav Shpilevoy <v.shpilevoy@tarantool.org>
Cc: tarantool-patches@freelists.org
List-ID: <tarantool-patches.dev.tarantool.org>

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.