URL: https://github.com/SSSD/sssd/pull/152 Author: jhrozek Title: #152: Add a tevent wrapper around libcurl's asynchronous interface Action: opened
PR body: """ This patch set adds a generic utility module that uses libcurl's multi interface to allow the caller to talk HTTP using libcurl. The patch set was part of the KCM responder branch, but as it might be useful to unblock pbrezina's development, I'm submitting it separately.
The iobuf module might seem like an overkill for the libcurl tevent wrapper, but please also check the KCM branch during review -- there it proved quite valuable, so I think it makes sense to just reuse the same code in the curl wrapper as well. """
To pull the PR as Git branch: git remote add ghsssd https://github.com/SSSD/sssd git fetch ghsssd pull/152/head:pr152 git checkout pr152
URL: https://github.com/SSSD/sssd/pull/152 Author: jhrozek Title: #152: Add a tevent wrapper around libcurl's asynchronous interface Action: synchronized
To pull the PR as Git branch: git remote add ghsssd https://github.com/SSSD/sssd git fetch ghsssd pull/152/head:pr152 git checkout pr152
URL: https://github.com/SSSD/sssd/pull/152 Title: #152: Add a tevent wrapper around libcurl's asynchronous interface
jhrozek commented: """ I changed three patches a bit: * the build patch now detects if libcurl supports unix sockets and skips building the curl code if not completely. Pavel, you might change this bit once you add non-unix socket support * minor fixes in the tevent wrapper, just for better readability * fixed pep8 issues in tests """
See the full comment at https://github.com/SSSD/sssd/pull/152#issuecomment-278613114
URL: https://github.com/SSSD/sssd/pull/152 Title: #152: Add a tevent wrapper around libcurl's asynchronous interface
jhrozek commented: """ retest this please """
See the full comment at https://github.com/SSSD/sssd/pull/152#issuecomment-279656176
URL: https://github.com/SSSD/sssd/pull/152 Title: #152: Add a tevent wrapper around libcurl's asynchronous interface
jhrozek commented: """ I tried to submit a new job for the centos CI because our CI "passed": http://sssd-ci.duckdns.org/logs/job/62/42/summary.html
(there is an unrelated failure in rawhide, test-uid failed) """
See the full comment at https://github.com/SSSD/sssd/pull/152#issuecomment-279656417
URL: https://github.com/SSSD/sssd/pull/152 Title: #152: Add a tevent wrapper around libcurl's asynchronous interface
pbrezina commented: """ Thank you. So far I'm reading `secrets` code trying to figure out my needs to comment on `curl` implementation. Here are some comments on the `iobuf` module:
1. `struct sss_iobuf` should be opaque 2. I'm expecting `sss_iobuf_init()` to also allocate `iobuf` context. How about:
```c sss_iobuf_init() -> /** * Creates a new buffer with size @size and maximum capacity @max_capacity and copy @data to this buffer. */ struct sss_iobuf *sss_iobuf_init(TALLOC_CTX *mem_ctx, uint8_t *data, size_t size, size_t max_capacity)
/** * Creates a new read-only buffer with size @size and copy @data to this buffer. */ struct sss_iobuf *sss_iobuf_init_readonly(TALLOC_CTX *mem_ctx, uint8_t *data, size_t size, size_t)
sss_iobuf_tc() -> /** * Creates a new empty buffer with initial size @size and maximum capacity @max_capacity. Buffer is initialized to zeros. */ struct sss_iobuf *sss_iobuf_init_empty(TALLOC_CTX *mem_ctx, size_t initial_size, size_t max_capacity) ```
Non-empty buffers should memcpy data.
And here are two requirements for using this as a replacement for `sss_packet`: 1. Maximum capacity should not be always enforced, consider treating `capacity == 0` as unlimited. I will definitely want to replace `sss_packet` at one point and you don't always know total size there.
2. Once capacity is unlimited, multiplying the size by factor two indefinitely to gain enough space to will become dangerous. Can we grow the buffer by small chunks instead? See `sss_packet_grow` for an example.
"""
See the full comment at https://github.com/SSSD/sssd/pull/152#issuecomment-279713381
URL: https://github.com/SSSD/sssd/pull/152 Title: #152: Add a tevent wrapper around libcurl's asynchronous interface
Label: +Changes requested
URL: https://github.com/SSSD/sssd/pull/152 Author: jhrozek Title: #152: Add a tevent wrapper around libcurl's asynchronous interface Action: synchronized
To pull the PR as Git branch: git remote add ghsssd https://github.com/SSSD/sssd git fetch ghsssd pull/152/head:pr152 git checkout pr152
URL: https://github.com/SSSD/sssd/pull/152 Title: #152: Add a tevent wrapper around libcurl's asynchronous interface
jhrozek commented: """ On Tue, Feb 14, 2017 at 05:57:32AM -0800, Pavel Březina wrote:
Thank you. So far I'm reading `secrets` code trying to figure out my needs to comment on `curl` implementation. Here are some comments on the `iobuf` module:
- `struct sss_iobuf` should be opaque
Done. I previously let the structure transparent so that users can allocate an iobuf on stack, but it's true that with sssd's async processing this is only useful in tests.
- I'm expecting `sss_iobuf_init()` to also allocate `iobuf` context. How about:
sss_iobuf_init() -> /** * Creates a new buffer with size @size and maximum capacity @max_capacity and copy @data to this buffer. */ struct sss_iobuf *sss_iobuf_init(TALLOC_CTX *mem_ctx, uint8_t *data, size_t size, size_t max_capacity) /** * Creates a new read-only buffer with size @size and copy @data to this buffer. */ struct sss_iobuf *sss_iobuf_init_readonly(TALLOC_CTX *mem_ctx, uint8_t *data, size_t size, size_t)
OK, I implemented these two.
sss_iobuf_tc() -> /**
- Creates a new empty buffer with initial size @size and maximum capacity @max_capacity. Buffer is initialized to zeros.
*/ struct sss_iobuf *sss_iobuf_init_empty(TALLOC_CTX *mem_ctx, size_t initial_size, size_t max_capacity)
But I didn't see much reason for this one. Either you want to create an empty io buffer for writing, or initialize one with data for reading.
Non-empty buffers should memcpy data. And here are two requirements for using this as a replacement for `sss_packet`: 1. Maximum capacity should not be always enforced, consider treating `capacity == 0` as unlimited. I will definitely want to replace `sss_packet` at one point and you don't always know total size there.
OK, added. The buffer doesn't grow indefinitely, but up to SIZE_MAX/2 which should be enough for everybody(TM) without completely eating up all memory.
- Once capacity is unlimited, multiplying the size by factor two indefinitely to gain enough space to will become dangerous. Can we grow the buffer by small chunks instead? See `sss_packet_grow` for an example.
Well, it's not dangerous if you intent to grow indefinitely and in most cases you should specify a reasonable limit anyway (megabytes maybe).
I'm not dead-set on this, I just don't see the issue I guess.
"""
See the full comment at https://github.com/SSSD/sssd/pull/152#issuecomment-280060216
URL: https://github.com/SSSD/sssd/pull/152 Title: #152: Add a tevent wrapper around libcurl's asynchronous interface
Label: -Changes requested
URL: https://github.com/SSSD/sssd/pull/152 Title: #152: Add a tevent wrapper around libcurl's asynchronous interface
pbrezina commented: """ Hi, I prepared few trivial commits for you to squash if you agree: https://github.com/pbrezina/sssd/commits/curl I tried to push it to your branch but I can't.
On the growing issue. I guess the ideal step depends on the use case. When you build a new message and write small data you can save some allocation if you will grow with fixed chunk (512B sss_packet_grow), having more allocations when you write large data set. When you use multiplication you can have more allocation with small data and lots of empty space with big data. I guess neither is really an issue in our use cases. """
See the full comment at https://github.com/SSSD/sssd/pull/152#issuecomment-280631603
URL: https://github.com/SSSD/sssd/pull/152 Title: #152: Add a tevent wrapper around libcurl's asynchronous interface
jhrozek commented: """ On Fri, Feb 17, 2017 at 03:59:51AM -0800, Pavel Březina wrote:
Hi, I prepared few trivial commits for you to squash if you agree: https://github.com/pbrezina/sssd/commits/curl I tried to push it to your branch but I can't.
Thank you, squashed with a minor fix and pushed a new version.
On the growing issue. I guess the ideal step depends on the use case. When you build a new message and write small data you can save some allocation if you will grow with fixed chunk (512B sss_packet_grow), having more allocations when you write large data set. When you use multiplication you can have more allocation with small data and lots of empty space with big data. I guess neither is really an issue in our use cases.
I would say we should fix this when we see an issue with the current approach, not before :-)
"""
See the full comment at https://github.com/SSSD/sssd/pull/152#issuecomment-281024509
URL: https://github.com/SSSD/sssd/pull/152 Author: jhrozek Title: #152: Add a tevent wrapper around libcurl's asynchronous interface Action: synchronized
To pull the PR as Git branch: git remote add ghsssd https://github.com/SSSD/sssd git fetch ghsssd pull/152/head:pr152 git checkout pr152
URL: https://github.com/SSSD/sssd/pull/152 Title: #152: Add a tevent wrapper around libcurl's asynchronous interface
jhrozek commented: """ Per Pavel's request I moved the headers parameter to the send function so every request can run with different headers. """
See the full comment at https://github.com/SSSD/sssd/pull/152#issuecomment-281094061
URL: https://github.com/SSSD/sssd/pull/152 Author: jhrozek Title: #152: Add a tevent wrapper around libcurl's asynchronous interface Action: synchronized
To pull the PR as Git branch: git remote add ghsssd https://github.com/SSSD/sssd git fetch ghsssd pull/152/head:pr152 git checkout pr152
URL: https://github.com/SSSD/sssd/pull/152 Title: #152: Add a tevent wrapper around libcurl's asynchronous interface
jhrozek commented: """ I also added more comments per Pavel's request. If there are more areas that need to be commented better, please point them out and I will decorate the code some more. """
See the full comment at https://github.com/SSSD/sssd/pull/152#issuecomment-281106344
URL: https://github.com/SSSD/sssd/pull/152 Title: #152: Add a tevent wrapper around libcurl's asynchronous interface
Label: +Changes requested
URL: https://github.com/SSSD/sssd/pull/152 Author: jhrozek Title: #152: Add a tevent wrapper around libcurl's asynchronous interface Action: synchronized
To pull the PR as Git branch: git remote add ghsssd https://github.com/SSSD/sssd git fetch ghsssd pull/152/head:pr152 git checkout pr152
URL: https://github.com/SSSD/sssd/pull/152 Title: #152: Add a tevent wrapper around libcurl's asynchronous interface
jhrozek commented: """ Thank you, hopefully I addressed all concerns. """
See the full comment at https://github.com/SSSD/sssd/pull/152#issuecomment-281941839
URL: https://github.com/SSSD/sssd/pull/152 Title: #152: Add a tevent wrapper around libcurl's asynchronous interface
jhrozek commented: """ @lslebodn could you please check if you agree with how the curlwrap tool is built and detected during the intgcheck target? The intent was to skip building it on old platforms or when building without libcurl. In that case, the integration tests that need the curlwrap tool are skipped. """
See the full comment at https://github.com/SSSD/sssd/pull/152#issuecomment-281942128
URL: https://github.com/SSSD/sssd/pull/152 Title: #152: Add a tevent wrapper around libcurl's asynchronous interface
Label: -Changes requested
URL: https://github.com/SSSD/sssd/pull/152 Title: #152: Add a tevent wrapper around libcurl's asynchronous interface
pbrezina commented: """ Ack from my side. We can push these patches. I will modify the integration to some extend for `secrets` needs. """
See the full comment at https://github.com/SSSD/sssd/pull/152#issuecomment-281951656
URL: https://github.com/SSSD/sssd/pull/152 Title: #152: Add a tevent wrapper around libcurl's asynchronous interface
Label: +Changes requested
URL: https://github.com/SSSD/sssd/pull/152 Author: jhrozek Title: #152: Add a tevent wrapper around libcurl's asynchronous interface Action: synchronized
To pull the PR as Git branch: git remote add ghsssd https://github.com/SSSD/sssd git fetch ghsssd pull/152/head:pr152 git checkout pr152
URL: https://github.com/SSSD/sssd/pull/152 Title: #152: Add a tevent wrapper around libcurl's asynchronous interface
jhrozek commented: """ On Thu, Feb 23, 2017 at 03:10:12AM -0800, lslebodn wrote:
lslebodn commented on this pull request.
@@ -3241,6 +3281,7 @@ intgcheck-prepare:
--without-semanage \ $(INTGCHECK_CONFIGURE_FLAGS); \$(MAKE) $(AM_MAKEFLAGS); \
- $(MAKE) $(AM_MAKEFLAGS) tcurl-test-tool || echo "libcurl tests will be disabled"; \
Ahh, you want to use it in integration tests. Then it should not part of test `check_PROGRAMS` IMHO; there are simpler option add it to `noinst_PROGRAMS` or install it only with integration tests. But we do not have such AM_CONDITION yet
OK, done, added to noinst programs. But I was wondering if the OR condition is too much of a hack. Because if libcurl is not installed, we can't build the tcurl-test-tool binary, we have to either make this call conditional (but wrapping it with BUILD_WITH_LIBCURL didn't work for me) or non-fatal (and there I tried prefixing the rule with '-', but that didn't work either).
"""
See the full comment at https://github.com/SSSD/sssd/pull/152#issuecomment-282322164
URL: https://github.com/SSSD/sssd/pull/152 Title: #152: Add a tevent wrapper around libcurl's asynchronous interface
Label: -Changes requested
URL: https://github.com/SSSD/sssd/pull/152 Author: jhrozek Title: #152: Add a tevent wrapper around libcurl's asynchronous interface Action: synchronized
To pull the PR as Git branch: git remote add ghsssd https://github.com/SSSD/sssd git fetch ghsssd pull/152/head:pr152 git checkout pr152
URL: https://github.com/SSSD/sssd/pull/152 Title: #152: Add a tevent wrapper around libcurl's asynchronous interface
jhrozek commented: """ I squashed in changes @lslebodn suggested for the tests and pushed a new version """
See the full comment at https://github.com/SSSD/sssd/pull/152#issuecomment-283054051
URL: https://github.com/SSSD/sssd/pull/152 Title: #152: Add a tevent wrapper around libcurl's asynchronous interface
Label: +Changes requested
URL: https://github.com/SSSD/sssd/pull/152 Title: #152: Add a tevent wrapper around libcurl's asynchronous interface
lslebodn commented: """ I am sorry but there is some problem with integration test on rhel7. I need to kill the test after 1.5 hour http://sssd-ci.duckdns.org/logs/job/63/49/rhel7/ci.html
and the same problem seems to be in centos ci. https://ci.centos.org/job/sssd-CentOS7/427/console
"""
See the full comment at https://github.com/SSSD/sssd/pull/152#issuecomment-283083177
URL: https://github.com/SSSD/sssd/pull/152 Title: #152: Add a tevent wrapper around libcurl's asynchronous interface
lslebodn commented: """ I am sorry but there is some problem with integration test on rhel7. I need to kill the test after 1.5 hour http://sssd-ci.duckdns.org/logs/job/63/49/rhel7/ci.html
and the same problem seems to be in centos ci. https://ci.centos.org/job/sssd-CentOS7/427/console
"""
See the full comment at https://github.com/SSSD/sssd/pull/152#issuecomment-283083177
URL: https://github.com/SSSD/sssd/pull/152 Title: #152: Add a tevent wrapper around libcurl's asynchronous interface
lslebodn commented: """ On (01/03/17 04:41), Pavel Březina wrote:
pbrezina commented on this pull request.
urls[i],
headers,inbufs[i],5);if (ctx == NULL) {DEBUG(SSSDBG_FATAL_FAILURE, "Could not create request\n");talloc_zfree(tool_ctx);return 1;}tevent_req_set_callback(req, request_done, tool_ctx);- }
- while (tool_ctx->done == false) {
tevent_loop_once(ev);- }
We should not create a deadlock, if there is one its a bug :-)
Yes; but in case of bug in libcurl/somewhere else we should fail after timeout and exit with non-zere value.
It's more friednly for tests rather then waiting for unfinished requests.
LS
"""
See the full comment at https://github.com/SSSD/sssd/pull/152#issuecomment-283330883
URL: https://github.com/SSSD/sssd/pull/152 Author: jhrozek Title: #152: Add a tevent wrapper around libcurl's asynchronous interface Action: synchronized
To pull the PR as Git branch: git remote add ghsssd https://github.com/SSSD/sssd git fetch ghsssd pull/152/head:pr152 git checkout pr152
URL: https://github.com/SSSD/sssd/pull/152 Title: #152: Add a tevent wrapper around libcurl's asynchronous interface
jhrozek commented: """ This was very embarrassing, but the timers computation was wrong. This is the interdiff of the wrapper: ``` iff --git a/src/util/tev_curl.c b/src/util/tev_curl.c index 149d5be..cb6ed2e 100644 --- a/src/util/tev_curl.c +++ b/src/util/tev_curl.c @@ -50,7 +50,7 @@ static bool global_is_curl_initialized; struct tcurl_ctx { struct tevent_context *ev; /* See where we set CURLMOPT_TIMERFUNCTION */ - struct tevent_timer *initial_timer; + struct tevent_timer *process_timer;
/* Since we want the API to be non-blocking, all the transfers use * the curl's multi interface: @@ -423,18 +423,32 @@ static int schedule_fd_processing(CURLM *multi, struct timeval tv = { 0, 0 }; struct tcurl_ctx *tctx = talloc_get_type(userp, struct tcurl_ctx);
+ DEBUG(SSSDBG_TRACE_INTERNAL, "timeout_ms: %ld\n", timeout_ms); + if (timeout_ms == -1) { /* man curlmopt_timerfunction(3) says: * A timeout_ms value of -1 means you should delete your timer. */ - talloc_zfree(tctx->initial_timer); + talloc_zfree(tctx->process_timer); + return 0; }
- tv = tevent_timeval_current_ofs(0, timeout_ms * 10); + tv = tevent_timeval_current_ofs(0, timeout_ms * 1000);
- tctx->initial_timer = tevent_add_timer(tctx->ev, tctx, tv, + /* There is only one timer per multi handle, so it makes sense to cancel + * the previous one. + * + * From https://ec.haxx.se/libcurl-drive-multi-socket.html: + * There is only one timeout for the application to handle for the + * entire multi handle, no matter how many individual easy handles + * that have been added or transfers that are in progress. The timer + * callback will be updated with the current nearest-in-time period to + * wait. + */ + talloc_zfree(tctx->process_timer); + tctx->process_timer = tevent_add_timer(tctx->ev, tctx, tv, check_fd_activity, tctx); - if (tctx->initial_timer == NULL) { + if (tctx->process_timer == NULL) { return -1; }
``` """
See the full comment at https://github.com/SSSD/sssd/pull/152#issuecomment-283444639
URL: https://github.com/SSSD/sssd/pull/152 Title: #152: Add a tevent wrapper around libcurl's asynchronous interface
lslebodn commented: """ seems to work well with el7 http://sssd-ci.duckdns.org/logs/job/63/65/summary.html http://sssd-ci.duckdns.org/logs/job/63/66/summary.html
LS
"""
See the full comment at https://github.com/SSSD/sssd/pull/152#issuecomment-283646496
URL: https://github.com/SSSD/sssd/pull/152 Title: #152: Add a tevent wrapper around libcurl's asynchronous interface
jhrozek commented: """ retest this please """
See the full comment at https://github.com/SSSD/sssd/pull/152#issuecomment-283681560
URL: https://github.com/SSSD/sssd/pull/152 Title: #152: Add a tevent wrapper around libcurl's asynchronous interface
lslebodn commented: """ Update: latest version works well in internal CI; but there is still problem in CentOS infrastructure with `test_secrets.py::test_curlwrap_parallel`. The test didn't finish for some reason.
This is a reason why I would prefer to have timeout in `tcurl_test_tool`. Waiting forever due to some bug is not ideal. """
See the full comment at https://github.com/SSSD/sssd/pull/152#issuecomment-284425655
URL: https://github.com/SSSD/sssd/pull/152 Author: jhrozek Title: #152: Add a tevent wrapper around libcurl's asynchronous interface Action: synchronized
To pull the PR as Git branch: git remote add ghsssd https://github.com/SSSD/sssd git fetch ghsssd pull/152/head:pr152 git checkout pr152
URL: https://github.com/SSSD/sssd/pull/152 Author: jhrozek Title: #152: Add a tevent wrapper around libcurl's asynchronous interface Action: synchronized
To pull the PR as Git branch: git remote add ghsssd https://github.com/SSSD/sssd git fetch ghsssd pull/152/head:pr152 git checkout pr152
URL: https://github.com/SSSD/sssd/pull/152 Author: jhrozek Title: #152: Add a tevent wrapper around libcurl's asynchronous interface Action: synchronized
To pull the PR as Git branch: git remote add ghsssd https://github.com/SSSD/sssd git fetch ghsssd pull/152/head:pr152 git checkout pr152
URL: https://github.com/SSSD/sssd/pull/152 Author: jhrozek Title: #152: Add a tevent wrapper around libcurl's asynchronous interface Action: synchronized
To pull the PR as Git branch: git remote add ghsssd https://github.com/SSSD/sssd git fetch ghsssd pull/152/head:pr152 git checkout pr152
URL: https://github.com/SSSD/sssd/pull/152 Author: jhrozek Title: #152: Add a tevent wrapper around libcurl's asynchronous interface Action: synchronized
To pull the PR as Git branch: git remote add ghsssd https://github.com/SSSD/sssd git fetch ghsssd pull/152/head:pr152 git checkout pr152
URL: https://github.com/SSSD/sssd/pull/152 Title: #152: Add a tevent wrapper around libcurl's asynchronous interface
Label: -Changes requested
URL: https://github.com/SSSD/sssd/pull/152 Title: #152: Add a tevent wrapper around libcurl's asynchronous interface
jhrozek commented: """ OK, I /hope/ I finally fixed the bug. Here is the diff: ``` diff --git a/src/util/tev_curl.c b/src/util/tev_curl.c index d6e4d41..b966a78 100644 --- a/src/util/tev_curl.c +++ b/src/util/tev_curl.c @@ -672,6 +672,16 @@ static errno_t tcurl_add_headers(struct tcurl_http_state *state, } }
+ /* Add a dummy header to suppress libcurl adding Expect 100-continue which + * was causing libcurl to always wait for the internal timeout when sending + * a PUT/PATCH request + */ + state->curl_headers = curl_slist_append(state->curl_headers, "Expect:"); + if (state->curl_headers == NULL) { + DEBUG(SSSDBG_CRIT_FAILURE, "Cannot add the dummy expect header\n"); + return ENOMEM; + } + return EOK; }
@@ -818,6 +828,11 @@ static errno_t tcurl_set_options(struct tcurl_http_state *state, return EIO; }
+ /* Causes libcurl to add a sane Content-Length header */ + crv = curl_easy_setopt(state->http_handle, + CURLOPT_INFILESIZE_LARGE, + sss_iobuf_get_size(state->inbuf)); + ret = tcurl_set_read_options(state); if (ret != EOK) { DEBUG(SSSDBG_CRIT_FAILURE, ```
"""
See the full comment at https://github.com/SSSD/sssd/pull/152#issuecomment-286228328
URL: https://github.com/SSSD/sssd/pull/152 Title: #152: Add a tevent wrapper around libcurl's asynchronous interface
jhrozek commented: """ @lslebodn @pbrezina OK to push?
I admit I don't understand the inner workings of neither libcurl nor the HTTP server, but I noticed that the requests from curl command line do not contain the "expect 100 continue" header but do include a content-length. These two additions fixed all issues I was seeing.. """
See the full comment at https://github.com/SSSD/sssd/pull/152#issuecomment-286228600
URL: https://github.com/SSSD/sssd/pull/152 Author: jhrozek Title: #152: Add a tevent wrapper around libcurl's asynchronous interface Action: synchronized
To pull the PR as Git branch: git remote add ghsssd https://github.com/SSSD/sssd git fetch ghsssd pull/152/head:pr152 git checkout pr152
URL: https://github.com/SSSD/sssd/pull/152 Title: #152: Add a tevent wrapper around libcurl's asynchronous interface
jhrozek commented: """ I squashed one last trivial change that just checks the return value of the option set: ``` diff --git a/src/util/tev_curl.c b/src/util/tev_curl.c index b966a78..233ad2c 100644 --- a/src/util/tev_curl.c +++ b/src/util/tev_curl.c @@ -832,6 +832,12 @@ static errno_t tcurl_set_options(struct tcurl_http_state *state, crv = curl_easy_setopt(state->http_handle, CURLOPT_INFILESIZE_LARGE, sss_iobuf_get_size(state->inbuf)); + if (crv != CURLE_OK) { + DEBUG(SSSDBG_OP_FAILURE, + "Failed to set CURLOPT_INFILESIZE_LARGE [%d]: %s\n", + crv, curl_easy_strerror(crv)); + return EIO; + }
ret = tcurl_set_read_options(state); if (ret != EOK) { ``` """
See the full comment at https://github.com/SSSD/sssd/pull/152#issuecomment-286354583
On (14/03/17 09:34), jhrozek wrote:
URL: https://github.com/SSSD/sssd/pull/152 Title: #152: Add a tevent wrapper around libcurl's asynchronous interface
jhrozek commented: """ I squashed one last trivial change that just checks the return value of the option set:
diff --git a/src/util/tev_curl.c b/src/util/tev_curl.c index b966a78..233ad2c 100644 --- a/src/util/tev_curl.c +++ b/src/util/tev_curl.c @@ -832,6 +832,12 @@ static errno_t tcurl_set_options(struct tcurl_http_state *state, crv = curl_easy_setopt(state->http_handle, CURLOPT_INFILESIZE_LARGE, sss_iobuf_get_size(state->inbuf));
gcc on my machine complains that last argument has wrong type
``` In function ‘tcurl_set_options’, inlined from ‘tcurl_http_send’ at src/util/tev_curl.c:625:9: src/util/tev_curl.c:835:15: error: call to ‘_curl_easy_setopt_err_curl_off_t’ declared with attribute warning: curl_easy_setopt expects a curl_off_t argument for this option [-Werror] crv = curl_easy_setopt(state->http_handle, ^~~~~~~~~~~~~~~~ cc1: all warnings being treated as errors ```
I am not sure whether you have `/usr/include/curl/typecheck-gcc.h` on your machine. I have curl-7.53.1-3.fc26.
if (crv != CURLE_OK) {DEBUG(SSSDBG_OP_FAILURE,"Failed to set CURLOPT_INFILESIZE_LARGE [%d]: %s\n",crv, curl_easy_strerror(crv));return EIO;}
But there are few more warnings In function ‘handle_curlmsg_done’, inlined from ‘process_curl_activity.isra.0’ at src/util/tev_curl.c:252:13: src/util/tev_curl.c:208:11: error: call to ‘_curl_easy_getinfo_err_string’ declared with attribute warning: curl_easy_getinfo expects a pointer to 'char *' for this info [-Werror] crv = curl_easy_getinfo(easy_handle, CURLINFO_PRIVATE, &req); ^~~~~~~~~~~~~~~~~ src/util/tev_curl.c:234:11: error: call to ‘_curl_easy_getinfo_err_long’ declared with attribute warning: curl_easy_getinfo expects a pointer to long for this info [-Werror] crv = curl_easy_getinfo(easy_handle, CURLINFO_RESPONSE_CODE, &state->http_code); ^~~~~~~~~~~~~~~~~
and here is a diff which suppress warnings for me. Not sure whether it is the best solution. ``` diff --git a/src/util/tev_curl.c b/src/util/tev_curl.c index 233ad2c49..ea9987e68 100644 --- a/src/util/tev_curl.c +++ b/src/util/tev_curl.c @@ -94,7 +94,7 @@ struct tcurl_http_state {
/* Output data */ struct sss_iobuf *outbuf; - int http_code; + long http_code; };
static errno_t curl_code2errno(CURLcode crv) @@ -205,7 +205,7 @@ static void handle_curlmsg_done(CURLMsg *message) } }
- crv = curl_easy_getinfo(easy_handle, CURLINFO_PRIVATE, &req); + crv = curl_easy_getinfo(easy_handle, CURLINFO_PRIVATE, (void *)&req); if (crv != CURLE_OK) { DEBUG(SSSDBG_CRIT_FAILURE, "Cannot get CURLINFO_PRIVATE [%d]: %s\n", @@ -831,7 +831,7 @@ static errno_t tcurl_set_options(struct tcurl_http_state *state, /* Causes libcurl to add a sane Content-Length header */ crv = curl_easy_setopt(state->http_handle, CURLOPT_INFILESIZE_LARGE, - sss_iobuf_get_size(state->inbuf)); + (curl_off_t)sss_iobuf_get_size(state->inbuf)); if (crv != CURLE_OK) { DEBUG(SSSDBG_OP_FAILURE, "Failed to set CURLOPT_INFILESIZE_LARGE [%d]: %s\n", ```
On (14/03/17 11:25), Lukas Slebodnik wrote:
On (14/03/17 09:34), jhrozek wrote:
URL: https://github.com/SSSD/sssd/pull/152 Title: #152: Add a tevent wrapper around libcurl's asynchronous interface
jhrozek commented: """ I squashed one last trivial change that just checks the return value of the option set:
diff --git a/src/util/tev_curl.c b/src/util/tev_curl.c index b966a78..233ad2c 100644 --- a/src/util/tev_curl.c +++ b/src/util/tev_curl.c @@ -832,6 +832,12 @@ static errno_t tcurl_set_options(struct tcurl_http_state *state, crv = curl_easy_setopt(state->http_handle, CURLOPT_INFILESIZE_LARGE, sss_iobuf_get_size(state->inbuf));gcc on my machine complains that last argument has wrong type
Ahh, again replied to wrong thread.
LS
URL: https://github.com/SSSD/sssd/pull/152 Title: #152: Add a tevent wrapper around libcurl's asynchronous interface
pbrezina commented: """ The changes looks good, although I would say that disabling `Expect: 100-continue` is not the way to go in a long-term. I believe the problem lies in `secrets` responder that does not handle this properly. I agree that the responder is not supposed to be a full HTTP server, however given the fact that this is default behavior of `libcurl` we should implement it (later).
"""
See the full comment at https://github.com/SSSD/sssd/pull/152#issuecomment-286379622
URL: https://github.com/SSSD/sssd/pull/152 Title: #152: Add a tevent wrapper around libcurl's asynchronous interface
lslebodn commented: """ On (14/03/17 01:34), Jakub Hrozek wrote:
I squashed one last trivial change that just checks the return value of the option set:
diff --git a/src/util/tev_curl.c b/src/util/tev_curl.c index b966a78..233ad2c 100644 --- a/src/util/tev_curl.c +++ b/src/util/tev_curl.c @@ -832,6 +832,12 @@ static errno_t tcurl_set_options(struct tcurl_http_state *state, crv = curl_easy_setopt(state->http_handle, CURLOPT_INFILESIZE_LARGE, sss_iobuf_get_size(state->inbuf));
gcc on my machine complains that last argument has wrong type
``` In function ‘tcurl_set_options’, inlined from ‘tcurl_http_send’ at src/util/tev_curl.c:625:9: src/util/tev_curl.c:835:15: error: call to ‘_curl_easy_setopt_err_curl_off_t’ declared with attribute warning: curl_easy_setopt expects a curl_off_t argument for this option [-Werror] crv = curl_easy_setopt(state->http_handle, ^~~~~~~~~~~~~~~~ cc1: all warnings being treated as errors ```
I am not sure whether you have `/usr/include/curl/typecheck-gcc.h` on your machine. I have curl-7.53.1-3.fc26.
if (crv != CURLE_OK) {DEBUG(SSSDBG_OP_FAILURE,"Failed to set CURLOPT_INFILESIZE_LARGE [%d]: %s\n",crv, curl_easy_strerror(crv));return EIO;} ret = tcurl_set_read_options(state); if (ret != EOK) {
But there are few more warnings In function ‘handle_curlmsg_done’, inlined from ‘process_curl_activity.isra.0’ at src/util/tev_curl.c:252:13: src/util/tev_curl.c:208:11: error: call to ‘_curl_easy_getinfo_err_string’ declared with attribute warning: curl_easy_getinfo expects a pointer to 'char *' for this info [-Werror] crv = curl_easy_getinfo(easy_handle, CURLINFO_PRIVATE, &req); ^~~~~~~~~~~~~~~~~ src/util/tev_curl.c:234:11: error: call to ‘_curl_easy_getinfo_err_long’ declared with attribute warning: curl_easy_getinfo expects a pointer to long for this info [-Werror] crv = curl_easy_getinfo(easy_handle, CURLINFO_RESPONSE_CODE, &state->http_code); ^~~~~~~~~~~~~~~~~
and here is a diff which suppress warnings for me. Not sure whether it is the best solution. ``` diff --git a/src/util/tev_curl.c b/src/util/tev_curl.c index 233ad2c49..ea9987e68 100644 --- a/src/util/tev_curl.c +++ b/src/util/tev_curl.c @@ -94,7 +94,7 @@ struct tcurl_http_state {
/* Output data */ struct sss_iobuf *outbuf; - int http_code; + long http_code; };
static errno_t curl_code2errno(CURLcode crv) @@ -205,7 +205,7 @@ static void handle_curlmsg_done(CURLMsg *message) } }
- crv = curl_easy_getinfo(easy_handle, CURLINFO_PRIVATE, &req); + crv = curl_easy_getinfo(easy_handle, CURLINFO_PRIVATE, (void *)&req); if (crv != CURLE_OK) { DEBUG(SSSDBG_CRIT_FAILURE, "Cannot get CURLINFO_PRIVATE [%d]: %s\n", @@ -831,7 +831,7 @@ static errno_t tcurl_set_options(struct tcurl_http_state *state, /* Causes libcurl to add a sane Content-Length header */ crv = curl_easy_setopt(state->http_handle, CURLOPT_INFILESIZE_LARGE, - sss_iobuf_get_size(state->inbuf)); + (curl_off_t)sss_iobuf_get_size(state->inbuf)); if (crv != CURLE_OK) { DEBUG(SSSDBG_OP_FAILURE, "Failed to set CURLOPT_INFILESIZE_LARGE [%d]: %s\n", ```
LS
"""
See the full comment at https://github.com/SSSD/sssd/pull/152#issuecomment-286381213
URL: https://github.com/SSSD/sssd/pull/152 Title: #152: Add a tevent wrapper around libcurl's asynchronous interface
lslebodn commented: """ BTW `util/sss_iobuf.h` includes itself
following diff fixes it ``` diff --git a/src/tests/cmocka/test_iobuf.c b/src/tests/cmocka/test_iobuf.c index eaee966e5..489cc2c5d 100644 --- a/src/tests/cmocka/test_iobuf.c +++ b/src/tests/cmocka/test_iobuf.c @@ -29,6 +29,7 @@ #include <cmocka.h>
#include "util/sss_iobuf.h" +#include "util/util.h"
static void test_sss_iobuf_read(void **state) { diff --git a/src/util/sss_iobuf.h b/src/util/sss_iobuf.h index e546d5996..eae357a40 100644 --- a/src/util/sss_iobuf.h +++ b/src/util/sss_iobuf.h @@ -5,8 +5,9 @@ #include <stdint.h> #include <errno.h>
-#include "util/util.h" -#include "util/sss_iobuf.h" +#include "util/util_errors.h" + +struct sss_iobuf;
/* * @brief Allocate an empty IO buffer ```
LS
"""
See the full comment at https://github.com/SSSD/sssd/pull/152#issuecomment-286381815
URL: https://github.com/SSSD/sssd/pull/152 Title: #152: Add a tevent wrapper around libcurl's asynchronous interface
jhrozek commented: """ On Tue, Mar 14, 2017 at 03:22:19AM -0700, Pavel Březina wrote:
The changes looks good, although I would say that disabling `Expect: 100-continue` is not the way to go in a long-term.
Would you prefer if this wasn't directly in the tcurl layer, but only in the callers to keep the tcurl layer generic?
I believe the problem lies in `secrets` responder that does not handle this properly. I agree that the responder is not supposed to be a full HTTP server, however given the fact that this is default behavior of `libcurl` we should implement it (later).
Right, so the purpose of the Expect 100: Continue is to send the request without the (potentially large) POST data, let the server validate the request and if it's invalid, let it reply with an error code. If the request is valid, let it send 100: Continue which triggers sending the data to the server.
Which is why there was the potential timeout, libcurl was waiting for the 100: continue which never came..
"""
See the full comment at https://github.com/SSSD/sssd/pull/152#issuecomment-286392184
URL: https://github.com/SSSD/sssd/pull/152 Title: #152: Add a tevent wrapper around libcurl's asynchronous interface
jhrozek commented: """ On Tue, Mar 14, 2017 at 03:29:17AM -0700, lslebodn wrote:
On (14/03/17 01:34), Jakub Hrozek wrote:
I squashed one last trivial change that just checks the return value of the option set:
diff --git a/src/util/tev_curl.c b/src/util/tev_curl.c index b966a78..233ad2c 100644 --- a/src/util/tev_curl.c +++ b/src/util/tev_curl.c @@ -832,6 +832,12 @@ static errno_t tcurl_set_options(struct tcurl_http_state *state, crv = curl_easy_setopt(state->http_handle, CURLOPT_INFILESIZE_LARGE, sss_iobuf_get_size(state->inbuf));gcc on my machine complains that last argument has wrong type
In function ‘tcurl_set_options’, inlined from ‘tcurl_http_send’ at src/util/tev_curl.c:625:9: src/util/tev_curl.c:835:15: error: call to ‘_curl_easy_setopt_err_curl_off_t’ declared with attribute warning: curl_easy_setopt expects a curl_off_t argument for this option [-Werror] crv = curl_easy_setopt(state->http_handle, ^~~~~~~~~~~~~~~~ cc1: all warnings being treated as errorsI am not sure whether you have `/usr/include/curl/typecheck-gcc.h` on your machine. I have curl-7.53.1-3.fc26.
if (crv != CURLE_OK) {DEBUG(SSSDBG_OP_FAILURE,"Failed to set CURLOPT_INFILESIZE_LARGE [%d]: %s\n",crv, curl_easy_strerror(crv));return EIO;} ret = tcurl_set_read_options(state); if (ret != EOK) {But there are few more warnings In function ‘handle_curlmsg_done’, inlined from ‘process_curl_activity.isra.0’ at src/util/tev_curl.c:252:13: src/util/tev_curl.c:208:11: error: call to ‘_curl_easy_getinfo_err_string’ declared with attribute warning: curl_easy_getinfo expects a pointer to 'char *' for this info [-Werror] crv = curl_easy_getinfo(easy_handle, CURLINFO_PRIVATE, &req); ^~~~~~~~~~~~~~~~~ src/util/tev_curl.c:234:11: error: call to ‘_curl_easy_getinfo_err_long’ declared with attribute warning: curl_easy_getinfo expects a pointer to long for this info [-Werror] crv = curl_easy_getinfo(easy_handle, CURLINFO_RESPONSE_CODE, &state->http_code); ^~~~~~~~~~~~~~~~~
and here is a diff which suppress warnings for me. Not sure whether it is the best solution.
diff --git a/src/util/tev_curl.c b/src/util/tev_curl.c index 233ad2c49..ea9987e68 100644 --- a/src/util/tev_curl.c +++ b/src/util/tev_curl.c @@ -94,7 +94,7 @@ struct tcurl_http_state { /* Output data */ struct sss_iobuf *outbuf; - int http_code; + long http_code; }; static errno_t curl_code2errno(CURLcode crv) @@ -205,7 +205,7 @@ static void handle_curlmsg_done(CURLMsg *message) } } - crv = curl_easy_getinfo(easy_handle, CURLINFO_PRIVATE, &req); + crv = curl_easy_getinfo(easy_handle, CURLINFO_PRIVATE, (void *)&req); if (crv != CURLE_OK) { DEBUG(SSSDBG_CRIT_FAILURE, "Cannot get CURLINFO_PRIVATE [%d]: %s\n", @@ -831,7 +831,7 @@ static errno_t tcurl_set_options(struct tcurl_http_state *state, /* Causes libcurl to add a sane Content-Length header */ crv = curl_easy_setopt(state->http_handle, CURLOPT_INFILESIZE_LARGE, - sss_iobuf_get_size(state->inbuf)); + (curl_off_t)sss_iobuf_get_size(state->inbuf)); if (crv != CURLE_OK) { DEBUG(SSSDBG_OP_FAILURE, "Failed to set CURLOPT_INFILESIZE_LARGE [%d]: %s\n",
I think this is fine. Thanks, merged.
"""
See the full comment at https://github.com/SSSD/sssd/pull/152#issuecomment-286392933
URL: https://github.com/SSSD/sssd/pull/152 Title: #152: Add a tevent wrapper around libcurl's asynchronous interface
jhrozek commented: """ On Tue, Mar 14, 2017 at 03:31:45AM -0700, lslebodn wrote:
BTW `util/sss_iobuf.h` includes itself
following diff fixes it
diff --git a/src/tests/cmocka/test_iobuf.c b/src/tests/cmocka/test_iobuf.c index eaee966e5..489cc2c5d 100644 --- a/src/tests/cmocka/test_iobuf.c +++ b/src/tests/cmocka/test_iobuf.c @@ -29,6 +29,7 @@ #include <cmocka.h> #include "util/sss_iobuf.h" +#include "util/util.h" static void test_sss_iobuf_read(void **state) { diff --git a/src/util/sss_iobuf.h b/src/util/sss_iobuf.h index e546d5996..eae357a40 100644 --- a/src/util/sss_iobuf.h +++ b/src/util/sss_iobuf.h @@ -5,8 +5,9 @@ #include <stdint.h> #include <errno.h> -#include "util/util.h" -#include "util/sss_iobuf.h" +#include "util/util_errors.h" + +struct sss_iobuf; /* * @brief Allocate an empty IO buffer
Thanks, merged.
"""
See the full comment at https://github.com/SSSD/sssd/pull/152#issuecomment-286392967
URL: https://github.com/SSSD/sssd/pull/152 Author: jhrozek Title: #152: Add a tevent wrapper around libcurl's asynchronous interface Action: synchronized
To pull the PR as Git branch: git remote add ghsssd https://github.com/SSSD/sssd git fetch ghsssd pull/152/head:pr152 git checkout pr152
URL: https://github.com/SSSD/sssd/pull/152 Title: #152: Add a tevent wrapper around libcurl's asynchronous interface
pbrezina commented: """ On 03/14/2017 12:19 PM, Jakub Hrozek wrote:
On Tue, Mar 14, 2017 at 03:22:19AM -0700, Pavel Březina wrote:
The changes looks good, although I would say that disabling `Expect: 100-continue` is not the way to go in a long-term.
Would you prefer if this wasn't directly in the tcurl layer, but only in the callers to keep the tcurl layer generic?
No, given the use case we can keep it there.
I believe the problem lies in `secrets` responder that does not handle this properly. I agree
that the
responder is not supposed to be a full HTTP server, however given the fact that this is default behavior of `libcurl` we should implement it (later).
Right, so the purpose of the Expect 100: Continue is to send the request without the (potentially large) POST data, let the server validate the request and if it's invalid, let it reply with an error code. If the request is valid, let it send 100: Continue which triggers sending the data to the server.
Which is why there was the potential timeout, libcurl was waiting for the 100: continue which never came..
Yes. And because libcurl will be used by users in their programs to communicate with secrets responder we should document it for the moment and fix it in some future version.
"""
See the full comment at https://github.com/SSSD/sssd/pull/152#issuecomment-286401298
URL: https://github.com/SSSD/sssd/pull/152 Author: jhrozek Title: #152: Add a tevent wrapper around libcurl's asynchronous interface Action: closed
To pull the PR as Git branch: git remote add ghsssd https://github.com/SSSD/sssd git fetch ghsssd pull/152/head:pr152 git checkout pr152
URL: https://github.com/SSSD/sssd/pull/152 Title: #152: Add a tevent wrapper around libcurl's asynchronous interface
lslebodn commented: """ master:
* 91b0592cdab22915dff27ceae6d8e49c608aea4a * ca90f2102a43a3d49a2ef26610d7b4ff3062a823 * 321ca28277cbf9882769537fd4c0dfaea224c86e * 9a9b5e115b079751422be22fd252c0b283611c62 * cab319e2db4b3d85dcadbfdf4c88939df103892e """
See the full comment at https://github.com/SSSD/sssd/pull/152#issuecomment-286407564
URL: https://github.com/SSSD/sssd/pull/152 Title: #152: Add a tevent wrapper around libcurl's asynchronous interface
Label: +Pushed
sssd-devel@lists.fedorahosted.org
-
jhrozek -
lslebodn
-
Lukas Slebodnik -
pbrezina