<HTML><BODY>branch: gh-3040-http-over-unix-request-opts<br><br><br><br><blockquote style="border-left:1px solid #0857A6; margin:10px; padding:0 0 0 10px;">
Среда, 7 февраля 2018, 13:53 +03:00 от Vladimir Davydov <vdavydov.dev@gmail.com>:<br><br><div id=""><div class="js-helper js-readmsg-msg"><div><div id="style_15180008190000000301_BODY">On Tue, Feb 06, 2018 at 08:11:31PM +0300, Konstantin Belyavskiy wrote:<br>
> Add compile-time libcurl version check to make sure it supports<br>
> Unix socket (by using CURL_VERSION_UNIX_SOCKETS macro)<br>
> <br>
> Closes #3040<br>
> ---<br>
> branch: gh-3040-http-over-unix-request-opts<br>
> src/httpc.c | 10 +++++++++-<br>
> src/httpc.h | 3 ++-<br>
> src/lua/httpc.c | 4 +++-<br>
> test/app-tap/http_client.test.lua | 26 +++++++++++++++++++++++---<br>
> 4 files changed, 37 insertions(+), 6 deletions(-)<br>
> <br>
> diff --git a/src/httpc.c b/src/httpc.c<br>
> index 5eb79f00e..f3c6a1c0e 100644<br>
> --- a/src/httpc.c<br>
> +++ b/src/httpc.c<br>
> @@ -263,10 +263,18 @@ httpc_set_ca_file(struct httpc_request *req, const char *ca_file)<br>
> curl_easy_setopt(req->curl_request.easy, CURLOPT_CAINFO, ca_file);<br>
> }<br>
> <br>
> -void<br>
> +int<br>
> httpc_set_unix_socket(struct httpc_request *req, const char *unix_socket)<br>
> {<br>
> +#ifdef CURL_VERSION_UNIX_SOCKETS<br>
> curl_easy_setopt(req->curl_request.easy, CURLOPT_UNIX_SOCKET_PATH, unix_socket);<br>
> + return 0;<br>
> +#else<br>
> +#pragma message "UNIX_SOCKETS are not supported, please upgrade curl to 7.40.0"<br>
> + (void) req;<br>
> + (void) unix_socket;<br>
> + return -1;<br>
> +#endif<br>
> }<br><br>
This is minor, but we usually set diag on error. What about<br>
IllegalParams? Another option - silently ignore the option if<br>
it is not supported, as we do in case of httpc_set_keepalive -<br>
let httpc_execute fail if unix connections are not supported.<br></div></div></div></div></blockquote>Fixed<br><blockquote style="border-left:1px solid #0857A6; margin:10px; padding:0 0 0 10px;"><div id=""><div class="js-helper js-readmsg-msg"><div><div id="style_15180008190000000301_BODY"><br>
> <br>
> void<br><br>
> diff --git a/src/lua/httpc.c b/src/lua/httpc.c<br>
> index 937d15fc8..577e614df 100644<br>
> --- a/src/lua/httpc.c<br>
> +++ b/src/lua/httpc.c<br>
> @@ -124,7 +124,9 @@ luaT_httpc_request(lua_State *L)<br>
> <br>
> lua_getfield(L, 5, "unix_socket");<br>
> if (!lua_isnil(L, -1))<br>
> - httpc_set_unix_socket(req, lua_tostring(L, -1));<br>
> + if(httpc_set_unix_socket(req, lua_tostring(L, -1)))<br>
> + return luaL_error(L, "please upgrade libcurl to 7.40"<br>
> + " to support unix socket");<br><br>
AFAICS httpc_request_delete must be called in case of error.</div></div></div></div></blockquote>Fixed<br><blockquote style="border-left:1px solid #0857A6; margin:10px; padding:0 0 0 10px;"><div id=""><div class="js-helper js-readmsg-msg"><div><div id="style_15180008190000000301_BODY"><br><br>
> lua_pop(L, 1);<br>
> <br>
> lua_getfield(L, 5, "verify_host");<br>
> diff --git a/test/app-tap/http_client.test.lua b/test/app-tap/http_client.test.lua<br>
> index 274ac4d1f..6289b5b92 100755<br>
> --- a/test/app-tap/http_client.test.lua<br>
> +++ b/test/app-tap/http_client.test.lua<br>
> @@ -391,7 +391,20 @@ test:test('http over AF_INET', function(test)<br>
> end)<br>
> <br>
> test:test('http over AF_UNIX', function(test)<br>
> - test:plan(8)<br>
> + local url = "http://localhost/"<br>
> + local tmpname = os.tmpname()<br>
> + local sock = tmpname .. '.sock'<br>
> + local check = pcall(client.get, url, {unix_socket=sock, timeout = 0.01})<br>
> + if check == false then<br>
> + os.remove(tmpname)<br>
> + return<br>
> + end<br>
> +<br>
> + local extra = 1<br>
> + if jit.os == "OSX" then<br>
> + extra = 0<br>
> + end<br>
> + test:plan(extra + 7)<br>
> <br>
> local tmpname = os.tmpname()<br>
> local sock = tmpname .. '.sock'<br>
> @@ -400,9 +413,14 @@ test:test('http over AF_UNIX', function(test)<br>
> local cmd = string.format("%s/test/app-tap/httpd.py --unix %s",<br>
> TARANTOOL_SRC_DIR, sock)<br>
> local server = io.popen(cmd)<br>
> + if server == nil then<br>
> + test:diag("create server failes")<br>
> + os.remove(tmpname)<br>
> + os.remove(sock)<br>
> + return<br>
> + end<br>
> test:is(server:read("*l"), "heartbeat", "server started")<br>
> <br>
> - local url = "http://localhost/"<br>
> test:diag("trying to connect to %s", url)<br>
> local r<br>
> for i=1,10 do<br>
> @@ -428,7 +446,9 @@ test:test('http over AF_UNIX', function(test)<br>
> test:test("basic http post/get", post_and_get_test, url, opts)<br>
> test:test("headers", headers_test, url, opts)<br>
> test:test("special methods", special_methods_test, url, opts)<br>
> - test:test("concurrent", concurrent_test, url, opts)<br>
> + if jit.os ~= "OSX" then<br>
> + test:test("concurrent", concurrent_test, url, opts)<br>
> + end<br><br>
Why is that? At least, it deserves a comment.<br></div></div></div></div></blockquote>Add comment<br><blockquote style="border-left:1px solid #0857A6; margin:10px; padding:0 0 0 10px;"><div id=""><div class="js-helper js-readmsg-msg"><div><div id="style_15180008190000000301_BODY"><br>
Anyway, I think the series is split in patches incorrectly.<br>
Currently, we have on the branch:<br><br>
19e911d3 Fix OSX tests and add compile-time check<br>
9c607ad4 Refactor http.client autotests<br>
e766c6b3 Allow http client connection to use unix socket<br><br>
Patch #1 (the last one on the list above) introduces the http-over-unix<br>
feature, but doesn't add a test. Besides, it doesn't even work if curl<br>
is older than a certain version. That is, the patch is incomplete.<br><br>
Patch #2 not just refactors the test, as the comment claims, but also<br>
partially adds a test for patch #1, which is confusing.<br><br>
Patch #3 is a fix for patch #1 plus the remnants of the test.<br><br>
How I see the series: patch #1 prepares the http client test for testing<br>
http-over-unix without adding any new cases; patch #2 introduces the<br>
feature with a test and without any bugs.<br></div></div></div></div></blockquote>
I can merge them in one, OK?<br>
<br>С уважением,<br>Konstantin Belyavskiy<br>k.belyavskiy@tarantool.org<br></BODY></HTML>