[tarantool-patches] Re: [PATCH v1 1/1] http: fix httpc auto-managed headers

[Vladimir Davydov]

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?