[patches] Re: [PATCH] Fix OSX tests and add compile-time check
Yaroslav Dynnikov
yaroslav.dynnikov at tarantool.org
Wed Feb 7 17:21:25 MSK 2018
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;
}
+#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.
+ 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.
+ 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
@@ -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
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.tarantool.org/pipermail/tarantool-patches/attachments/20180207/43579a58/attachment.html>
More information about the Tarantool-patches
mailing list