| Message ID | 1389806638-3114-1-git-send-email-peter.maydell@linaro.org |
|---|---|
| State | New |
| Headers | show |
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). 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 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). Rich.
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
Il 15/01/2014 22:56, Peter Maydell ha scritto: > > 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" :-) Which was hard to do only 6 months ago. But now that Alex Bligh implemented all the infrastructure for aio_timer_init, it is fairly easy as Peter's patch and mine show. Paolo
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(+)