| Message ID | 1389806638-3114-1-git-send-email-peter.maydell@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
On 15 January 2014 21:37, Richard W.M. Jones <rjones@redhat.com> wrote: > On Wed, Jan 15, 2014 at 05:23:58PM +0000, Peter Maydell wrote: >> libcurl versions 7.16.0 and later have a timer callback interface which >> must be implemented in order for libcurl to make forward progress (it >> will sometimes rely on being called back on the timeout if there are >> no file descriptors registered). Implement the callback, and use a >> QEMU AIO timer to ensure we prod libcurl again when it asks us to. > > Somewhat off topic, but does the curl driver work for you at all? > Especially if you have a disk image on a remote server, but one with > relatively low latency (eg. on a LAN but not localhost). I haven't attempted to use it in anger. I just found that it was hanging completely when I tried a test case for the modules code, investigated a bit, found that libcurl requires this timer callback, implemented it and determined that my test case at least started to boot from the remote image. > We have had endless problems with it, including upstream discussions > with curl people, summarised in this bug: > > https://bugzilla.redhat.com/show_bug.cgi?id=971790 Looking through the thread from upstream, I see they basically said "you need to implement the timer callback" :-) > Just now I tried the latest qemu from git with and without your patch > and still cannot make it work, although the bugginess is now different > from RHBZ#971790. I opened a new bug about what I'm seeing today > (https://bugs.launchpad.net/qemu/+bug/1269606). This should be relatively easy to debug, because that "Error opening file" case happens very early on, in curl_open(), before we even try to use the curl-multi interface or do anything event driven or AIO based, only for the case where something went wrong during the initial "HEAD" operation to get the size and confirm the server supports HTTP ranges. You should be able to just throw a pile of debugging at curl_open() to see what's actually failing. thanks -- PMM
On 15 January 2014 22:06, Paolo Bonzini <pbonzini@redhat.com> wrote: > Il 15/01/2014 18:23, Peter Maydell ha scritto: >> libcurl versions 7.16.0 and later have a timer callback interface which >> must be implemented in order for libcurl to make forward progress (it >> will sometimes rely on being called back on the timeout if there are >> no file descriptors registered). Implement the callback, and use a >> QEMU AIO timer to ensure we prod libcurl again when it asks us to. >> >> Signed-off-by: Peter Maydell <peter.maydell@linaro.org> >> --- >> This fixes the problem I was seeing where trying to use the curl block >> backend just hung. I'm not sure whether all libcurl versions that provide >> the timer callback API require its use, but it shouldn't hurt. > > It still hangs here, but the adding the following patch on top fixes > curl on Fedora for me! Hrm. I wonder why I didn't need to do this bit... > + curl_multi_socket_action(s->multi, CURL_SOCKET_TIMEOUT, 0, &running); The libcurl docs say "This function was added in libcurl 7.15.4, and is deemed stable since 7.16.0. " So if we want to keep supporting pre-7.16 libcurl then we need to retain the multi_socket_all codepath. On the other hand 7.16 was released in October 2006. What's the oldest version we actually care about? thanks -- PMM
On 16 January 2014 08:38, Paolo Bonzini <pbonzini@redhat.com> wrote: > Il 15/01/2014 23:15, Peter Maydell ha scritto: >> >>> > + curl_multi_socket_action(s->multi, CURL_SOCKET_TIMEOUT, 0, &running); >> The libcurl docs say "This function was added in libcurl 7.15.4, and >> is deemed stable since 7.16.0. " So if we want to keep supporting >> pre-7.16 libcurl then we need to retain the multi_socket_all codepath. >> >> On the other hand 7.16 was released in October 2006. What's >> the oldest version we actually care about? > > I say 7.16 :) What dos RHEL5 ship? That's usually our benchmark for "oldest thing we need to support". Ubuntu 10.04 LTS (lucid) and Debian oldstable (squeeze) both ship something more recent than 7.16, so we're OK there. We should probably update the configure test to check for curl_multi_socket_action() rather than curl_multi_setopt(). thanks -- PMM
diff --git a/block/curl.c b/block/curl.c index a603936..3c4c5fc 100644 --- a/block/curl.c +++ b/block/curl.c @@ -34,6 +34,11 @@ #define DPRINTF(fmt, ...) do { } while (0) #endif +#if LIBCURL_VERSION_NUM >= 0x071000 +/* The multi interface timer callback was introduced in 7.16.0 */ +#define NEED_CURL_TIMER_CALLBACK +#endif + #define PROTOCOLS (CURLPROTO_HTTP | CURLPROTO_HTTPS | \ CURLPROTO_FTP | CURLPROTO_FTPS | \ CURLPROTO_TFTP) @@ -77,6 +82,7 @@ typedef struct CURLState typedef struct BDRVCURLState { CURLM *multi; + QEMUTimer timer; size_t len; CURLState states[CURL_NUM_STATES]; char *url; @@ -87,6 +93,23 @@ typedef struct BDRVCURLState { static void curl_clean_state(CURLState *s); static void curl_multi_do(void *arg); +#ifdef NEED_CURL_TIMER_CALLBACK +static int curl_timer_cb(CURLM *multi, long timeout_ms, void *opaque) +{ + BDRVCURLState *s = opaque; + + DPRINTF("CURL: timer callback timeout_ms %ld\n", timeout_ms); + if (timeout_ms == -1) { + timer_del(&s->timer); + } else { + int64_t timeout_ns = (int64_t)timeout_ms * 1000 * 1000; + timer_mod(&s->timer, + qemu_clock_get_ns(QEMU_CLOCK_REALTIME) + timeout_ns); + } + return 0; +} +#endif + static int curl_sock_cb(CURL *curl, curl_socket_t fd, int action, void *s, void *sp) { @@ -473,12 +496,20 @@ static int curl_open(BlockDriverState *bs, QDict *options, int flags, curl_easy_cleanup(state->curl); state->curl = NULL; + aio_timer_init(bdrv_get_aio_context(bs), &s->timer, + QEMU_CLOCK_REALTIME, SCALE_NS, + curl_multi_do, s); + // Now we know the file exists and its size, so let's // initialize the multi interface! s->multi = curl_multi_init(); curl_multi_setopt(s->multi, CURLMOPT_SOCKETDATA, s); curl_multi_setopt(s->multi, CURLMOPT_SOCKETFUNCTION, curl_sock_cb); +#ifdef NEED_CURL_TIMER_CALLBACK + curl_multi_setopt(s->multi, CURLMOPT_TIMERDATA, s); + curl_multi_setopt(s->multi, CURLMOPT_TIMERFUNCTION, curl_timer_cb); +#endif curl_multi_do(s); qemu_opts_del(opts); @@ -597,6 +628,9 @@ static void curl_close(BlockDriverState *bs) } if (s->multi) curl_multi_cleanup(s->multi); + + timer_del(&s->timer); + g_free(s->url); }
libcurl versions 7.16.0 and later have a timer callback interface which must be implemented in order for libcurl to make forward progress (it will sometimes rely on being called back on the timeout if there are no file descriptors registered). Implement the callback, and use a QEMU AIO timer to ensure we prod libcurl again when it asks us to. Signed-off-by: Peter Maydell <peter.maydell@linaro.org> --- This fixes the problem I was seeing where trying to use the curl block backend just hung. I'm not sure whether all libcurl versions that provide the timer callback API require its use, but it shouldn't hurt. This is probably a candidate for stable if it passes code review. block/curl.c | 34 ++++++++++++++++++++++++++++++++++ 1 file changed, 34 insertions(+)