From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtpng2.m.smailru.net (smtpng2.m.smailru.net [94.100.179.3]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by dev.tarantool.org (Postfix) with ESMTPS id 4199B469719 for ; Wed, 11 Mar 2020 03:30:23 +0300 (MSK) References: <045901d5f6e7$bc2d47a0$3487d6e0$@tarantool.org> From: Vladislav Shpilevoy Message-ID: <1e1a11dc-cce3-603d-69f0-3dabf371d59a@tarantool.org> Date: Wed, 11 Mar 2020 01:30:19 +0100 MIME-Version: 1.0 In-Reply-To: <045901d5f6e7$bc2d47a0$3487d6e0$@tarantool.org> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit Subject: Re: [Tarantool-patches] [PATCH] Work-around WSL assert when SO_LINGER is set on unix sockets List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Timur Safin , tarantool-patches@dev.tarantool.org, Kirill Yukhin Hi! Thanks for the patch! See 7 comments below. 1. Please, add a subsystem prefix to the commit title. For examples see other commits in the repository. On 10/03/2020 15:25, Timur Safin wrote: > Using SO_LINGER over unix sockets makes no much sense, though > it's harmless on Linux. The problem is, it breaks majority of > tests under Windows/WSL with assertion, because setsockopt() > would return EINVAL in that case under WSL.> > This is known WSL issue and reported here > https://github.com/microsoft/WSL/issues/3992 > > So we filter out SO_LINGER if evio_setsockopt_server is being > called with AF_UNIX family. > > Branch: https://github.com/tarantool/tarantool/tree/tsafin/gh-4659-wsl-no-linger-assert 2. Please, provide both branch and issue links. 3. I see, that on the branch your commit message is just empty. Seems like you didn't push the latest message. 4. The patch has nothing to do with gh-4659: https://github.com/tarantool/tarantool/issues/4659 "sql: raise an error in case space features HASH index". > --- 5. Links should be below this marker '---'. Some people apply patches from emails, and when you write links above '---', you make them part of the commit message. > src/lib/core/evio.c | 15 +++++++++------ > 1 file changed, 9 insertions(+), 6 deletions(-) > > diff --git a/src/lib/core/evio.c b/src/lib/core/evio.c > index 2152c15e6..06aa11ce4 100644 > --- a/src/lib/core/evio.c > +++ b/src/lib/core/evio.c > @@ -140,13 +140,16 @@ evio_setsockopt_server(int fd, int family, int type) > &on, sizeof(on))) > return -1; > > - /* Send all buffered messages on socket before take > - * control out from close(2) or shutdown(2). */ > - struct linger linger = { 0, 0 }; > + if (family != AF_UNIX) { 6. Is there any proof that it is no-op on Linux for AF_UNIX? I would rather call sio_setsockopt() always. And ignore an error, if it is EINVAL for AF_UNIX. 7. It is worth adding a comment why SO_LINGER is workarounded somehow. > + /* Send all buffered messages on socket before > + * take control out from close(2) or shutdown(2). */ > + struct linger linger = { 0, 0 }; > + > + if (sio_setsockopt(fd, SOL_SOCKET, SO_LINGER, > + &linger, sizeof(linger))) > + return -1; > + } > > - if (sio_setsockopt(fd, SOL_SOCKET, SO_LINGER, > - &linger, sizeof(linger))) > - return -1; > if (type == SOCK_STREAM && family != AF_UNIX && > evio_setsockopt_keepalive(fd) != 0) > return -1; >