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 AF9EB256B0 for ; Tue, 29 Jan 2019 14:44:18 -0500 (EST) 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 A2bJkBqUmstY for ; Tue, 29 Jan 2019 14:44:18 -0500 (EST) Received: from smtpng3.m.smailru.net (smtpng3.m.smailru.net [94.100.177.149]) (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 5FF3A21C82 for ; Tue, 29 Jan 2019 14:44:18 -0500 (EST) Subject: [tarantool-patches] Re: [PATCH] httpc: add checking of headers in httpc:request References: <20181221181844.4417-1-roman.habibov@tarantool.org> <20181223031929.hhmxdlsox2aon5ih@tkn_work_nb> <3b2626ce-b887-70dd-38b0-ef38b37a983c@tarantool.org> <72bbdfde-54d7-0e94-c5e7-ca84c2a6078d@tarantool.org> From: Vladislav Shpilevoy Message-ID: Date: Tue, 29 Jan 2019 22:44:16 +0300 MIME-Version: 1.0 In-Reply-To: <72bbdfde-54d7-0e94-c5e7-ca84c2a6078d@tarantool.org> Content-Type: text/plain; charset="utf-8"; format="flowed" Content-Language: en-US Content-Transfer-Encoding: 8bit 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: tarantool-patches@freelists.org, Roman , Alexander Turenko Hi! Thanks for the patch! See 7 comments below. On 09/01/2019 16:29, Roman wrote: > >> 1. Firstly, I think, that the patch should be in C in >> lua/httpc.c and affect only function luaT_httpc_request. It >> will prevent creation of additional table 'res', maybe >> will look even shorter, and evidently will be more efficient. >> It is not so complex logic so as to write it in Lua, IMO. >> > Done. > > commit beb0562425dd5062af5d18ae202f0f4434cb5108 > Author: Roman Khabibov > Date:   Wed Dec 26 17:49:34 2018 +0300 > >     httpc: add checking of headers in httpc:request > >     Add preprocessing of the request headers. Each header must be nil, 'string' or 'table' >     with '__tostring' metamethod. > >     Closes #3679 > > diff --git a/src/lua/httpc.c b/src/lua/httpc.c > index 5f4e2e912..16a435ef1 100644 > --- a/src/lua/httpc.c > +++ b/src/lua/httpc.c > @@ -173,6 +173,15 @@ luaT_httpc_request(lua_State *L) >      if (!lua_isnil(L, -1)) { >          lua_pushnil(L); 1. I see, that you didn't use format-patch + send-email, since here we have white-spaces, but in the real diff there are tabs. Please, use format-patch + send-email. Or if you used them, then fix you text editor. Or stop using web client - use a specialized one like Thunderbird. >          while (lua_next(L, -2) != 0) { > +            int header_type = lua_type(L, -1); > +            if (header_type != LUA_TSTRING) { 2. Why so complex instead of just lua_isstring()? > +                if (header_type != LUA_TTABLE) { > +                    return luaL_error(L, "cannot convert header to a string"); 3. Word 'cannot' does not exist. Please, use "can't" or "can not". 4. In the commit message you said that 'nil' is ok, but here you did not check for that and return an error. Of course, real nil can not appear in a table, because it means absence of a value, but what about box.NULL? 5. Out of 80 symbols. > +                } else if (luaL_getmetafield(L, -1, "__tostring") == LUA_TNIL) { 6. Why do you compare with LUA_TNIL instead of direct checking of 0/1? I see, that luaL_getmetafield does return exactly 0 or 1, not LUA_TNIL. Maybe they accidentally matched, but it is not a reason to use LUA_TNIL here. > +                    return luaL_error(L, "cannot convert header to a string"); > +                } > +                lua_pop(L, 1); 7. Why do you pop? Now on a stack we have key (L[-2]) and value (L[-1]). Here you pops one, and we have key (L[-1]). So the function below does a mess. And maybe this is a reason why the test below fails. > +            } >              if (httpc_set_header(req, "%s: %s", >                           lua_tostring(L, -2), >                           lua_tostring(L, -1)) < 0) { > diff --git a/test/app-tap/http_client.test.lua b/test/app-tap/http_client.test.lua > index 10a3728f8..d1fde5009 100755 > --- a/test/app-tap/http_client.test.lua > +++ b/test/app-tap/http_client.test.lua