[tarantool-patches] Re: [PATCH 02/11] sio: remove unused functions, restyle code

[Vladislav Shpilevoy]

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
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.