From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from localhost (localhost [127.0.0.1]) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTP id D4B3022554 for ; Tue, 9 Jul 2019 04:04:36 -0400 (EDT) Received: from turing.freelists.org ([127.0.0.1]) by localhost (turing.freelists.org [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id Tp1PiOclSMN0 for ; Tue, 9 Jul 2019 04:04:36 -0400 (EDT) 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 turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTPS id 219B721CF0 for ; Tue, 9 Jul 2019 04:04:36 -0400 (EDT) Date: Tue, 9 Jul 2019 11:04:11 +0300 From: Alexander Turenko Subject: [tarantool-patches] Re: [PATCH v2 1/2] lua: return getaddrinfo() errors Message-ID: <20190709080407.sv5quzrxpbnijwp4@tkn_work_nb> References: <20190615165829.11888-1-roman.habibov@tarantool.org> <20190623203141.snnjgyqrntr6okp4@tkn_work_nb> <8C0820C4-7F5E-40A9-A634-E449E700D816@tarantool.org> MIME-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <8C0820C4-7F5E-40A9-A634-E449E700D816@tarantool.org> Sender: tarantool-patches-bounce@freelists.org Errors-to: tarantool-patches-bounce@freelists.org Reply-To: tarantool-patches@freelists.org List-Help: List-Unsubscribe: List-software: Ecartis version 1.0.0 List-Id: tarantool-patches List-Subscribe: List-Owner: List-post: List-Archive: To: Roman Khabibov Cc: tarantool-patches@freelists.org > > The commit message should mention Roman's commit and cleanly state that > > (rc < 0) assumption does not work on Mac OS. This is not done. > + /* gh-4138: Check getaddrinfo() error. */ > + isnt(coio_getaddrinfo("hostname", port, NULL, &i, 1), 0, "getaddrinfo error"); The line is longer then 80 symbols. > + isnt(strstr(diag_get()->last->errmsg, "getaddrinfo"), NULL, > + "getaddrinfo error"); It would be good to have different case names. The problem that stated in the issue is about improper error message. I don't understand the test in this context: it does not check whether it is so. It is applicable to lua tests too. > + * Wrap coio_getaddrinfo() and call it. Push returned values onto > + * @a L Lua stack. > + * > + * @param L Lua stack. > + * > + * @retval 1 Number of returned values by Lua function if > + * coio_getaddrinfo() success. > + * @retval 2 Number of returned values by Lua function if > + * coio_getaddrinfo() is failed (fist is nil, second is error Typo: fist -> first. > + * message). > + * @retval One of luaT_error() codes. 'luaT_error() code' is the strange phrase. I guess you want to say that the function can raise a lua error. It is not a return value, because a function does not return at all in the case (it is longjmp to be precise). > >> +s, err = socket:connect('hostname:3301'); > > > > I would use less generic hostname like 'non_exists_hostname’. The comment is still actual for test/unit/coio.cc. (I have copied parts of your patch below to comment them.) > @@ -1347,10 +1350,10 @@ 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, socket_error(self) > + self._errno = boxerrno.EINVAL Broken indent. > @@ -1534,9 +1537,12 @@ local function lsocket_connect(host, port) > if host == nil or port == nil then > error("Usage: luasocket.connect(host, port)") > end > - local s = tcp_connect(host, port) > + local s, err = tcp_connect(host, port) > if not s then > - return nil, boxerrno.strerror() > + if not err then > + return nil, boxerrno.strerror() > + end > + return nil, err Why not always set errno to EINVAL and return err, nil in case on an error? It seems the 'if' here is redundant. Also applicable to similar changes in other lua functions. > for i, remote in pairs(dns) do > timeout = stop - fiber.clock() > if timeout <= 0 then > boxerrno(boxerrno.ETIMEDOUT) > - return nil > + return nil, 'timed out' > end > local s = socket_new(remote.family, remote.type, remote.protocol) > if s then Do you really think this change is self-explanatory and it is obvious why this is needed (and why this was not needed before)? Yep, we discussed it voicely, but I'm not a last reader of your code.