Re: [patches] Re: [PATCH] Fix OSX tests and add compile-time check

Konstantin Belyavskiy k.belyavskiy at tarantool.org
Wed Feb 7 15:34:09 MSK 2018


branch: gh-3040-http-over-unix-request-opts



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

С уважением,
Konstantin Belyavskiy
k.belyavskiy at tarantool.org
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.tarantool.org/pipermail/tarantool-patches/attachments/20180207/be76916d/attachment.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-Fix-OSX-tests-and-add-compile-time-check.patch
Type: application/x-patch
Size: 4091 bytes
Desc: not available
URL: <https://lists.tarantool.org/pipermail/tarantool-patches/attachments/20180207/be76916d/attachment.bin>


More information about the Tarantool-patches mailing list