From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Subject: Re: [tarantool-patches] Re: [PATCH 08/11] coio: fix double close of a file descriptor References: <20181203161950.2eusd6vxj5zejeof@esperanza> From: Vladislav Shpilevoy Message-ID: <5ff038a6-ea42-727a-3077-df6472e97bcc@tarantool.org> Date: Wed, 5 Dec 2018 00:29:26 +0300 MIME-Version: 1.0 In-Reply-To: <20181203161950.2eusd6vxj5zejeof@esperanza> Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 7bit To: Vladimir Davydov Cc: tarantool-patches@freelists.org List-ID: 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.