From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Date: Thu, 7 Mar 2019 16:51:35 +0300 From: Vladimir Davydov Subject: Re: [tarantool-patches] Re: [PATCH v1 1/1] http: fix httpc auto-managed headers Message-ID: <20190307135135.dner6r66szejw6lx@esperanza> References: <9099f8a339c278e3a5dac683923d13c7ee470ce9.1550761676.git.kshcherbatov@tarantool.org> <80130a1b-84ad-5f90-9422-95bb0a1e67cb@tarantool.org> <998b800b-acc1-f0c8-1fd3-322d0c04f657@tarantool.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <998b800b-acc1-f0c8-1fd3-322d0c04f657@tarantool.org> To: Kirill Shcherbatov Cc: tarantool-patches@freelists.org, Vladislav Shpilevoy List-ID: On Mon, Feb 25, 2019 at 06:21:31PM +0300, Kirill Shcherbatov wrote: > diff --git a/src/httpc.c b/src/httpc.c > index 950f8b32f..96740f950 100644 > --- a/src/httpc.c > +++ b/src/httpc.c > @@ -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}, - A space after a colon (:) isn't mandatory according to the http protocol, e.g. "content-length:10" is a valid header. - I don't quite like string literal duplication. Can we do without it? Use strlen, may be? The compiler should resolve it at compile time AFAIK. - Checking "content-length" doesn't make much sense IMO. No-one in their right mind will overwrite it after setting a body. We only need to handle "accept", "connection", "keep-alive". > + }; > + 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); > + I don't like how httpc internal API now looks like: one should set headers before the method/url and keepalive options, otherwise he/she risks loosing them. And there isn't a word about this idiosyncrasy anywhere in the comments. Also, splitting request construction into allocation (_new) and method/url initialization (_set_url) looks ugly to me, because no request makes sense without them. May be, we could set default headers in httpc_execute instead?