From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp56.i.mail.ru (smtp56.i.mail.ru [217.69.128.36]) (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 9E6D846971A for ; Sun, 8 Dec 2019 22:48:05 +0300 (MSK) Date: Sun, 8 Dec 2019 22:48:00 +0300 From: Alexander Turenko Message-ID: <20191208194800.b6oiyzy3zx23mv4h@tkn_work_nb> References: <20190709080407.sv5quzrxpbnijwp4@tkn_work_nb> <20190723123954.dgtfsetqlmjhued3@tkn_work_nb> <704142F3-802F-4C1C-8FA2-6155BFF9A951@tarantool.org> <20190829004547.k24fktai33tlbl73@tkn_work_nb> <868EAF2C-A491-46C9-AD37-7512D6CAB213@tarantool.org> <20190906134509.egjpa2vxfdolkeme@tkn_work_nb> <20191101143929.7gv53nqgscwpbayv@tkn_work_nb> <0D96FEEE-3814-4C90-980F-723A837F8157@tarantool.org> MIME-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <0D96FEEE-3814-4C90-980F-723A837F8157@tarantool.org> Subject: Re: [Tarantool-patches] [tarantool-patches] [server-dev] Re: Re: [PATCH v2 1/2] lua: return getaddrinfo() errors List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Roman Khabibov Cc: tarantool-patches@dev.tarantool.org I didn't find where to comment args validation patch, so I'll comment it here: It changes socket.connect() (part of Lua Socket API), but does not change luasocket_master:connect() (part of Lua Socket API too) and does not change socket.tcp_connect() (our native function). The same: socket.bind() (Lua Socket API) was changed, but luasocket_master:bind() (Lua Socket API) was not. Our native socket.tcp_server() was not changed too. That's all looks strange. Let's extract this patch and either drop it or bring it to an acceptable state and send separately from this patchset. ---- > > `socket_object:connect` is not valid Lua syntax. It is better to write > > it as socket_object:connect() or socket_object.connect. > > > > net.box API also was changed. > > net.box’s .connect() returns error as second return value. Now, it can return > tcp_connect()’s errors (getaddrinfo). Don’t think, we should note this in the > net.box doc. I see now, the API is not changed, only connection.error message. > >> Connect to a remote host using tcp_connect() within. If it is no > >> error, return a socket object. > > > > It is hard to understood w/o a context. Please, either describe API > > changes formally or give an example that will show the API change. > In the commit message. See the comment below. > >> diff --git a/src/lua/socket.lua b/src/lua/socket.lua > >> index a334ad45b..e4815adbb 100644 > >> --- a/src/lua/socket.lua > >> +++ b/src/lua/socket.lua > >> @@ -1041,9 +1041,12 @@ local function tcp_connect(host, port, timeout) > >> end > >> local timeout = timeout or TIMEOUT_INFINITY > >> local stop = fiber.clock() + timeout > >> - local dns = getaddrinfo(host, port, timeout, { type = 'SOCK_STREAM', > >> + local dns, err = getaddrinfo(host, port, timeout, { type = 'SOCK_STREAM', > >> protocol = 'tcp' }) > >> - if dns == nil or #dns == 0 then > >> + if dns == nil then > >> + return nil, err > >> + end > >> + if #dns == 0 then > >> boxerrno(boxerrno.EINVAL) > >> return nil > >> end > > > > Cited to link from below. > > > >> @@ -1360,9 +1363,16 @@ local function lsocket_tcp_connect(self, host, port) > >> -- This function is broken by design > >> local ga_opts = { family = 'AF_INET', type = 'SOCK_STREAM' } > >> local timeout = deadline - fiber.clock() > >> - local dns = getaddrinfo(host, port, timeout, ga_opts) > >> + local dns, err = getaddrinfo(host, port, timeout, ga_opts) > >> if dns == nil or #dns == 0 then > >> - self._errno = boxerrno.EINVAL > >> + return nil, err > >> + end > >> + if dns == nil then > >> + boxerrno(self._errno) > >> + return nil, err > >> + end > >> + if #dns == 0 then > >> + boxerrno(self._errno) > >> return nil, socket_error(self) > >> end > > > > We'll never reach the last two if's bodies. However the similar fragment > > above in tcp_connect() was changed in the right way. > > @@ -1360,9 +1363,12 @@ local function lsocket_tcp_connect(self, host, port) > -- This function is broken by design > local ga_opts = { family = 'AF_INET', type = 'SOCK_STREAM' } > local timeout = deadline - fiber.clock() > - local dns = getaddrinfo(host, port, timeout, ga_opts) > - if dns == nil or #dns == 0 then > - self._errno = boxerrno.EINVAL > + local dns, err = getaddrinfo(host, port, timeout, ga_opts) > + if dns == nil then > + return nil, err > + end > + if #dns == 0 then > + boxerrno(self._errno) > return nil, socket_error(self) > end > for _, remote in ipairs(dns) do EINVAL here is changed to self._errno, which is always `nil` at least for typical usage of lua socket: `s = socket.tcp()`, then `s:connect()`. Let's keep the old behaviour (EINVAL) when getaddrinfo() returns zero amount of addresses. RFC 2553 ('Basic Socket Interface Extensions for IPv6') states: | Upon successful return a pointer to a linked list of one or more | addrinfo structures is returned through the final argument. RFC 3493 ('Basic Socket Interface Extensions for IPv6') states the same, but more explicitly: | Upon successful return of getaddrinfo(), the location to which res | points shall refer to a linked list of addrinfo structures, each of | which shall specify a socket address and information for use in | creating a socket with which to use that socket address. The list | shall include at least one addrinfo structure. So this situation should not occur. But anyway we should not miss to set a certain errno in the case at least for consistency with tcp_connect(). Also I think that we should set errno to EINVAL when getaddrinfo() fails to keep the code backward compatible: say, a user lean on this value when tcp_connect() / :connect() returns `nil`. > commit b3a8af5c605db811deb40946cfbcf9dcd45ad75c > Author: Roman Khabibov > Date: Thu Nov 21 14:37:54 2019 +0300 > > lua: return getaddrinfo() errors > > Add getaddrinfo() errors into the several fuctions of socket. Now > getaddrinfo() can return a pair of values (nil and error message) > in case of error. > > Closes #4138 > > @TarantoolBot document > Title: socket API changes > > * socket.getaddrinfo() > > Can return error message as second return value in case of > internal getaddrinfo() error. > > Example: > tarantool> socket.getaddrinfo('non_exists_hostname', 3301) > --- > - null > - 'getaddrinfo: nodename nor servname provided, or not known' > ... > > * socket.tcp_connect() > > Can return socket.getaddrinfo() error message as second return > value. > > Example: > tarantool> socket.tcp_connect('non_exists_hostname', 3301) > --- > - null > - 'getaddrinfo: nodename nor servname provided, or not known' > ... > > * socket_object:connect() > > Wrapper for the socket.tcp_connect() with arguments format > checking. If it is no error, return a socket object. If it is not, > return nil and error message (socket.tcp_connect() just returns > nil in case of error except when it is an internal getaddrinfo() > error). socket_object:connect() is part of Lua Socket API, so I would name the instance 'luasocket_master': this would follow Lua Socket terminology: http://w3.impa.br/~diego/software/luasocket/tcp.html We end with the situation that Lua Socket function checks its args, but our one does not. This looks strange. Describing this function whose primary reason is to verify arguments is not correct too. This function exists for compatibility with Lua Socket API. The behaviour change here is that luasocket_master:connect() returns an error from getaddrinfo() in the case rather than 'Invalid argument' as it was before the commit. So the API is not changed and we can remove this function from the docbot request (the same situation as in net.box). Also I see that we don't document Lua Socket part of the API: https://www.tarantool.io/en/doc/1.10/reference/reference_lua/socket/ > diff --git a/test/app/socket.test.lua b/test/app/socket.test.lua > index 022cd4f40..9e66d6a8b 100644 > --- a/test/app/socket.test.lua > +++ b/test/app/socket.test.lua > <...> > @@ -968,6 +976,26 @@ socket.bind('127.0.0.1', 3301, '1') > > test_run:cmd("clear filter") > > +-- gh-4138 Check getaddrinfo() error from socket:connect() only. > +-- Error code and error message returned by getaddrinfo() depends > +-- on system's gai_strerror(). > +test_run:cmd("setopt delimiter ';'") > +function check_err(err) > + if err == 'getaddrinfo: nodename nor servname provided, or not known' or > + err == 'getaddrinfo: Servname not supported for ai_socktype' or > + err == 'getaddrinfo: Name or service not known' then > + return true > + end > + return false > +end; > +test_run:cmd("setopt delimiter ''"); > + > +s, err = socket.getaddrinfo('non_exists_hostname', 3301) > +check_err(err) > +s, err = socket.connect('non_exists_hostname', 3301) > +check_err(err) Okay, we have the test case for Lua Socket API's socket.connect(). Let's add a case for our native socket.tcp_connect().