[Qemu-devel] [PATCH v7 00/13] curl: fix curl read

[Qemu-devel] [PATCH v7 00/13] curl: fix curl read

[Fam Zheng]

CURL library API has changed, the current curl driver is not working with
current libcurl. It may or may not have worked with old libcurl, but currently,
when testing with apache http URL, it just hangs before fetching any data.  The
problem is because the mismatch of our code and how libcurl wants to be used.
(man 3 curl_multi_socket_action, section 'TYPICAL USAGE')

 - Step 3. We need timer to support libcurl timeout.
 - Step 6. We'll call the right API function, replacing the deprecated.
 - Step 5, 8. Manage socket properly (take actions on socket fd that are passed
   into socket callback from libcurl)

This patch rewrites the use of API as well as the structure of internal states:

BDRVCURLState holds the pointer to curl multi interface (man 3
libcurl-multi), and 4 lists for internal states:
 - CURLState holds state for libcurl connection (man 3 libcurl-easy)
 - CURLSockInfo holds information for libcurl socket interface (man 3
   curl_multi_socket_action).
 - CURLDataCache holds the user data read from libcurl, it is in a list
   ordered by access, the used cache is moved to list head on access, so
   the tail element is freed first. BDRVCURLState.cache_quota is the
   threshold to start freeing cache.
 - CURLAIOCB holds ongoing aio information.

v7:
  13: Added:
      curl: change timeout to 30 seconds

v6:
  05: Rename bs to s for BDRVCURLState.
  06: Use int64_t for offsets.
      Fix printf format string.
      Move introducing of use_count to 07.
  07: Drop explicit cast.
      Use int64_t for offsets.
      Use_count moved here.
  08: Remove duplicated.
      Move s->url = NULL to separate patch.
  09: Fix while(0);
  12: Added:
      curl: set s->url to NULL after free.



Fam Zheng (12):
  curl: introduce CURLSockInfo to BDRVCURLState.
  curl: change magic number to sizeof
  curl: change curl_multi_do to curl_fd_handler
  curl: fix curl_open
  curl: add timer to BDRVCURLState
  curl: introduce CURLDataCache
  curl: make use of CURLDataCache.
  curl: use list to store CURLState
  curl: add cache quota.
  curl: introduce ssl_no_cert runtime option.
  curl: set s->url to NULL after free.
  curl: change timeout to 30 seconds

Richard W.M. Jones (1):
  block/curl.c: Refuse to open the handle for writes.

 block/curl.c | 568 +++++++++++++++++++++++++++++++++++++----------------------
 1 file changed, 353 insertions(+), 215 deletions(-)

-- 
1.8.3

[Qemu-devel] [PATCH v7 01/13] curl: introduce CURLSockInfo to BDRVCURLState.

[Fam Zheng]

We use socket provided by curl in the driver.  Libcurl multi interface
has option CURLMOPT_SOCKETFUNCTION for socket.

Per man 3 curl_multi_setopt:

    ...
    CURLMOPT_SOCKETFUNCTION

    Pass a pointer to a function matching the curl_socket_callback
    prototype. The curl_multi_socket_action(3) function informs the
    application about updates in the socket (file descriptor) status by
    doing none, one, or multiple calls to the curl_socket_callback given in
    the param argument. They update the status with changes since the
    previous time a curl_multi_socket(3) function was called. If the given
    callback pointer is NULL, no callback will be called. Set the callback's
    userp argument with CURLMOPT_SOCKETDATA. See curl_multi_socket(3) for
    more callback details.
    ...

The added structure store information for all the socket returned by
libcurl. The most important field is action, which is used to keep what
actions are needed when this socket's fd handler is called.

CURLSockInfo is added here and used in later commits to save the socket
actions.

Signed-off-by: Fam Zheng <famz@redhat.com>
---
 block/curl.c | 33 ++++++++++++++++++++++++++++++---
 1 file changed, 30 insertions(+), 3 deletions(-)

diff --git a/block/curl.c b/block/curl.c
index b8935fd..a829fe7 100644
--- a/block/curl.c
+++ b/block/curl.c
@@ -75,10 +75,18 @@ typedef struct CURLState
     char in_use;
 } CURLState;
 
+typedef struct CURLSockInfo {
+    curl_socket_t fd;
+    int action;
+    struct BDRVCURLState *s;
+    QLIST_ENTRY(CURLSockInfo) next;
+} CURLSockInfo;
+
 typedef struct BDRVCURLState {
     CURLM *multi;
     size_t len;
     CURLState states[CURL_NUM_STATES];
+    QLIST_HEAD(, CURLSockInfo) socks;
     char *url;
     size_t readahead_size;
 } BDRVCURLState;
@@ -88,9 +96,18 @@ static void curl_multi_do(void *arg);
 static int curl_aio_flush(void *opaque);
 
 static int curl_sock_cb(CURL *curl, curl_socket_t fd, int action,
-                        void *s, void *sp)
+                        void *userp, void *sockp)
 {
+    BDRVCURLState *s = userp;
     DPRINTF("CURL (AIO): Sock action %d on fd %d\n", action, fd);
+    CURLSockInfo *sock = sockp;
+    if (!sock) {
+        sock = g_malloc0(sizeof(CURLSockInfo));
+        sock->fd = fd;
+        sock->s = s;
+        QLIST_INSERT_HEAD(&s->socks, sock, next);
+        curl_multi_assign(s->multi, fd, sock);
+    }
     switch (action) {
         case CURL_POLL_IN:
             qemu_aio_set_fd_handler(fd, curl_multi_do, NULL, curl_aio_flush, s);
@@ -433,6 +450,8 @@ static int curl_open(BlockDriverState *bs, QDict *options, int flags)
         inited = 1;
     }
 
+    QLIST_INIT(&s->socks);
+
     DPRINTF("CURL: Opening %s\n", file);
     s->url = g_strdup(file);
     state = curl_init_state(s);
@@ -462,8 +481,8 @@ static int curl_open(BlockDriverState *bs, QDict *options, int flags)
     // 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 ); 
+    curl_multi_setopt(s->multi, CURLMOPT_SOCKETDATA, s);
+    curl_multi_setopt(s->multi, CURLMOPT_SOCKETFUNCTION, curl_sock_cb);
     curl_multi_do(s);
 
     qemu_opts_del(opts);
@@ -603,6 +622,14 @@ static void curl_close(BlockDriverState *bs)
     }
     if (s->multi)
         curl_multi_cleanup(s->multi);
+
+    while (!QLIST_EMPTY(&s->socks)) {
+        CURLSockInfo *sock = QLIST_FIRST(&s->socks);
+        QLIST_REMOVE(sock, next);
+        g_free(sock);
+        sock = NULL;
+    }
+
     g_free(s->url);
 }
 
-- 
1.8.3

[Qemu-devel] [PATCH v7 02/13] curl: change magic number to sizeof

[Fam Zheng]

String field length is duplicated in two places. Make it a sizeof.

Signed-off-by: Fam Zheng <famz@redhat.com>
---
 block/curl.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/block/curl.c b/block/curl.c
index a829fe7..a11002b 100644
--- a/block/curl.c
+++ b/block/curl.c
@@ -569,7 +569,7 @@ static void curl_readv_bh_cb(void *p)
     state->orig_buf = g_malloc(state->buf_len);
     state->acb[0] = acb;
 
-    snprintf(state->range, 127, "%zd-%zd", start, end);
+    snprintf(state->range, sizeof(state->range) - 1, "%zd-%zd", start, end);
     DPRINTF("CURL (AIO): Reading %d at %zd (%s)\n",
             (acb->nb_sectors * SECTOR_SIZE), start, state->range);
     curl_easy_setopt(state->curl, CURLOPT_RANGE, state->range);
-- 
1.8.3

[Qemu-devel] [PATCH v7 03/13] curl: change curl_multi_do to curl_fd_handler

[Fam Zheng]

The driver calls curl_multi_do to take action at several points, while
it's also registered as socket fd handler. This patch removes internal
call of curl_multi_do because they are not necessary when handler can be
called by socket data update.

Since curl_multi_do becomes a pure fd handler, the function is renamed.
It takes a pointer to CURLSockInfo now, instead of pointer to
BDRVCURLState.

Signed-off-by: Fam Zheng <famz@redhat.com>
---
 block/curl.c | 28 +++++++++++++++++-----------
 1 file changed, 17 insertions(+), 11 deletions(-)

diff --git a/block/curl.c b/block/curl.c
index a11002b..82a7a30 100644
--- a/block/curl.c
+++ b/block/curl.c
@@ -92,7 +92,7 @@ typedef struct BDRVCURLState {
 } BDRVCURLState;
 
 static void curl_clean_state(CURLState *s);
-static void curl_multi_do(void *arg);
+static void curl_fd_handler(void *arg);
 static int curl_aio_flush(void *opaque);
 
 static int curl_sock_cb(CURL *curl, curl_socket_t fd, int action,
@@ -110,17 +110,23 @@ static int curl_sock_cb(CURL *curl, curl_socket_t fd, int action,
     }
     switch (action) {
         case CURL_POLL_IN:
-            qemu_aio_set_fd_handler(fd, curl_multi_do, NULL, curl_aio_flush, s);
+            qemu_aio_set_fd_handler(fd, curl_fd_handler, NULL,
+                                    curl_aio_flush, sock);
+            sock->action |= CURL_CSELECT_IN;
             break;
         case CURL_POLL_OUT:
-            qemu_aio_set_fd_handler(fd, NULL, curl_multi_do, curl_aio_flush, s);
+            qemu_aio_set_fd_handler(fd, NULL, curl_fd_handler, curl_aio_flush,
+                                    sock);
+            sock->action |= CURL_CSELECT_OUT;
             break;
         case CURL_POLL_INOUT:
-            qemu_aio_set_fd_handler(fd, curl_multi_do, curl_multi_do,
-                                    curl_aio_flush, s);
+            qemu_aio_set_fd_handler(fd, curl_fd_handler, curl_fd_handler,
+                                    curl_aio_flush, sock);
+            sock->action |= CURL_CSELECT_IN | CURL_CSELECT_OUT;
             break;
         case CURL_POLL_REMOVE:
             qemu_aio_set_fd_handler(fd, NULL, NULL, NULL, NULL);
+            sock->action = 0;
             break;
     }
 
@@ -226,9 +232,10 @@ static int curl_find_buf(BDRVCURLState *s, size_t start, size_t len,
     return FIND_RET_NONE;
 }
 
-static void curl_multi_do(void *arg)
+static void curl_fd_handler(void *arg)
 {
-    BDRVCURLState *s = (BDRVCURLState *)arg;
+    CURLSockInfo *sock = (CURLSockInfo *)arg;
+    BDRVCURLState *s = sock->s;
     int running;
     int r;
     int msgs_in_queue;
@@ -237,7 +244,9 @@ static void curl_multi_do(void *arg)
         return;
 
     do {
-        r = curl_multi_socket_all(s->multi, &running);
+        r = curl_multi_socket_action(s->multi,
+                sock->fd, sock->action,
+                &running);
     } while(r == CURLM_CALL_MULTI_PERFORM);
 
     /* Try to find done transfers, so we can free the easy
@@ -302,7 +311,6 @@ static CURLState *curl_init_state(BDRVCURLState *s)
         }
         if (!state) {
             g_usleep(100);
-            curl_multi_do(s);
         }
     } while(!state);
 
@@ -483,7 +491,6 @@ static int curl_open(BlockDriverState *bs, QDict *options, int flags)
     s->multi = curl_multi_init();
     curl_multi_setopt(s->multi, CURLMOPT_SOCKETDATA, s);
     curl_multi_setopt(s->multi, CURLMOPT_SOCKETFUNCTION, curl_sock_cb);
-    curl_multi_do(s);
 
     qemu_opts_del(opts);
     return 0;
@@ -575,7 +582,6 @@ static void curl_readv_bh_cb(void *p)
     curl_easy_setopt(state->curl, CURLOPT_RANGE, state->range);
 
     curl_multi_add_handle(s->multi, state->curl);
-    curl_multi_do(s);
 
 }
 
-- 
1.8.3

[Qemu-devel] [PATCH v7 04/13] curl: fix curl_open

[Fam Zheng]

Change curl_size_cb to curl_header_cb, as what the function is really
doing. Fix the registering, CURLOPT_WRITEFUNCTION is apparently wrong,
should be CURLOPT_HEADERFUNCTION.

Parsing size from header is not necessary as we're using

  curl_easy_getinfo(state->curl, CURLINFO_CONTENT_LENGTH_DOWNLOAD, &d)

The validity of d is version dependant per man 3 curl_easy_getinfo:

    ...
    CURLINFO_CONTENT_LENGTH_DOWNLOAD

    Pass a pointer to a double to receive the content-length of the
    download. This is the value read from the Content-Length: field.
    Since 7.19.4, this returns -1 if the size isn't known.
    ...

And Accept-Ranges is essential for curl to fetch right data from
http/https servers, so we check for this in the header and fail the open
if it's not supported.

Signed-off-by: Fam Zheng <famz@redhat.com>
---
 block/curl.c | 44 ++++++++++++++++++++++++++++++++------------
 1 file changed, 32 insertions(+), 12 deletions(-)

diff --git a/block/curl.c b/block/curl.c
index 82a7a30..d238489 100644
--- a/block/curl.c
+++ b/block/curl.c
@@ -89,6 +89,8 @@ typedef struct BDRVCURLState {
     QLIST_HEAD(, CURLSockInfo) socks;
     char *url;
     size_t readahead_size;
+    /* Whether http server accept range in header */
+    bool accept_range;
 } BDRVCURLState;
 
 static void curl_clean_state(CURLState *s);
@@ -133,14 +135,14 @@ static int curl_sock_cb(CURL *curl, curl_socket_t fd, int action,
     return 0;
 }
 
-static size_t curl_size_cb(void *ptr, size_t size, size_t nmemb, void *opaque)
+static size_t curl_header_cb(void *ptr, size_t size, size_t nmemb, void *opaque)
 {
-    CURLState *s = ((CURLState*)opaque);
+    BDRVCURLState *s = (BDRVCURLState *)opaque;
     size_t realsize = size * nmemb;
-    size_t fsize;
+    const char *accept_line = "Accept-Ranges: bytes";
 
-    if(sscanf(ptr, "Content-Length: %zd", &fsize) == 1) {
-        s->s->len = fsize;
+    if (strncmp((char *)ptr, accept_line, strlen(accept_line)) == 0) {
+        s->accept_range = true;
     }
 
     return realsize;
@@ -428,6 +430,7 @@ static int curl_open(BlockDriverState *bs, QDict *options, int flags)
     Error *local_err = NULL;
     const char *file;
     double d;
+    int running;
 
     static int inited = 0;
 
@@ -469,16 +472,29 @@ static int curl_open(BlockDriverState *bs, QDict *options, int flags)
     // Get file size
 
     curl_easy_setopt(state->curl, CURLOPT_NOBODY, 1);
-    curl_easy_setopt(state->curl, CURLOPT_WRITEFUNCTION, (void *)curl_size_cb);
-    if (curl_easy_perform(state->curl))
+    curl_easy_setopt(state->curl, CURLOPT_HEADERFUNCTION,
+                     curl_header_cb);
+    curl_easy_setopt(state->curl, CURLOPT_HEADERDATA, s);
+    if (curl_easy_perform(state->curl)) {
         goto out;
+    }
     curl_easy_getinfo(state->curl, CURLINFO_CONTENT_LENGTH_DOWNLOAD, &d);
-    curl_easy_setopt(state->curl, CURLOPT_WRITEFUNCTION, (void *)curl_read_cb);
     curl_easy_setopt(state->curl, CURLOPT_NOBODY, 0);
-    if (d)
+#if LIBCURL_VERSION_NUM > 0x071304
+    if (d != -1) {
+#else
+    if (d) {
+#endif
         s->len = (size_t)d;
-    else if(!s->len)
+    } else if (!s->len) {
+        goto out;
+    }
+    if (!strncasecmp(s->url, "http://", strlen("http://"))
+        && !strncasecmp(s->url, "https://", strlen("https://"))
+        && !s->accept_range) {
+        pstrcpy(state->errmsg, CURL_ERROR_SIZE, "Server not supporting range.");
         goto out;
+    }
     DPRINTF("CURL: Size = %zd\n", s->len);
 
     curl_clean_state(state);
@@ -487,10 +503,13 @@ static int curl_open(BlockDriverState *bs, QDict *options, int flags)
 
     // Now we know the file exists and its size, so let's
     // initialize the multi interface!
-
     s->multi = curl_multi_init();
+    if (!s->multi) {
+        goto out_noclean;
+    }
     curl_multi_setopt(s->multi, CURLMOPT_SOCKETDATA, s);
     curl_multi_setopt(s->multi, CURLMOPT_SOCKETFUNCTION, curl_sock_cb);
+    curl_multi_socket_action(s->multi, CURL_SOCKET_TIMEOUT, 0, &running);
 
     qemu_opts_del(opts);
     return 0;
@@ -500,8 +519,9 @@ out:
     curl_easy_cleanup(state->curl);
     state->curl = NULL;
 out_noclean:
-    g_free(s->url);
     qemu_opts_del(opts);
+    g_free(s->url);
+    s->url = NULL;
     return -EINVAL;
 }
 
-- 
1.8.3

[Qemu-devel] [PATCH v7 05/13] curl: add timer to BDRVCURLState

[Fam Zheng]

libcurl uses timer to manage ongoing sockets, it needs us to supply
timer. This patch introduce QEMUTimer to BDRVCURLState and handles
timeouts as libcurl expects (curl_multi_timer_cb sets given timeout
value on the timer and curl_timer_cb calls curl_multi_socket_action on
triggered).

Signed-off-by: Fam Zheng <famz@redhat.com>
---
 block/curl.c | 42 ++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 42 insertions(+)

diff --git a/block/curl.c b/block/curl.c
index d238489..bb46c8f 100644
--- a/block/curl.c
+++ b/block/curl.c
@@ -89,6 +89,7 @@ typedef struct BDRVCURLState {
     QLIST_HEAD(, CURLSockInfo) socks;
     char *url;
     size_t readahead_size;
+    QEMUTimer *timer;
     /* Whether http server accept range in header */
     bool accept_range;
 } BDRVCURLState;
@@ -148,6 +149,38 @@ static size_t curl_header_cb(void *ptr, size_t size, size_t nmemb, void *opaque)
     return realsize;
 }
 
+static void curl_timer_cb(void *opaque)
+{
+    int running;
+    BDRVCURLState *s = opaque;
+    DPRINTF("curl timeout!\n");
+    curl_multi_socket_action(s->multi, CURL_SOCKET_TIMEOUT, 0, &running);
+}
+
+/* Call back for curl_multi interface */
+static int curl_multi_timer_cb(CURLM *multi, long timeout_ms, void *s_)
+{
+    BDRVCURLState *s = s_;
+    DPRINTF("curl multi timer cb, timeout: %ld (ms)\n", timeout_ms);
+    if (timeout_ms < 0) {
+        if (s->timer) {
+            qemu_del_timer(s->timer);
+            qemu_free_timer(s->timer);
+            s->timer = NULL;
+        }
+    } else if (timeout_ms == 0) {
+        curl_timer_cb(s);
+    } else {
+        if (!s->timer) {
+            s->timer = qemu_new_timer_ms(host_clock, curl_timer_cb, s);
+            assert(s->timer);
+        }
+        qemu_mod_timer(s->timer, qemu_get_clock_ms(host_clock) + timeout_ms);
+    }
+
+    return 0;
+}
+
 static size_t curl_read_cb(void *ptr, size_t size, size_t nmemb, void *opaque)
 {
     CURLState *s = ((CURLState*)opaque);
@@ -509,6 +542,8 @@ static int curl_open(BlockDriverState *bs, QDict *options, int flags)
     }
     curl_multi_setopt(s->multi, CURLMOPT_SOCKETDATA, s);
     curl_multi_setopt(s->multi, CURLMOPT_SOCKETFUNCTION, curl_sock_cb);
+    curl_multi_setopt(s->multi, CURLMOPT_TIMERDATA, s);
+    curl_multi_setopt(s->multi, CURLMOPT_TIMERFUNCTION, curl_multi_timer_cb);
     curl_multi_socket_action(s->multi, CURL_SOCKET_TIMEOUT, 0, &running);
 
     qemu_opts_del(opts);
@@ -634,6 +669,13 @@ static void curl_close(BlockDriverState *bs)
     int i;
 
     DPRINTF("CURL: Close\n");
+
+    if (s->timer) {
+        qemu_del_timer(s->timer);
+        qemu_free_timer(s->timer);
+        s->timer = NULL;
+    }
+
     for (i=0; i<CURL_NUM_STATES; i++) {
         if (s->states[i].in_use)
             curl_clean_state(&s->states[i]);
-- 
1.8.3

[Qemu-devel] [PATCH v7 06/13] curl: introduce CURLDataCache

[Fam Zheng]

Data buffer was contained by CURLState, they are allocated and freed
together. This patch try to isolate them, by introducing a dedicated
cache list to BDRVCURLState. The benifit is we can now release the
CURLState (and associated sockets) while keep the fetched data for later
use, and simplies the prefetch and buffer logic for some degree.

Note: There's already page cache in guest kernel, why cache data here?
Since we don't want to submit http/ftp/* request for every 2KB in
sequencial read, but there are crude guest that sends small IO reqs,
which will result in horrible performance.  GRUB/isolinux loading kernel
is a typical case and we workaround this by prefetch cache.  This is
what curl.c has been doing along. This patch just refectors the buffer.

Signed-off-by: Fam Zheng <famz@redhat.com>
---
 block/curl.c | 137 ++++++++++++++++++++++++++++-------------------------------
 1 file changed, 66 insertions(+), 71 deletions(-)

diff --git a/block/curl.c b/block/curl.c
index bb46c8f..a99d8b5 100644
--- a/block/curl.c
+++ b/block/curl.c
@@ -43,10 +43,6 @@
 #define SECTOR_SIZE     512
 #define READ_AHEAD_SIZE (256 * 1024)
 
-#define FIND_RET_NONE   0
-#define FIND_RET_OK     1
-#define FIND_RET_WAIT   2
-
 struct BDRVCURLState;
 
 typedef struct CURLAIOCB {
@@ -61,6 +57,14 @@ typedef struct CURLAIOCB {
     size_t end;
 } CURLAIOCB;
 
+typedef struct CURLDataCache {
+    char *data;
+    int64_t base_pos;
+    size_t data_len;
+    int64_t write_pos;
+    QLIST_ENTRY(CURLDataCache) next;
+} CURLDataCache;
+
 typedef struct CURLState
 {
     struct BDRVCURLState *s;
@@ -90,6 +94,8 @@ typedef struct BDRVCURLState {
     char *url;
     size_t readahead_size;
     QEMUTimer *timer;
+    /* List of data cache ordered by access, freed from tail */
+    QLIST_HEAD(, CURLDataCache) cache;
     /* Whether http server accept range in header */
     bool accept_range;
 } BDRVCURLState;
@@ -98,6 +104,19 @@ static void curl_clean_state(CURLState *s);
 static void curl_fd_handler(void *arg);
 static int curl_aio_flush(void *opaque);
 
+static CURLDataCache *curl_find_cache(BDRVCURLState *bs,
+                                      int64_t start, size_t len)
+{
+    CURLDataCache *c;
+    QLIST_FOREACH(c, &bs->cache, next) {
+        if (start >= c->base_pos &&
+            start + len <= c->base_pos + c->write_pos) {
+            return c;
+        }
+    }
+    return NULL;
+}
+
 static int curl_sock_cb(CURL *curl, curl_socket_t fd, int action,
                         void *userp, void *sockp)
 {
@@ -181,6 +200,23 @@ static int curl_multi_timer_cb(CURLM *multi, long timeout_ms, void *s_)
     return 0;
 }
 
+static void curl_complete_io(BDRVCURLState *bs, CURLAIOCB *acb,
+                             CURLDataCache *cache)
+{
+    int64_t aio_base = acb->sector_num * SECTOR_SIZE;
+    size_t aio_bytes = acb->nb_sectors * SECTOR_SIZE;
+    int64_t off = aio_base - cache->base_pos;
+
+    qemu_iovec_from_buf(acb->qiov, 0, cache->data + off, aio_bytes);
+    acb->common.cb(acb->common.opaque, 0);
+    DPRINTF("AIO Request OK: " PRId64 "%10zd\n", aio_base, aio_bytes);
+    qemu_aio_release(acb);
+    acb = NULL;
+    /* Move cache next in the list */
+    QLIST_REMOVE(cache, next);
+    QLIST_INSERT_HEAD(&bs->cache, cache, next);
+}
+
 static size_t curl_read_cb(void *ptr, size_t size, size_t nmemb, void *opaque)
 {
     CURLState *s = ((CURLState*)opaque);
@@ -214,59 +250,6 @@ read_end:
     return realsize;
 }
 
-static int curl_find_buf(BDRVCURLState *s, size_t start, size_t len,
-                         CURLAIOCB *acb)
-{
-    int i;
-    size_t end = start + len;
-
-    for (i=0; i<CURL_NUM_STATES; i++) {
-        CURLState *state = &s->states[i];
-        size_t buf_end = (state->buf_start + state->buf_off);
-        size_t buf_fend = (state->buf_start + state->buf_len);
-
-        if (!state->orig_buf)
-            continue;
-        if (!state->buf_off)
-            continue;
-
-        // Does the existing buffer cover our section?
-        if ((start >= state->buf_start) &&
-            (start <= buf_end) &&
-            (end >= state->buf_start) &&
-            (end <= buf_end))
-        {
-            char *buf = state->orig_buf + (start - state->buf_start);
-
-            qemu_iovec_from_buf(acb->qiov, 0, buf, len);
-            acb->common.cb(acb->common.opaque, 0);
-
-            return FIND_RET_OK;
-        }
-
-        // Wait for unfinished chunks
-        if ((start >= state->buf_start) &&
-            (start <= buf_fend) &&
-            (end >= state->buf_start) &&
-            (end <= buf_fend))
-        {
-            int j;
-
-            acb->start = start - state->buf_start;
-            acb->end = acb->start + len;
-
-            for (j=0; j<CURL_NUM_ACB; j++) {
-                if (!state->acb[j]) {
-                    state->acb[j] = acb;
-                    return FIND_RET_WAIT;
-                }
-            }
-        }
-    }
-
-    return FIND_RET_NONE;
-}
-
 static void curl_fd_handler(void *arg)
 {
     CURLSockInfo *sock = (CURLSockInfo *)arg;
@@ -299,7 +282,9 @@ static void curl_fd_handler(void *arg)
             case CURLMSG_DONE:
             {
                 CURLState *state = NULL;
-                curl_easy_getinfo(msg->easy_handle, CURLINFO_PRIVATE, (char**)&state);
+                curl_easy_getinfo(msg->easy_handle,
+                                  CURLINFO_PRIVATE,
+                                  (char **)&state);
 
                 /* ACBs for successful messages get completed in curl_read_cb */
                 if (msg->data.result != CURLE_OK) {
@@ -495,6 +480,7 @@ static int curl_open(BlockDriverState *bs, QDict *options, int flags)
     }
 
     QLIST_INIT(&s->socks);
+    QLIST_INIT(&s->cache);
 
     DPRINTF("CURL: Opening %s\n", file);
     s->url = g_strdup(file);
@@ -589,26 +575,24 @@ static const AIOCBInfo curl_aiocb_info = {
 static void curl_readv_bh_cb(void *p)
 {
     CURLState *state;
-
+    CURLDataCache *cache = NULL;
     CURLAIOCB *acb = p;
     BDRVCURLState *s = acb->common.bs->opaque;
+    int64_t aio_base, aio_bytes;
+    int64_t start, end;
 
     qemu_bh_delete(acb->bh);
     acb->bh = NULL;
 
-    size_t start = acb->sector_num * SECTOR_SIZE;
-    size_t end;
+    aio_base = acb->sector_num * SECTOR_SIZE;
+    aio_bytes = acb->nb_sectors * SECTOR_SIZE;
 
-    // In case we have the requested data already (e.g. read-ahead),
-    // we can just call the callback and be done.
-    switch (curl_find_buf(s, start, acb->nb_sectors * SECTOR_SIZE, acb)) {
-        case FIND_RET_OK:
-            qemu_aio_release(acb);
-            // fall through
-        case FIND_RET_WAIT:
-            return;
-        default:
-            break;
+    start = acb->sector_num * SECTOR_SIZE;
+
+    cache = curl_find_cache(s, aio_base, aio_bytes);
+    if (cache) {
+        curl_complete_io(s, acb, cache);
+        return;
     }
 
     // No cache found, so let's start a new request
@@ -691,6 +675,17 @@ static void curl_close(BlockDriverState *bs)
     if (s->multi)
         curl_multi_cleanup(s->multi);
 
+    while (!QLIST_EMPTY(&s->cache)) {
+        CURLDataCache *cache = QLIST_FIRST(&s->cache);
+        if (cache->data) {
+            g_free(cache->data);
+            cache->data = NULL;
+        }
+        QLIST_REMOVE(cache, next);
+        g_free(cache);
+        cache = NULL;
+    }
+
     while (!QLIST_EMPTY(&s->socks)) {
         CURLSockInfo *sock = QLIST_FIRST(&s->socks);
         QLIST_REMOVE(sock, next);
-- 
1.8.3

[Qemu-devel] [PATCH v7 07/13] curl: make use of CURLDataCache.

[Fam Zheng]

Make subsequecial changes to make use of introduced CURLDataCache. Moved
acb struct from CURLState to BDRVCURLState, and changed to list.

Signed-off-by: Fam Zheng <famz@redhat.com>
---
 block/curl.c | 170 ++++++++++++++++++++++++++++++++---------------------------
 1 file changed, 92 insertions(+), 78 deletions(-)

diff --git a/block/curl.c b/block/curl.c
index a99d8b5..5405485 100644
--- a/block/curl.c
+++ b/block/curl.c
@@ -39,7 +39,6 @@
                    CURLPROTO_TFTP)
 
 #define CURL_NUM_STATES 8
-#define CURL_NUM_ACB    8
 #define SECTOR_SIZE     512
 #define READ_AHEAD_SIZE (256 * 1024)
 
@@ -52,9 +51,7 @@ typedef struct CURLAIOCB {
 
     int64_t sector_num;
     int nb_sectors;
-
-    size_t start;
-    size_t end;
+    QLIST_ENTRY(CURLAIOCB) next;
 } CURLAIOCB;
 
 typedef struct CURLDataCache {
@@ -62,20 +59,18 @@ typedef struct CURLDataCache {
     int64_t base_pos;
     size_t data_len;
     int64_t write_pos;
+    /* Ref count for CURLState */
+    int use_count;
     QLIST_ENTRY(CURLDataCache) next;
 } CURLDataCache;
 
 typedef struct CURLState
 {
     struct BDRVCURLState *s;
-    CURLAIOCB *acb[CURL_NUM_ACB];
     CURL *curl;
-    char *orig_buf;
-    size_t buf_start;
-    size_t buf_off;
-    size_t buf_len;
     char range[128];
     char errmsg[CURL_ERROR_SIZE];
+    CURLDataCache *cache;
     char in_use;
 } CURLState;
 
@@ -90,6 +85,7 @@ typedef struct BDRVCURLState {
     CURLM *multi;
     size_t len;
     CURLState states[CURL_NUM_STATES];
+    QLIST_HEAD(, CURLAIOCB) acbs;
     QLIST_HEAD(, CURLSockInfo) socks;
     char *url;
     size_t readahead_size;
@@ -219,31 +215,35 @@ static void curl_complete_io(BDRVCURLState *bs, CURLAIOCB *acb,
 
 static size_t curl_read_cb(void *ptr, size_t size, size_t nmemb, void *opaque)
 {
-    CURLState *s = ((CURLState*)opaque);
+    CURLState *s = opaque;
+    CURLDataCache *c = s->cache;
     size_t realsize = size * nmemb;
-    int i;
-
-    DPRINTF("CURL: Just reading %zd bytes\n", realsize);
+    CURLAIOCB *acb;
 
-    if (!s || !s->orig_buf)
+    if (!c || !c->data) {
         goto read_end;
+    }
+    if (c->write_pos >= c->data_len) {
+        goto read_end;
+    }
+    memcpy(c->data + c->write_pos, ptr,
+           MIN(realsize, c->data_len - c->write_pos));
+    c->write_pos += realsize;
+    if (c->write_pos >= c->data_len) {
+        c->write_pos = c->data_len;
+    }
 
-    memcpy(s->orig_buf + s->buf_off, ptr, realsize);
-    s->buf_off += realsize;
-
-    for(i=0; i<CURL_NUM_ACB; i++) {
-        CURLAIOCB *acb = s->acb[i];
-
-        if (!acb)
-            continue;
-
-        if ((s->buf_off >= acb->end)) {
-            qemu_iovec_from_buf(acb->qiov, 0, s->orig_buf + acb->start,
-                                acb->end - acb->start);
-            acb->common.cb(acb->common.opaque, 0);
-            qemu_aio_release(acb);
-            s->acb[i] = NULL;
+    acb = QLIST_FIRST(&s->s->acbs);
+    while (acb) {
+        int64_t aio_base = acb->sector_num * SECTOR_SIZE;
+        size_t aio_len = acb->nb_sectors * SECTOR_SIZE;
+        CURLAIOCB *next = QLIST_NEXT(acb, next);
+        if (aio_base >= c->base_pos &&
+            aio_base + aio_len <= c->base_pos + c->write_pos) {
+            QLIST_REMOVE(acb, next);
+            curl_complete_io(s->s, acb, c);
         }
+        acb = next;
     }
 
 read_end:
@@ -273,10 +273,12 @@ static void curl_fd_handler(void *arg)
         CURLMsg *msg;
         msg = curl_multi_info_read(s->multi, &msgs_in_queue);
 
-        if (!msg)
+        if (!msg) {
             break;
-        if (msg->msg == CURLMSG_NONE)
+        }
+        if (msg->msg == CURLMSG_NONE) {
             break;
+        }
 
         switch (msg->msg) {
             case CURLMSG_DONE:
@@ -286,19 +288,17 @@ static void curl_fd_handler(void *arg)
                                   CURLINFO_PRIVATE,
                                   (char **)&state);
 
-                /* ACBs for successful messages get completed in curl_read_cb */
+                /* ACBs for successful messages get completed in curl_read_cb,
+                 * fail existing acbs for now */
                 if (msg->data.result != CURLE_OK) {
-                    int i;
-                    for (i = 0; i < CURL_NUM_ACB; i++) {
-                        CURLAIOCB *acb = state->acb[i];
-
-                        if (acb == NULL) {
-                            continue;
-                        }
-
+                    CURLAIOCB *acb = QLIST_FIRST(&s->acbs);
+                    while (acb) {
+                        CURLAIOCB *next = QLIST_NEXT(acb, next);
+                        DPRINTF("EIO, %s\n", state->errmsg);
                         acb->common.cb(acb->common.opaque, -EIO);
+                        QLIST_REMOVE(acb, next);
                         qemu_aio_release(acb);
-                        state->acb[i] = NULL;
+                        acb = next;
                     }
                 }
 
@@ -315,13 +315,10 @@ static void curl_fd_handler(void *arg)
 static CURLState *curl_init_state(BDRVCURLState *s)
 {
     CURLState *state = NULL;
-    int i, j;
+    int i;
 
     do {
         for (i=0; i<CURL_NUM_STATES; i++) {
-            for (j=0; j<CURL_NUM_ACB; j++)
-                if (s->states[i].acb[j])
-                    continue;
             if (s->states[i].in_use)
                 continue;
 
@@ -378,6 +375,10 @@ static void curl_clean_state(CURLState *s)
     if (s->s->multi)
         curl_multi_remove_handle(s->s->multi, s->curl);
     s->in_use = 0;
+    if (s->cache) {
+        s->cache->use_count--;
+        assert(s->cache->use_count >= 0);
+    }
 }
 
 static void curl_parse_filename(const char *filename, QDict *options,
@@ -481,6 +482,7 @@ static int curl_open(BlockDriverState *bs, QDict *options, int flags)
 
     QLIST_INIT(&s->socks);
     QLIST_INIT(&s->cache);
+    QLIST_INIT(&s->acbs);
 
     DPRINTF("CURL: Opening %s\n", file);
     s->url = g_strdup(file);
@@ -549,14 +551,8 @@ out_noclean:
 static int curl_aio_flush(void *opaque)
 {
     BDRVCURLState *s = opaque;
-    int i, j;
-
-    for (i=0; i < CURL_NUM_STATES; i++) {
-        for(j=0; j < CURL_NUM_ACB; j++) {
-            if (s->states[i].acb[j]) {
-                return 1;
-            }
-        }
+    if (!QLIST_EMPTY(&s->acbs)) {
+        return 1;
     }
     return 0;
 }
@@ -579,7 +575,7 @@ static void curl_readv_bh_cb(void *p)
     CURLAIOCB *acb = p;
     BDRVCURLState *s = acb->common.bs->opaque;
     int64_t aio_base, aio_bytes;
-    int64_t start, end;
+    int running;
 
     qemu_bh_delete(acb->bh);
     acb->bh = NULL;
@@ -587,7 +583,9 @@ static void curl_readv_bh_cb(void *p)
     aio_base = acb->sector_num * SECTOR_SIZE;
     aio_bytes = acb->nb_sectors * SECTOR_SIZE;
 
-    start = acb->sector_num * SECTOR_SIZE;
+    if (aio_base + aio_bytes > s->len) {
+        goto err_release;
+    }
 
     cache = curl_find_cache(s, aio_base, aio_bytes);
     if (cache) {
@@ -598,29 +596,41 @@ static void curl_readv_bh_cb(void *p)
     // No cache found, so let's start a new request
     state = curl_init_state(s);
     if (!state) {
-        acb->common.cb(acb->common.opaque, -EIO);
-        qemu_aio_release(acb);
-        return;
+        goto err_release;
     }
 
-    acb->start = 0;
-    acb->end = (acb->nb_sectors * SECTOR_SIZE);
-
-    state->buf_off = 0;
-    if (state->orig_buf)
-        g_free(state->orig_buf);
-    state->buf_start = start;
-    state->buf_len = acb->end + s->readahead_size;
-    end = MIN(start + state->buf_len, s->len) - 1;
-    state->orig_buf = g_malloc(state->buf_len);
-    state->acb[0] = acb;
-
-    snprintf(state->range, sizeof(state->range) - 1, "%zd-%zd", start, end);
-    DPRINTF("CURL (AIO): Reading %d at %zd (%s)\n",
-            (acb->nb_sectors * SECTOR_SIZE), start, state->range);
-    curl_easy_setopt(state->curl, CURLOPT_RANGE, state->range);
+    cache = g_malloc0(sizeof(CURLDataCache));
+    cache->base_pos = acb->sector_num * SECTOR_SIZE;
+    cache->data_len = aio_bytes + s->readahead_size;
+    cache->write_pos = 0;
+    cache->data = g_malloc(cache->data_len);
 
+    QLIST_INSERT_HEAD(&s->acbs, acb, next);
+    snprintf(state->range, sizeof(state->range) - 1, "%zd-%zd", cache->base_pos,
+             cache->base_pos + cache->data_len);
+    DPRINTF("Reading range: %s\n", state->range);
+    curl_easy_setopt(state->curl, CURLOPT_RANGE, state->range);
+    QLIST_INSERT_HEAD(&s->cache, cache, next);
+    state->cache = cache;
+    cache->use_count++;
     curl_multi_add_handle(s->multi, state->curl);
+    /* kick off curl to start the action */
+    curl_multi_socket_action(s->multi, 0, CURL_SOCKET_TIMEOUT, &running);
+    return;
+
+err_release:
+    if (cache) {
+        if (cache->data) {
+            g_free(cache->data);
+            cache->data = NULL;
+        }
+        g_free(cache);
+        cache = NULL;
+    }
+    acb->common.cb(acb->common.opaque, -EIO);
+    qemu_aio_release(acb);
+    return;
+
 
 }
 
@@ -667,14 +677,18 @@ static void curl_close(BlockDriverState *bs)
             curl_easy_cleanup(s->states[i].curl);
             s->states[i].curl = NULL;
         }
-        if (s->states[i].orig_buf) {
-            g_free(s->states[i].orig_buf);
-            s->states[i].orig_buf = NULL;
-        }
     }
     if (s->multi)
         curl_multi_cleanup(s->multi);
 
+    while (!QLIST_EMPTY(&s->acbs)) {
+        CURLAIOCB *acb = QLIST_FIRST(&s->acbs);
+        acb->common.cb(acb->common.opaque, -EIO);
+        QLIST_REMOVE(acb, next);
+        qemu_aio_release(acb);
+        acb = NULL;
+    }
+
     while (!QLIST_EMPTY(&s->cache)) {
         CURLDataCache *cache = QLIST_FIRST(&s->cache);
         if (cache->data) {
-- 
1.8.3

[Qemu-devel] [PATCH v7 08/13] curl: use list to store CURLState

[Fam Zheng]

Make it consistent to other structures to use QLIST to store CURLState.
It also simplifies initialization and releasing of data.

Signed-off-by: Fam Zheng <famz@redhat.com>
---
 block/curl.c | 82 +++++++++++++++++++++++++++---------------------------------
 1 file changed, 37 insertions(+), 45 deletions(-)

diff --git a/block/curl.c b/block/curl.c
index 5405485..9b18238 100644
--- a/block/curl.c
+++ b/block/curl.c
@@ -38,7 +38,6 @@
                    CURLPROTO_FTP | CURLPROTO_FTPS | \
                    CURLPROTO_TFTP)
 
-#define CURL_NUM_STATES 8
 #define SECTOR_SIZE     512
 #define READ_AHEAD_SIZE (256 * 1024)
 
@@ -64,14 +63,13 @@ typedef struct CURLDataCache {
     QLIST_ENTRY(CURLDataCache) next;
 } CURLDataCache;
 
-typedef struct CURLState
-{
+typedef struct CURLState {
     struct BDRVCURLState *s;
     CURL *curl;
     char range[128];
     char errmsg[CURL_ERROR_SIZE];
     CURLDataCache *cache;
-    char in_use;
+    QLIST_ENTRY(CURLState) next;
 } CURLState;
 
 typedef struct CURLSockInfo {
@@ -84,7 +82,7 @@ typedef struct CURLSockInfo {
 typedef struct BDRVCURLState {
     CURLM *multi;
     size_t len;
-    CURLState states[CURL_NUM_STATES];
+    QLIST_HEAD(, CURLState) curl_states;
     QLIST_HEAD(, CURLAIOCB) acbs;
     QLIST_HEAD(, CURLSockInfo) socks;
     char *url;
@@ -303,6 +301,9 @@ static void curl_fd_handler(void *arg)
                 }
 
                 curl_clean_state(state);
+                QLIST_REMOVE(state, next);
+                g_free(state);
+                state = NULL;
                 break;
             }
             default:
@@ -314,29 +315,17 @@ static void curl_fd_handler(void *arg)
 
 static CURLState *curl_init_state(BDRVCURLState *s)
 {
-    CURLState *state = NULL;
-    int i;
-
-    do {
-        for (i=0; i<CURL_NUM_STATES; i++) {
-            if (s->states[i].in_use)
-                continue;
-
-            state = &s->states[i];
-            state->in_use = 1;
-            break;
-        }
-        if (!state) {
-            g_usleep(100);
-        }
-    } while(!state);
-
-    if (state->curl)
-        goto has_curl;
+    CURLState *state;
 
+    state = g_malloc0(sizeof(CURLState));
+    state->s = s;
     state->curl = curl_easy_init();
-    if (!state->curl)
-        return NULL;
+    if (!state->curl) {
+        DPRINTF("CURL: curl_easy_init failed\n");
+        g_free(state);
+        state = NULL;
+        goto out;
+    }
     curl_easy_setopt(state->curl, CURLOPT_URL, s->url);
     curl_easy_setopt(state->curl, CURLOPT_TIMEOUT, 5);
     curl_easy_setopt(state->curl, CURLOPT_WRITEFUNCTION, (void *)curl_read_cb);
@@ -362,19 +351,19 @@ static CURLState *curl_init_state(BDRVCURLState *s)
 #ifdef DEBUG_VERBOSE
     curl_easy_setopt(state->curl, CURLOPT_VERBOSE, 1);
 #endif
-
-has_curl:
-
-    state->s = s;
-
+out:
     return state;
 }
 
 static void curl_clean_state(CURLState *s)
 {
-    if (s->s->multi)
-        curl_multi_remove_handle(s->s->multi, s->curl);
-    s->in_use = 0;
+    if (s->curl) {
+        if (s->s->multi) {
+            curl_multi_remove_handle(s->s->multi, s->curl);
+        }
+        curl_easy_cleanup(s->curl);
+        s->curl = NULL;
+    }
     if (s->cache) {
         s->cache->use_count--;
         assert(s->cache->use_count >= 0);
@@ -483,6 +472,7 @@ static int curl_open(BlockDriverState *bs, QDict *options, int flags)
     QLIST_INIT(&s->socks);
     QLIST_INIT(&s->cache);
     QLIST_INIT(&s->acbs);
+    QLIST_INIT(&s->curl_states);
 
     DPRINTF("CURL: Opening %s\n", file);
     s->url = g_strdup(file);
@@ -520,7 +510,8 @@ static int curl_open(BlockDriverState *bs, QDict *options, int flags)
 
     curl_clean_state(state);
     curl_easy_cleanup(state->curl);
-    state->curl = NULL;
+    g_free(state);
+    state = NULL;
 
     // Now we know the file exists and its size, so let's
     // initialize the multi interface!
@@ -610,6 +601,7 @@ static void curl_readv_bh_cb(void *p)
              cache->base_pos + cache->data_len);
     DPRINTF("Reading range: %s\n", state->range);
     curl_easy_setopt(state->curl, CURLOPT_RANGE, state->range);
+    QLIST_INSERT_HEAD(&s->curl_states, state, next);
     QLIST_INSERT_HEAD(&s->cache, cache, next);
     state->cache = cache;
     cache->use_count++;
@@ -631,7 +623,6 @@ err_release:
     qemu_aio_release(acb);
     return;
 
-
 }
 
 static BlockDriverAIOCB *curl_aio_readv(BlockDriverState *bs,
@@ -660,7 +651,6 @@ static BlockDriverAIOCB *curl_aio_readv(BlockDriverState *bs,
 static void curl_close(BlockDriverState *bs)
 {
     BDRVCURLState *s = bs->opaque;
-    int i;
 
     DPRINTF("CURL: Close\n");
 
@@ -670,16 +660,18 @@ static void curl_close(BlockDriverState *bs)
         s->timer = NULL;
     }
 
-    for (i=0; i<CURL_NUM_STATES; i++) {
-        if (s->states[i].in_use)
-            curl_clean_state(&s->states[i]);
-        if (s->states[i].curl) {
-            curl_easy_cleanup(s->states[i].curl);
-            s->states[i].curl = NULL;
-        }
+    while (!QLIST_EMPTY(&s->curl_states)) {
+        CURLState *state = QLIST_FIRST(&s->curl_states);
+        /* Remove and clean curl easy handles */
+        curl_clean_state(state);
+        QLIST_REMOVE(state, next);
+        g_free(state);
+        state = NULL;
     }
-    if (s->multi)
+
+    if (s->multi) {
         curl_multi_cleanup(s->multi);
+    }
 
     while (!QLIST_EMPTY(&s->acbs)) {
         CURLAIOCB *acb = QLIST_FIRST(&s->acbs);
-- 
1.8.3

[Qemu-devel] [PATCH v7 09/13] curl: add cache quota.

[Fam Zheng]

Introduce a cache quota: BDRVCURLState.cache_quota.
When adding new CURLDataCache to BDRVCURLState, if number of existing
CURLDataCache is larger than CURL_CACHE_QUOTA, try to release some first
to limit the in memory cache size.

A least used entry is selected for releasing.

Signed-off-by: Fam Zheng <famz@redhat.com>
---
 block/curl.c | 27 +++++++++++++++++++++++++++
 1 file changed, 27 insertions(+)

diff --git a/block/curl.c b/block/curl.c
index 9b18238..6e893d0 100644
--- a/block/curl.c
+++ b/block/curl.c
@@ -40,6 +40,7 @@
 
 #define SECTOR_SIZE     512
 #define READ_AHEAD_SIZE (256 * 1024)
+#define CURL_CACHE_QUOTA 10
 
 struct BDRVCURLState;
 
@@ -90,6 +91,8 @@ typedef struct BDRVCURLState {
     QEMUTimer *timer;
     /* List of data cache ordered by access, freed from tail */
     QLIST_HEAD(, CURLDataCache) cache;
+    /* Threshold to release unused cache when cache list is longer than it */
+    int cache_quota;
     /* Whether http server accept range in header */
     bool accept_range;
 } BDRVCURLState;
@@ -526,6 +529,8 @@ static int curl_open(BlockDriverState *bs, QDict *options, int flags)
     curl_multi_socket_action(s->multi, CURL_SOCKET_TIMEOUT, 0, &running);
 
     qemu_opts_del(opts);
+    s->cache_quota = CURL_CACHE_QUOTA;
+
     return 0;
 
 out:
@@ -595,6 +600,27 @@ static void curl_readv_bh_cb(void *p)
     cache->data_len = aio_bytes + s->readahead_size;
     cache->write_pos = 0;
     cache->data = g_malloc(cache->data_len);
+    /* Try to release some cache */
+    while (s->cache_quota <= 0) {
+        CURLDataCache *p;
+        CURLDataCache *q = NULL;
+        assert(!QLIST_EMPTY(&s->cache));
+        for (p = QLIST_FIRST(&s->cache);
+             p; p = QLIST_NEXT(p, next)) {
+            if (p->use_count == 0) {
+                q = p;
+            }
+        }
+        if (!q) {
+            break;
+        }
+        QLIST_REMOVE(q, next);
+        g_free(q->data);
+        q->data = NULL;
+        g_free(q);
+        q = NULL;
+        s->cache_quota++;
+    }
 
     QLIST_INSERT_HEAD(&s->acbs, acb, next);
     snprintf(state->range, sizeof(state->range) - 1, "%zd-%zd", cache->base_pos,
@@ -605,6 +631,7 @@ static void curl_readv_bh_cb(void *p)
     QLIST_INSERT_HEAD(&s->cache, cache, next);
     state->cache = cache;
     cache->use_count++;
+    s->cache_quota--;
     curl_multi_add_handle(s->multi, state->curl);
     /* kick off curl to start the action */
     curl_multi_socket_action(s->multi, 0, CURL_SOCKET_TIMEOUT, &running);
-- 
1.8.3

[Qemu-devel] [PATCH v7 10/13] curl: introduce ssl_no_cert runtime option.

[Fam Zheng]

Added an option to let curl disable ssl certificate check.

Signed-off-by: Fam Zheng <famz@redhat.com>
---
 block/curl.c | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

diff --git a/block/curl.c b/block/curl.c
index 6e893d0..e067417 100644
--- a/block/curl.c
+++ b/block/curl.c
@@ -95,6 +95,8 @@ typedef struct BDRVCURLState {
     int cache_quota;
     /* Whether http server accept range in header */
     bool accept_range;
+    /* Whether certificated ssl only */
+    bool ssl_no_cert;
 } BDRVCURLState;
 
 static void curl_clean_state(CURLState *s);
@@ -339,6 +341,8 @@ static CURLState *curl_init_state(BDRVCURLState *s)
     curl_easy_setopt(state->curl, CURLOPT_NOSIGNAL, 1);
     curl_easy_setopt(state->curl, CURLOPT_ERRORBUFFER, state->errmsg);
     curl_easy_setopt(state->curl, CURLOPT_FAILONERROR, 1);
+    curl_easy_setopt(state->curl, CURLOPT_SSL_VERIFYPEER,
+                     s->ssl_no_cert ? 0 : 1);
 
     /* Restrict supported protocols to avoid security issues in the more
      * obscure protocols.  For example, do not allow POP3/SMTP/IMAP see
@@ -429,7 +433,12 @@ static QemuOptsList runtime_opts = {
             .type = QEMU_OPT_SIZE,
             .help = "Readahead size",
         },
-        { /* end of list */ }
+        {
+            .name = "ssl_no_cert",
+            .type = QEMU_OPT_BOOL,
+            .help = "SSL certificate check",
+        },
+        { /* End of list */ }
     },
 };
 
@@ -467,6 +476,7 @@ static int curl_open(BlockDriverState *bs, QDict *options, int flags)
         goto out_noclean;
     }
 
+    s->ssl_no_cert = qemu_opt_get_bool(opts, "ssl_no_cert", true);
     if (!inited) {
         curl_global_init(CURL_GLOBAL_ALL);
         inited = 1;
-- 
1.8.3

[Qemu-devel] [PATCH v7 11/13] block/curl.c: Refuse to open the handle for writes.

[Fam Zheng]

From: "Richard W.M. Jones" <rjones@redhat.com>

Signed-off-by: Richard W.M. Jones <rjones@redhat.com>
Signed-off-by: Fam Zheng <famz@redhat.com>
---
 block/curl.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/block/curl.c b/block/curl.c
index e067417..bce2e8a 100644
--- a/block/curl.c
+++ b/block/curl.c
@@ -454,6 +454,10 @@ static int curl_open(BlockDriverState *bs, QDict *options, int flags)
 
     static int inited = 0;
 
+    if (flags & BDRV_O_RDWR) {
+        return -ENOTSUP;
+    }
+
     opts = qemu_opts_create_nofail(&runtime_opts);
     qemu_opts_absorb_qdict(opts, options, &local_err);
     if (error_is_set(&local_err)) {
-- 
1.8.3

[Qemu-devel] [PATCH v7 12/13] curl: set s->url to NULL after free.

[Fam Zheng]

Signed-off-by: Fam Zheng <famz@redhat.com>
---
 block/curl.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/block/curl.c b/block/curl.c
index bce2e8a..50c7188 100644
--- a/block/curl.c
+++ b/block/curl.c
@@ -741,6 +741,7 @@ static void curl_close(BlockDriverState *bs)
     }
 
     g_free(s->url);
+    s->url = NULL;
 }
 
 static int64_t curl_getlength(BlockDriverState *bs)
-- 
1.8.3

[Qemu-devel] [PATCH v7 13/13] curl: change timeout to 30 seconds

[Fam Zheng]

On curl request timeout, guest gets -EIO. This happens too frequently
accessing a slow network resource, with 5 seconds timeout. Change it to
30 seconds to give more time before aborting the request.

Reported-by: Richard W.M. Jones <rjones@redhat.com>
Signed-off-by: Fam Zheng <famz@redhat.com>
---
 block/curl.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/block/curl.c b/block/curl.c
index 50c7188..5d91a05 100644
--- a/block/curl.c
+++ b/block/curl.c
@@ -332,7 +332,7 @@ static CURLState *curl_init_state(BDRVCURLState *s)
         goto out;
     }
     curl_easy_setopt(state->curl, CURLOPT_URL, s->url);
-    curl_easy_setopt(state->curl, CURLOPT_TIMEOUT, 5);
+    curl_easy_setopt(state->curl, CURLOPT_TIMEOUT, 30);
     curl_easy_setopt(state->curl, CURLOPT_WRITEFUNCTION, (void *)curl_read_cb);
     curl_easy_setopt(state->curl, CURLOPT_WRITEDATA, (void *)state);
     curl_easy_setopt(state->curl, CURLOPT_PRIVATE, (void *)state);
-- 
1.8.3

Re: [Qemu-devel] [PATCH v7 00/13] curl: fix curl read

[Richard W.M. Jones]

[-- Attachment #1: Type: text/plain, Size: 1141 bytes --]

On Thu, Jun 06, 2013 at 02:25:46PM +0800, Fam Zheng wrote:
> v7:
>   13: Added:
>       curl: change timeout to 30 seconds

I tested this against:

(1) HTTP to Apache server over slow but local wifi.

(2) HTTP to a remote ISO (on another continent).

Test (1) is fine.

Test (2) gives plenty of I/O errors.  I guess that 30 seconds isn't
sufficient here.

I should note that current upstream qemu *works* in both cases.
Although the timeout in current qemu is much shorter (5 seconds), for
some reason this does not affect the test.

I'm also confused about what problem this patch series is trying to
fix, since upstream qemu works fine for me with the latest curl.

The test script I'm using is attached.

I'm using: qemu v1.4.0-2150-g8819c10 with or without the v7 patch
series, curl curl-7_30_0-173-g5657c56 and libguestfs 1.23.2-15-g9585077

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
virt-p2v converts physical machines to virtual machines.  Boot with a
live CD or over the network (PXE) and turn machines into KVM guests.
http://libguestfs.org/virt-v2v

[-- Attachment #2: test.sh --]
[-- Type: application/x-sh, Size: 1020 bytes --]

Re: [Qemu-devel] [PATCH v7 00/13] curl: fix curl read

[Fam Zheng]

On Thu, 06/06 12:01, Richard W.M. Jones wrote:
> On Thu, Jun 06, 2013 at 02:25:46PM +0800, Fam Zheng wrote:
> > v7:
> >   13: Added:
> >       curl: change timeout to 30 seconds
> 
> I tested this against:
> 
> (1) HTTP to Apache server over slow but local wifi.
> 
> (2) HTTP to a remote ISO (on another continent).
> 
> Test (1) is fine.
> 
> Test (2) gives plenty of I/O errors.  I guess that 30 seconds isn't
> sufficient here.
> 
> I should note that current upstream qemu *works* in both cases.
> Although the timeout in current qemu is much shorter (5 seconds), for
> some reason this does not affect the test.
> 
> I'm also confused about what problem this patch series is trying to
> fix, since upstream qemu works fine for me with the latest curl.
> 

The weird thing is it doesn't work for me, I'm sure something is wrong
with current upstream, although not totally broken.

  $./qemu-io http://localhost/vm/arch.raw -c 'read -v 0 512'
  (stuck here forever, no output)

Attach with gdb:

    (gdb) bt
    #0  aio_dispatch (ctx=0xaa1af230) at aio-posix.c:128
    #1  0x00007f6fa96a18f7 in aio_poll (ctx=0x7f6faa1afa00,
        blocking=true) at aio-posix.c:190
    #2  0x00007f6fa96ecabc in qemu_aio_wait ()
        at main-loop.c:484
    #3  0x00007f6fa96a7530 in bdrv_rwv_co (bs=0x7f6faa1b9540,
        sector_num=0, qiov=0x7fffacf403b0, is_write=false)
            at block.c:2216
    #4  0x00007f6fa96a75ca in bdrv_rw_co (bs=0x7f6faa1b9540,
        sector_num=0, buf=0x7fffacf406f0 "", nb_sectors=4,
            is_write=false) at block.c:2235
    #5  0x00007f6fa96a7629 in bdrv_read (bs=0x7f6faa1b9540,
        sector_num=0, buf=0x7fffacf406f0 "", nb_sectors=4)
            at block.c:2242
    #6  0x00007f6fa96a78f5 in bdrv_pread (bs=0x7f6faa1b9540,
        offset=0, buf=0x7fffacf406f0, count1=2048)
            at block.c:2304
    #7  0x00007f6fa96a36f4 in find_image_format (
        bs=0x7f6faa1b9540,
            filename=0x7fffacf4441d "http://localhost/vm/arch.raw",
            pdrv=0x7fffacf40f28) at block.c:542
    #8  0x00007f6fa96a4a8a in bdrv_open (bs=0x7f6faa1b5b00,
        filename=0x7fffacf4441d "http://localhost/vm/arch.raw",
        options=0x7f6faa1b7500, flags=24578, drv=0x0)
            at block.c:1055
    #9  0x00007f6fa970e803 in openfile (
        name=0x7fffacf4441d "http://localhost/vm/arch.raw",
            flags=16386, growable=0) at qemu-io.c:1812
    #10 0x00007f6fa970ef5b in main (argc=4,
        argv=0x7fffacf43308) at qemu-io.c:2055

The BlockDriverState looks normal, just that libcurl not making any
progress (no callback, no timeout)


-- 
Fam

Re: [Qemu-devel] [PATCH v7 00/13] curl: fix curl read

[Richard W.M. Jones]

On Fri, Jun 07, 2013 at 09:54:42AM +0800, Fam Zheng wrote:
> The weird thing is it doesn't work for me, I'm sure something is wrong
> with current upstream, although not totally broken.
> 
>   $./qemu-io http://localhost/vm/arch.raw -c 'read -v 0 512'
>   (stuck here forever, no output)

This doesn't work for me either.

*However* it only doesn't work if I use the Fedora version of curl,
which I'm convinced is broken.

That's why I'm doing all my testing with upstream curl from git,
including all the qemu tests I did.  The above command works fine with
curl from git:

$ LD_LIBRARY_PATH=~/d/curl/lib/.libs ./qemu-io http://192.168.0.249/scratch/winxp.img -c 'read -v 0 512'
00000000:  33 c0 8e d0 bc 00 7c fb 50 07 50 1f fc be 1b 7c  3.......P.P.....
00000010:  bf 1b 06 50 57 b9 e5 01 f3 a4 cb bd be 07 b1 04  ...PW...........
00000020:  38 6e 00 7c 09 75 13 83 c5 10 e2 f4 cd 18 8b f5  8n...u..........
00000030:  83 c6 10 49 74 19 38 2c 74 f6 a0 b5 07 b4 07 8b  ...It.8.t.......
00000040:  f0 ac 3c 00 74 fc bb 07 00 b4 0e cd 10 eb f2 88  ....t...........
00000050:  4e 10 e8 46 00 73 2a fe 46 10 80 7e 04 0b 74 0b  N..F.s..F.....t.
00000060:  80 7e 04 0c 74 05 a0 b6 07 75 d2 80 46 02 06 83  ....t....u..F...
00000070:  46 08 06 83 56 0a 00 e8 21 00 73 05 a0 b6 07 eb  F...V.....s.....
00000080:  bc 81 3e fe 7d 55 aa 74 0b 80 7e 10 00 74 c8 a0  .....U.t.....t..
00000090:  b7 07 eb a9 8b fc 1e 57 8b f5 cb bf 05 00 8a 56  .......W.......V
000000a0:  00 b4 08 cd 13 72 23 8a c1 24 3f 98 8a de 8a fc  .....r..........
000000b0:  43 f7 e3 8b d1 86 d6 b1 06 d2 ee 42 f7 e2 39 56  C..........B..9V
000000c0:  0a 77 23 72 05 39 46 08 73 1c b8 01 02 bb 00 7c  .w.r.9F.s.......
000000d0:  8b 4e 02 8b 56 00 cd 13 73 51 4f 74 4e 32 e4 8a  .N..V...sQOtN2..
000000e0:  56 00 cd 13 eb e4 8a 56 00 60 bb aa 55 b4 41 cd  V......V....U.A.
000000f0:  13 72 36 81 fb 55 aa 75 30 f6 c1 01 74 2b 61 60  .r6..U.u0...t.a.
00000100:  6a 00 6a 00 ff 76 0a ff 76 08 6a 00 68 00 7c 6a  j.j..v..v.j.h..j
00000110:  01 6a 10 b4 42 8b f4 cd 13 61 61 73 0e 4f 74 0b  .j..B....aas.Ot.
00000120:  32 e4 8a 56 00 cd 13 eb d6 61 f9 c3 49 6e 76 61  2..V.....a..Inva
00000130:  6c 69 64 20 70 61 72 74 69 74 69 6f 6e 20 74 61  lid.partition.ta
00000140:  62 6c 65 00 45 72 72 6f 72 20 6c 6f 61 64 69 6e  ble.Error.loadin
00000150:  67 20 6f 70 65 72 61 74 69 6e 67 20 73 79 73 74  g.operating.syst
00000160:  65 6d 00 4d 69 73 73 69 6e 67 20 6f 70 65 72 61  em.Missing.opera
00000170:  74 69 6e 67 20 73 79 73 74 65 6d 00 00 00 00 00  ting.system.....
00000180:  00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00  ................
00000190:  00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00  ................
000001a0:  00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00  ................
000001b0:  00 00 00 00 00 2c 44 63 1a dd 1a dd 00 00 80 01  ......Dc........
000001c0:  01 00 07 fe ff 0d 3f 00 00 00 4f b1 bf 00 00 00  ..........O.....
000001d0:  00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00  ................
000001e0:  00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00  ................
000001f0:  00 00 00 00 00 00 00 00 00 00 00 00 00 00 55 aa  ..............U.
read 512/512 bytes at offset 0
512 bytes, 1 ops; 0.0000 sec (32.552 MiB/sec and 66666.6667 ops/sec)

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Fedora Windows cross-compiler. Compile Windows programs, test, and
build Windows installers. Over 100 libraries supported.
http://fedoraproject.org/wiki/MinGW

Re: [Qemu-devel] [PATCH v7 00/13] curl: fix curl read

[Fam Zheng]

On Fri, 06/07 08:27, Richard W.M. Jones wrote:
> On Fri, Jun 07, 2013 at 09:54:42AM +0800, Fam Zheng wrote:
> > The weird thing is it doesn't work for me, I'm sure something is wrong
> > with current upstream, although not totally broken.
> > 
> >   $./qemu-io http://localhost/vm/arch.raw -c 'read -v 0 512'
> >   (stuck here forever, no output)
> 
> This doesn't work for me either.
> 
> *However* it only doesn't work if I use the Fedora version of curl,
> which I'm convinced is broken.

I see, it turns out upstream curl works in my case too, thanks.

So it's more of curl broken rather than qemu driver. For the timeout
issue introduced here, I think it's the effect of implementing timer in
the driver, I can't think of a good way to workaround. However the
master code not using timer at all is an undocumented usage of libcurl.

-- 
Fam

Re: [Qemu-devel] [PATCH v7 00/13] curl: fix curl read

[Richard W.M. Jones]

On Fri, Jun 07, 2013 at 03:47:23PM +0800, Fam Zheng wrote:
> On Fri, 06/07 08:27, Richard W.M. Jones wrote:
> > On Fri, Jun 07, 2013 at 09:54:42AM +0800, Fam Zheng wrote:
> > > The weird thing is it doesn't work for me, I'm sure something is wrong
> > > with current upstream, although not totally broken.
> > > 
> > >   $./qemu-io http://localhost/vm/arch.raw -c 'read -v 0 512'
> > >   (stuck here forever, no output)
> > 
> > This doesn't work for me either.
> > 
> > *However* it only doesn't work if I use the Fedora version of curl,
> > which I'm convinced is broken.
> 
> I see, it turns out upstream curl works in my case too, thanks.

FYI this is still broken in the very latest curl in Fedora 20.  I
finally got around to filing a bug about it:

https://bugzilla.redhat.com/show_bug.cgi?id=971790

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Fedora Windows cross-compiler. Compile Windows programs, test, and
build Windows installers. Over 100 libraries supported.
http://fedoraproject.org/wiki/MinGW

Re: [Qemu-devel] [PATCH v7 00/13] curl: fix curl read

[Stefan Hajnoczi]

On Thu, Jun 06, 2013 at 12:01:57PM +0100, Richard W.M. Jones wrote:
> On Thu, Jun 06, 2013 at 02:25:46PM +0800, Fam Zheng wrote:
> > v7:
> >   13: Added:
> >       curl: change timeout to 30 seconds
> 
> I tested this against:
> 
> (1) HTTP to Apache server over slow but local wifi.
> 
> (2) HTTP to a remote ISO (on another continent).
> 
> Test (1) is fine.
> 
> Test (2) gives plenty of I/O errors.  I guess that 30 seconds isn't
> sufficient here.
> 
> I should note that current upstream qemu *works* in both cases.
> Although the timeout in current qemu is much shorter (5 seconds), for
> some reason this does not affect the test.
> 
> I'm also confused about what problem this patch series is trying to
> fix, since upstream qemu works fine for me with the latest curl.

One problem I've had with the upstream code is that HTTP servers which
do not support Range: headers fail in a weird way (I don't remember
exactly what happens).  This series does improve this by explicitly
checking for Range: header support.

But there is a lot of other refactoring going on which is reasonable
only if it works as well or better than the upstream code.

Stefan

Re: [Qemu-devel] [PATCH v7 00/13] curl: fix curl read

[Fam Zheng]

On Fri, 06/07 09:46, Stefan Hajnoczi wrote:
> On Thu, Jun 06, 2013 at 12:01:57PM +0100, Richard W.M. Jones wrote:
> > On Thu, Jun 06, 2013 at 02:25:46PM +0800, Fam Zheng wrote:
> > > v7:
> > >   13: Added:
> > >       curl: change timeout to 30 seconds
> > 
> > I tested this against:
> > 
> > (1) HTTP to Apache server over slow but local wifi.
> > 
> > (2) HTTP to a remote ISO (on another continent).
> > 
> > Test (1) is fine.
> > 
> > Test (2) gives plenty of I/O errors.  I guess that 30 seconds isn't
> > sufficient here.
> > 
> > I should note that current upstream qemu *works* in both cases.
> > Although the timeout in current qemu is much shorter (5 seconds), for
> > some reason this does not affect the test.
> > 
> > I'm also confused about what problem this patch series is trying to
> > fix, since upstream qemu works fine for me with the latest curl.
> 
> One problem I've had with the upstream code is that HTTP servers which
> do not support Range: headers fail in a weird way (I don't remember
> exactly what happens).  This series does improve this by explicitly
> checking for Range: header support.
> 
What I see is it always returns the first sectors to guest.

> But there is a lot of other refactoring going on which is reasonable
> only if it works as well or better than the upstream code.
> 
In this case it doesn't :(
I can probably send a fix for range and drop the refactoring for now.

-- 
Fam