From: Kirill Shcherbatov <kshcherbatov@tarantool.org> To: tarantool-patches@freelists.org, Vladislav Shpilevoy <v.shpilevoy@tarantool.org> Subject: [tarantool-patches] Re: [PATCH v1 1/1] http: fix httpc auto-managed headers Date: Mon, 25 Feb 2019 18:21:31 +0300 [thread overview] Message-ID: <998b800b-acc1-f0c8-1fd3-322d0c04f657@tarantool.org> (raw) In-Reply-To: <bb4e194d-2af1-34bf-5577-8dcce1f0027c@tarantool.org> Hi! Thank you for review. > 1. The email is utterly corrupted - all the indentation is lost. Please, > do not copypaste patches into an email without proper preparations. Ok. > 2. In such a small function it is already clear, that you > do not need this single 'goto error'. Just process that error > here. Done. > 3. Use doxygen style.> 4. I do not understand the name. Is it supposed to be a check? > Then, firstly, for checks we usually use '<object>_is/has_<property>' > naming template. But anyway it is not a check, because it changes > the mask. Come up with a better name, or use, for example, this: > > httpc_set_header_bit /** * Update bitmask of the http request headers that httpc may set * automatically. In case of reserved pattern is found in header, * routine sets corresponding bit in auto_headers_mask. * Returns -1 when header is reserved and it's bit is already set * in auto_headers_mask; 0 otherwise, * @param auto_headers_mask The bitmask of httpc-auto-managed * headers to pointer. * @param header The HTTP header string. * @retval 0 When specified header is not auto-managed or when * corresponding bit was not set in auto_headers_mask. * @retval -1 otherwise. */ static int httpc_set_header_bit(uint8_t *auto_headers_mask, const char *header) > 5. The code below looks like Java. Please, rename to just 'name' > and 'len'.> 6. I do not see any point in storing lengths here, because > anyway you can not use memcmp. Strncasecmp below will check > for terminating zero in both parameters anyway. It is not so, because we must compare just only prefix of specified length. For more details, show 9th comment. > 7. We have lengthof in trivia/utils.h. /* * The sequence of managed headers must be sorted to * stop scan when strcasecmp < 0. The header is expected * to be formated with "%s: %s" pattern, so direct size * verification is redundant. */ struct { const char *name; int len; } managed_headers[] = { {"Accept: ", sizeof("Accept: ") - 1}, {"Connection: ", sizeof("Connection: ") - 1}, {"Content-Length: ", sizeof("Content-Length: ") - 1}, {"Keep-Alive: ", sizeof("Keep-Alive: ") - 1}, }; > 8. Please, do not use non standard types without necessity. > https://github.com/tarantool/tarantool/issues/2161 for (int i = 0; i < managed_headers_cnt; i++) { > 9. As I said at least 2 times for one of previous issues, strncmp does > not work properly without additional checks. Your code will consider > 'Acceptttttt' and 'Accept' to be equal. Please, fix that and add a test. It is not so here, because header is a string that have strong format "%s: %s" I've modified managed_headers array below to use "%s: " prefix. Here It is not a bug, but feature =) > 10. I propose you to store const headers of httpc_managed_headers in a sorted > order. Sorted by the header names. Then, if during an iteration in the cycle > above you see, that strncasecmp() returned < 0, you can return without > further checks. I hope, that you understand why. > > Another option is a binary search, but I think, that it is overkill here. Implemented. > 11. Firstly, it is a method of http_request, and should have > prefix 'http_request_'. But as I see, it is a common style > violation for this library, so lets turn the blind eye to it. > > Secondly, personally I would like 'httpc_set_url', because it > looks shorter. And because 'resource' confused me at a first > glance. > > Thirdly, please, move that function implementation below > httpc_request_new to reduce diff. Done. > 12. Do not put references 'for details' to some internal > functions. They likely to be changed, or renamed, or deleted, > and your reference will be useless. Explain here everything. /** * A bitmask of the http request headers from among * the headers that httpc can set automatically. * Each bit represents some auto-managed HTTP header. * This allows to prevent set them twice. */ uint8_t auto_headers_mask; > 13. > - Please, start each @<option> description with a capital letter, > and finish with a dot; > > - Please, use separate @retval for different return values > description. > > - Do not put double whitespaces in your comments. Here and in other > places too. (I just copied the template of the next comment, they are all like that.) /** * Set HTTP method and destination url, set service headers. * @param req HTTP request object to update. * @param method HTTP request method (GET, HEAD, POST, PUT...). * @param url The network resource URL address string. * @retval 0 On success. * @retval -1 Otherwise. */ int httpc_set_url(struct httpc_request *req, const char *method, const char *url); > >> + */ >> +int >> +httpc_set_resource(struct httpc_request *req, const char *method, >> + const char *url); >> + >> /** >> * Sets body of request >> * @param req request >> diff --git a/test/app-tap/http_client.test.lua b/test/app-tap/http_client.test.lua >> index 10a3728f8..26119fa73 100755 >> --- a/test/app-tap/http_client.test.lua >> +++ b/test/app-tap/http_client.test.lua >> @@ -79,6 +79,15 @@ local function test_http_client(test, url, opts) >> test:is(r.status, 200, 'request') >> end >> >> +local function test_http_client_headers_redefine(test, url, opts) > > 14. Setting a new test out, please, write a comment about what this > test tests, and put a github issue number in it. Conforming our > test comments style. -- -- gh-3955: fix httpc auto-managed headers -- local function test_http_client_headers_redefine(test, url, opts) ============================================ The https library intelligently sets the headers "Accept", "Content-Length", "Connection", "Keep-Alive: timeout". However, when the user explicitly specified them in the header options section of the call argument, they could be written to the HTTP request twice. Was introduced service http c_managed_header_mask mask in which the bits corresponding to the http auto-manager headers, set 1 when referenced. When the bit is set, the new value is not applied. In this way, we prefer user-defined value for all httpc-managed headers. Closes #3955 --- src/httpc.c | 80 ++++++++++++++++++++++++++----- src/httpc.h | 23 +++++++-- src/lua/httpc.c | 45 ++++++++++------- test/app-tap/http_client.test.lua | 16 ++++++- 4 files changed, 131 insertions(+), 33 deletions(-) diff --git a/src/httpc.c b/src/httpc.c index 950f8b32f..96740f950 100644 --- a/src/httpc.c +++ b/src/httpc.c @@ -33,6 +33,7 @@ #include <assert.h> #include <curl/curl.h> +#include <trivia/util.h> #include "fiber.h" #include "errinj.h" @@ -95,8 +96,7 @@ httpc_env_destroy(struct httpc_env *ctx) } struct httpc_request * -httpc_request_new(struct httpc_env *env, const char *method, - const char *url) +httpc_request_new(struct httpc_env *env) { struct httpc_request *req = mempool_alloc(&env->req_pool); if (req == NULL) { @@ -109,9 +109,19 @@ httpc_request_new(struct httpc_env *env, const char *method, region_create(&req->resp_headers, &cord()->slabc); region_create(&req->resp_body, &cord()->slabc); - if (curl_request_create(&req->curl_request) != 0) + if (curl_request_create(&req->curl_request) != 0) { + mempool_free(&env->req_pool, req); return NULL; + } + + ibuf_create(&req->body, &cord()->slabc, 1); + + return req; +} +int +httpc_set_url(struct httpc_request *req, const char *method, const char *url) +{ if (strcmp(method, "GET") == 0) { curl_easy_setopt(req->curl_request.easy, CURLOPT_HTTPGET, 1L); } else if (strcmp(method, "HEAD") == 0) { @@ -130,7 +140,7 @@ httpc_request_new(struct httpc_env *env, const char *method, curl_easy_setopt(req->curl_request.easy, CURLOPT_POSTFIELDSIZE, 0); curl_easy_setopt(req->curl_request.easy, CURLOPT_CUSTOMREQUEST, method); if (httpc_set_header(req, "Accept: */*") < 0) - goto error; + return -1; } else { curl_easy_setopt(req->curl_request.easy, CURLOPT_CUSTOMREQUEST, method); } @@ -146,13 +156,7 @@ httpc_request_new(struct httpc_env *env, const char *method, curl_easy_setopt(req->curl_request.easy, CURLOPT_NOPROGRESS, 1L); curl_easy_setopt(req->curl_request.easy, CURLOPT_HTTP_VERSION, CURL_HTTP_VERSION_1_1); - - ibuf_create(&req->body, &cord()->slabc, 1); - - return req; -error: - mempool_free(&env->req_pool, req); - return NULL; + return 0; } void @@ -170,6 +174,56 @@ httpc_request_delete(struct httpc_request *req) mempool_free(&req->env->req_pool, req); } +/** + * Update bitmask of the http request headers that httpc may set + * automatically. In case of reserved pattern is found in header, + * routine sets corresponding bit in auto_headers_mask. + * Returns -1 when header is reserved and it's bit is already set + * in auto_headers_mask; 0 otherwise, + * @param auto_headers_mask The bitmask of httpc-auto-managed + * headers to pointer. + * @param header The HTTP header string. + * @retval 0 When specified header is not auto-managed or when + * corresponding bit was not set in auto_headers_mask. + * @retval -1 otherwise. +*/ +static int +httpc_set_header_bit(uint8_t *auto_headers_mask, const char *header) +{ + /* + * The sequence of managed headers must be sorted to + * stop scan when strcasecmp < 0. The header is expected + * to be formated with "%s: %s" pattern, so direct size + * verification is redundant. + */ + struct { + const char *name; + int len; + } managed_headers[] = { + {"Accept: ", sizeof("Accept: ") - 1}, + {"Connection: ", sizeof("Connection: ") - 1}, + {"Content-Length: ", sizeof("Content-Length: ") - 1}, + {"Keep-Alive: ", sizeof("Keep-Alive: ") - 1}, + }; + int managed_headers_cnt = lengthof(managed_headers); + assert(managed_headers_cnt < + (int)sizeof(*auto_headers_mask) * CHAR_BIT); + for (int i = 0; i < managed_headers_cnt; i++) { + int rc = strncasecmp(header, managed_headers[i].name, + managed_headers[i].len); + if (rc > 0) + continue; + if (rc < 0) + return 0; + int8_t bit_index = 1 << i; + if ((*auto_headers_mask & bit_index) != 0) + return -1; + *auto_headers_mask |= bit_index; + return 0; + } + return 0; +} + int httpc_set_header(struct httpc_request *req, const char *fmt, ...) { @@ -178,6 +232,10 @@ httpc_set_header(struct httpc_request *req, const char *fmt, ...) const char *header = tt_vsprintf(fmt, ap); va_end(ap); + if (httpc_set_header_bit(&req->auto_headers_mask, header) != 0) { + /* Do not add extra header to list. */ + return 0; + } struct curl_slist *l = curl_slist_append(req->headers, header); if (l == NULL) { diag_set(OutOfMemory, strlen(header), "curl", "http header"); diff --git a/src/httpc.h b/src/httpc.h index dc709e6fa..dc18ffb69 100644 --- a/src/httpc.h +++ b/src/httpc.h @@ -109,16 +109,22 @@ struct httpc_request { struct region resp_headers; /** buffer of body */ struct region resp_body; + /** + * A bitmask of the http request headers from among + * the headers that httpc can set automatically. + * Each bit represents some auto-managed HTTP header. + * This allows to prevent set them twice. + */ + uint8_t auto_headers_mask; }; /** * @brief Create a new HTTP request - * @param ctx - reference to context + * @param env - HTTP client environment. * @return a new HTTP request object */ struct httpc_request * -httpc_request_new(struct httpc_env *env, const char *method, - const char *url); +httpc_request_new(struct httpc_env *env); /** * @brief Delete HTTP request @@ -137,6 +143,17 @@ httpc_request_delete(struct httpc_request *req); int httpc_set_header(struct httpc_request *req, const char *fmt, ...); +/** + * Set HTTP method and destination url, set service headers. + * @param req HTTP request object to update. + * @param method HTTP request method (GET, HEAD, POST, PUT...). + * @param url The network resource URL address string. + * @retval 0 On success. + * @retval -1 Otherwise. + */ +int +httpc_set_url(struct httpc_request *req, const char *method, const char *url); + /** * Sets body of request * @param req request diff --git a/src/lua/httpc.c b/src/lua/httpc.c index 5572b70e9..d743eeb49 100644 --- a/src/lua/httpc.c +++ b/src/lua/httpc.c @@ -145,12 +145,36 @@ luaT_httpc_request(lua_State *L) if (ctx == NULL) return luaL_error(L, "can't get httpc environment"); + struct httpc_request *req = httpc_request_new(ctx); + if (req == NULL) + return luaT_error(L); + + /* + * All user-defined headers must be set before calling + * other httpc module methods so that they take priority + * over the headers that httpc tries to set automatically. + */ + lua_getfield(L, 5, "headers"); + if (!lua_isnil(L, -1)) { + lua_pushnil(L); + while (lua_next(L, -2) != 0) { + if (httpc_set_header(req, "%s: %s", + lua_tostring(L, -2), + lua_tostring(L, -1)) < 0) { + httpc_request_delete(req); + return luaT_error(L); + } + lua_pop(L, 1); + } + } + lua_pop(L, 1); + const char *method = luaL_checkstring(L, 2); const char *url = luaL_checkstring(L, 3); - - struct httpc_request *req = httpc_request_new(ctx, method, url); - if (req == NULL) + if (httpc_set_url(req, method, url) != 0) { + httpc_request_delete(req); return luaT_error(L); + } double timeout = TIMEOUT_INFINITY; int max_header_name_length = HEADER_NAME_LEN; @@ -171,21 +195,6 @@ luaT_httpc_request(lua_State *L) return luaL_error(L, "fifth argument must be a table"); } - lua_getfield(L, 5, "headers"); - if (!lua_isnil(L, -1)) { - lua_pushnil(L); - while (lua_next(L, -2) != 0) { - if (httpc_set_header(req, "%s: %s", - lua_tostring(L, -2), - lua_tostring(L, -1)) < 0) { - httpc_request_delete(req); - return luaT_error(L); - } - lua_pop(L, 1); - } - } - lua_pop(L, 1); - lua_getfield(L, 5, "ca_path"); if (!lua_isnil(L, -1)) httpc_set_ca_path(req, lua_tostring(L, -1)); diff --git a/test/app-tap/http_client.test.lua b/test/app-tap/http_client.test.lua index 10a3728f8..b8573a572 100755 --- a/test/app-tap/http_client.test.lua +++ b/test/app-tap/http_client.test.lua @@ -79,6 +79,18 @@ local function test_http_client(test, url, opts) test:is(r.status, 200, 'request') end +-- +-- gh-3955: Fix httpc auto-managed headers. +-- +local function test_http_client_headers_redefine(test, url, opts) + test:plan(3) + opts.headers={['Connection'] = 'Keep-Alive', ['Accept'] = 'text/html'} + local r = client.get(url, opts) + test:is(r.status, 200, 'simple 200') + test:is(r.headers["accept"], 'text/html', 'redefined Accept header') + test:is(r.headers["connection"], 'Keep-Alive', 'redefined Connection header') +end + local function test_cancel_and_errinj(test, url, opts) test:plan(3) local ch = fiber.channel(1) @@ -397,9 +409,11 @@ local function test_concurrent(test, url, opts) end function run_tests(test, sock_family, sock_addr) - test:plan(9) + test:plan(10) local server, url, opts = start_server(test, sock_family, sock_addr) test:test("http.client", test_http_client, url, opts) + test:test("http.client headers redefine", test_http_client_headers_redefine, + url, opts) test:test("cancel and errinj", test_cancel_and_errinj, url .. 'long_query', opts) test:test("basic http post/get", test_post_and_get, url, opts) test:test("errors", test_errors) -- 2.20.1
next prev parent reply other threads:[~2019-02-25 15:21 UTC|newest] Thread overview: 15+ messages / expand[flat|nested] mbox.gz Atom feed top 2019-02-21 15:09 [tarantool-patches] " Kirill Shcherbatov 2019-02-22 7:39 ` [tarantool-patches] " Kirill Shcherbatov 2019-02-25 7:52 ` Kirill Shcherbatov 2019-02-25 14:17 ` Vladislav Shpilevoy 2019-02-25 15:21 ` Kirill Shcherbatov [this message] 2019-03-07 13:51 ` Vladimir Davydov 2019-03-07 14:07 ` Kirill Shcherbatov 2019-03-07 14:32 ` Vladimir Davydov 2019-03-07 14:48 ` Kirill Shcherbatov 2019-03-07 14:53 ` Vladimir Davydov 2019-03-11 9:23 ` [tarantool-patches] Re: [PATCH v2 " Kirill Shcherbatov 2019-03-11 10:02 ` Vladimir Davydov 2019-03-11 15:57 ` Kirill Shcherbatov 2019-03-12 8:54 ` Vladimir Davydov 2019-03-07 14:07 ` [tarantool-patches] Re: [PATCH v1 " Vladislav Shpilevoy
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=998b800b-acc1-f0c8-1fd3-322d0c04f657@tarantool.org \ --to=kshcherbatov@tarantool.org \ --cc=tarantool-patches@freelists.org \ --cc=v.shpilevoy@tarantool.org \ --subject='[tarantool-patches] Re: [PATCH v1 1/1] http: fix httpc auto-managed headers' \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: link
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox