Re: [tarantool-patches] Re: [PATCH 08/11] coio: fix double close of a file descriptor

[Vladislav Shpilevoy <v.shpilevoy@tarantool.org>]

On 03/12/2018 19:19, Vladimir Davydov wrote:
> On Fri, Nov 30, 2018 at 06:39:40PM +0300, Vladislav Shpilevoy wrote:
>> coio_service_on_accept is called by evio by an
>> on_accept pointer. If evio obtains not zero from
>> on_accept pointer, it closes accepted socket. But
>> coio_service_on_accept closes it too, when fiber_new
>> fails. It is double close.
>>
>> Note that the bug existed even when on_accept was able
>> to throw.
>> ---
>>   src/coio.cc | 6 ++----
>>   1 file changed, 2 insertions(+), 4 deletions(-)
>>
>> diff --git a/src/coio.cc b/src/coio.cc
>> index 575bae712..a888a54dd 100644
>> --- a/src/coio.cc
>> +++ b/src/coio.cc
>> @@ -605,9 +605,6 @@ coio_service_on_accept(struct evio_service *evio_service,
>>   {
>>   	struct coio_service *service = (struct coio_service *)
>>   			evio_service->on_accept_param;
>> -	struct ev_io coio;
>> -
>> -	coio_create(&coio, fd);
>>   
>>   	/* Set connection name. */
>>   	char fiber_name[SERVICE_NAME_MAXLEN];
>> @@ -619,9 +616,10 @@ coio_service_on_accept(struct evio_service *evio_service,
>>   	if (f == NULL) {
>>   		diag_log();
>>   		say_error("can't create a handler fiber, dropping client connection");
>> -		evio_close(loop(), &coio);
>>   		return -1;
>>   	}
>> +	struct ev_io coio;
>> +	coio_create(&coio, fd);
>>   	/*
>>   	 * The coio is passed into the created fiber, reset the
>>   	 * libev callback param to point at it.
> 
> I see now that you remove an extra close() from coio_service_on_accept
> in a separate patch. This is OK, but then please remove an extra close()
> from iproto_on_accept in the same patch, not in patch 6, where you make
> on_accept exception free.
> 

If I left close in iproto_on_accept in the patch making on_accept
nothrow, I would introduce a new bug - double close in iproto_on_accept.
So I can not do it. Even temporary. Patch about nothrow on_accept is
refactoring, this patch is bugfix.

In v2 the patch is left as is.