Re[2]: [patches] Re: [PATCH] Fix OSX tests and add compile-time check
Konstantin Belyavskiy
k.belyavskiy at tarantool.org
Wed Feb 7 18:57:49 MSK 2018
>Среда, 7 февраля 2018, 17:21 +03:00 от Yaroslav Dynnikov < yaroslav.dynnikov at tarantool.org >:
>
>branch: gh-3040-http-over-unix-request-opts
>
>>From d00b44afbfab06beb98495991e267bf10e53f334 Mon Sep 17 00:00:00 2001
>>From: Konstantin Belyavskiy < k.belyavskiy at tarantool.org >
>>Date: Tue, 6 Feb 2018 19:27:37 +0300
>>Subject: [PATCH] Fix OSX tests and add compile-time check
>>
>>Add compile-time libcurl version check to make sure it supports
>>Unix socket (by using CURL_VERSION_UNIX_SOCKETS macro)
>>
>>Closes #3040
>>---
>> src/httpc.c | 12 +++++++++++-
>> src/httpc.h | 3 ++-
>> src/lua/httpc.c | 5 ++++-
>> test/app-tap/http_client.test.lua | 22 ++++++++++++++++++++--
>> 4 files changed, 37 insertions(+), 5 deletions(-)
>>
>>diff --git a/src/httpc.c b/src/httpc.c
>>index 5eb79f00e..bba5e8cde 100644
>>--- a/src/httpc.c
>>+++ b/src/httpc.c
>>@@ -263,10 +263,20 @@ 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;
>This part still doesnt look consistent. Why is the curl_easy_setopt error ignored?
>My proposal:
>if (CURLE_OK == curl_easy_setopt(...)) {
> return 0;
>} else {
> return -1;
>}
Decided to do everything the same way. Every function here shadow curl errors. Error handling could be done .. but as a separate commit and in all similar places.
>
>>+#else
>>+#pragma message "UNIX_SOCKETS are not supported, please upgrade curl to 7.40.0"
>>+ (void) req;
>>+ (void) unix_socket;
>>+ diag_set(IllegalParams, "please upgrade libcurl to 7.40"
>>+ " to support unix socket");
>diag_set message is irrelevant to runtime. Yet pragma is OK.
>My proposal: "tarantool was build without unix socket support"
>Because upgrading libcurl won't change anything in runtime.
It's very rare situation then someone got Tarantool binary build with old curl but possible. Update warning diag_set message.
>
>
>>+ return -1;
>>+#endif
>> }
>>
>> void
>>diff --git a/src/httpc.h b/src/httpc.h
>>index 8e85bd905..6e3267f2d 100644
>>--- a/src/httpc.h
>>+++ b/src/httpc.h
>>@@ -227,8 +227,9 @@ httpc_set_ca_file(struct httpc_request *req, const char *ca_file);
>> * endpoint instead of TCP. The application does not have to keep the string
>> * around after setting this option.
>> * @see https://curl.haxx.se/libcurl/c/CURLOPT_UNIX_SOCKET_PATH.html
>>+ * @return 0 on success
>Could you please mention @return -1 if the option is not supported
>
>> */
>>-void
>>+int
>> httpc_set_unix_socket(struct httpc_request *req, const char *unix_socket);
>>
>> /**
>>diff --git a/src/lua/httpc.c b/src/lua/httpc.c
>>index 937d15fc8..a217e3c59 100644
>>--- a/src/lua/httpc.c
>>+++ b/src/lua/httpc.c
>>@@ -124,7 +124,10 @@ 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))) {
>>+ httpc_request_delete(req);
>>+ return luaT_error(L);
>>+ }
>> 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..c0a66946f 100755
>>--- a/test/app-tap/http_client.test.lua
>>+++ b/test/app-tap/http_client.test.lua
>>@@ -391,6 +391,15 @@ test:test('http over AF_INET', function(test)
>> end)
>>
>> test:test('http over AF_UNIX', function(test)
>>+ local url = " http://localhost/ "
>>+ local tmpname = os.tmpname()
>>+ local sock = tmpname .. '.sock'
>>+ local ok = pcall(client.get, url, {unix_socket=sock, timeout = 0.01})
>>+ if ok == false then
>I don't like this check. It states that "if client.get FAILS for ANY reason the check is marked as PASSED". But it is not.
I think it's ok. Probability of such case is low. It could be done but I don't think it's really needed.
>
>
>>+ os.remove(tmpname)
>>+ return
>>+ end
>>+
>> test:plan(8)
>>
>> local tmpname = os.tmpname()
>Don't override tmpname or the previous file will not be deleted from tmp
fixed
>
>
>>@@ -400,9 +409,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")
>typo: "server creation failed"
>
>>+ 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 +442,11 @@ 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 -- due to difference in OSX Unix socket, skip
>>+ test:skip("concurrent")
>>+ else
>>+ test:test("concurrent", concurrent_test, url, opts)
>>+ end
>Agree
>
>>
>> -- STOP SERVER
>> test:diag("stopping HTTP server")
>>--
>>2.14.3 (Apple Git-98)
>
>
>
>On 7 February 2018 at 15:34, Konstantin Belyavskiy < k.belyavskiy at tarantool.org > wrote:
>>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?
>I agree, merge them all in one when you finish.
>>
>>С уважением,
>>Konstantin Belyavskiy
>>k.belyavskiy at tarantool.org
>
С уважением,
Konstantin Belyavskiy
k.belyavskiy at tarantool.org
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.tarantool.org/pipermail/tarantool-patches/attachments/20180207/7d38f005/attachment.html>
More information about the Tarantool-patches
mailing list