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 D638824B70 for ; Tue, 27 Aug 2019 10:30:32 -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 ai9Kl9a2_E_U for ; Tue, 27 Aug 2019 10:30:32 -0400 (EDT) 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 935D024A72 for ; Tue, 27 Aug 2019 10:30:32 -0400 (EDT) Date: Tue, 27 Aug 2019 17:30:16 +0300 From: Alexander Turenko Subject: [tarantool-patches] Re: [PATCH] http: add CURLOPT_ACCEPT_ENCODING option Message-ID: <20190827143015.tqifpjmzi63i7ca7@tkn_work_nb> References: <20190819121230.17621-1-i.kosarev@tarantool.org> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <20190819121230.17621-1-i.kosarev@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: Ilya Kosarev Cc: tarantool-patches@freelists.org, georgy@tarantool.org, i.kosarev@corp.mail.ru I have no objections against the code, but I would describe it in more details. See below. WBR, Alexander Turenko. On Mon, Aug 19, 2019 at 03:12:30PM +0300, Ilya Kosarev wrote: > CURLOPT_ACCEPT_ENCODING option is now supported. > This option enables automatic decompression of HTTP downloads. > See https://curl.haxx.se/libcurl/c/CURLOPT_ACCEPT_ENCODING.html Nit: docbot request is needed. > > Closes #4232 > --- > Branch: https://github.com/tarantool/tarantool/tree/i.kosarev/gh-4232-curlopt-accept-encoding > Issue: https://github.com/tarantool/tarantool/issues/4232 > > src/httpc.c | 13 +++++++++++++ > src/httpc.h | 12 ++++++++++++ > src/lua/httpc.c | 5 +++++ > src/lua/httpc.lua | 2 ++ > 4 files changed, 32 insertions(+) > > diff --git a/src/httpc.c b/src/httpc.c > index 54bc785e1..736184b6b 100644 > --- a/src/httpc.c > +++ b/src/httpc.c > @@ -337,6 +337,19 @@ httpc_set_follow_location(struct httpc_request *req, long follow) > follow); > } > > +void > +httpc_set_accept_encoding(struct httpc_request *req, const char *encoding) > +{ > +#if LIBCURL_VERSION_NUM >= 0x071506 /* CURLOPT_ACCEPT_ENCODING was called > + CURLOPT_ENCODING before libcurl 7.21.6 */ Nit: Move the comment to its own line(s), fit it to 66 symbols or carry it, add a period at the end. It will look so: | /* | * CURLOPT_ACCEPT_ENCODING was called CURLOPT_ENCODING before | * libcurl 7.21.6. | */ | #if LIBCURL_VERSION_NUM >= 0x071506 > + curl_easy_setopt(req->curl_request.easy, CURLOPT_ACCEPT_ENCODING, > + encoding); > +#else > + curl_easy_setopt(req->curl_request.easy, CURLOPT_ENCODING, > + encoding); > +#endif > +} > + It is possible that a user will set something that bundled / system libcurl does not support or a user will misspell one of values. What will going on in the case? As I see from libcurl sources it will just set passed value for the request header. An http server likely will ignore unknown values and will choose an encoding from a value that is spelled corectly (or will send plain http). So everything will work, no error will be reported, but the transfer will be performed w/o requested encoding. I think it worth to verify and mention such details. > int > httpc_execute(struct httpc_request *req, double timeout) > { > diff --git a/src/httpc.h b/src/httpc.h > index c6882a534..d5f4320c5 100644 > --- a/src/httpc.h > +++ b/src/httpc.h > @@ -312,6 +312,18 @@ httpc_set_interface(struct httpc_request *req, const char *interface); > void > httpc_set_follow_location(struct httpc_request *req, long follow); > > +/** > + * Enable automatic decompression of HTTP downloads: set the contents > + * of the Accept-Encoding: header sent in an HTTP request, > + * and enable decoding of a response > + * when a Content-Encoding: header is received. Nit: Fit the comment to 66 symbols; don't carry a line if it can be continued. I would remove colons after the header names, because it confuses at reading. > + * @param req request > + * @param encoding - specify what encoding you'd like. Let's describe the special value: empty string. Let's describe which options are available with our bundled libcurl and which are possibly availiable with system libcurl (it is for custom builds). Let's describe how to set several options. Add this information to a docbot comment too. > + * @see https://curl.haxx.se/libcurl/c/CURLOPT_ACCEPT_ENCODING.html > + */ > +void > +httpc_set_accept_encoding(struct httpc_request *req, const char *encoding); > + > /** > * This function does async HTTP request > * @param request - reference to request object with filled fields > diff --git a/src/lua/httpc.c b/src/lua/httpc.c > index e02adb9fb..a57f08c6c 100644 > --- a/src/lua/httpc.c > +++ b/src/lua/httpc.c > @@ -295,6 +295,11 @@ luaT_httpc_request(lua_State *L) > httpc_set_follow_location(req, lua_toboolean(L, -1)); > lua_pop(L, 1); > > + lua_getfield(L, 5, "accept_encoding"); > + if (!lua_isnil(L, -1)) > + httpc_set_accept_encoding(req, lua_tostring(L, -1)); > + lua_pop(L, 1); > + > if (httpc_execute(req, timeout) != 0) { > httpc_request_delete(req); > return luaT_error(L); > diff --git a/src/lua/httpc.lua b/src/lua/httpc.lua > index 50ff91a36..4b73187c2 100644 > --- a/src/lua/httpc.lua > +++ b/src/lua/httpc.lua > @@ -287,6 +287,8 @@ end > -- 'Location' header that a server sends as part of an > -- 3xx response; > -- > +-- accept_encoding - enables automatic decompression of HTTP downloads; > +-- > -- Returns: > -- { > -- status=NUMBER, > -- > 2.17.1 > Can we test, say, gzip encoding with our test/app-tap/httpd.py server? If so it would be good to add a test.