[Qemu-devel] [PATCH v5 00/11] curl: fix curl read

[Qemu-devel] [PATCH v5 00/11] curl: fix curl read

[Fam Zheng]

CURL library API has changed, the current curl driver is not working.
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.

Changes from v4:
  11: Added:
        block/curl.c: Refuse to open the handle for writes.

Changes from v3:
  01, 06, 07: Add QLIST_INIT in qemu_open to initialize each list.
  07: Move clean up for s->acbs from later patch to here. Use qemu_aio_relase instead of g_free on acb.
      Fix use-after-free bug. [Rich]

Changes from v2:
  00: Removed previous 09, since changes are included in previous
      commits.
      previous 09: curl: release everything on curl close.
  01: Add commit message for introducing CURLSockInfo. Remove
      unnecessary pointer type cast.
  02: Use sizeof instead of macro for s->range len.
  04: Use pstrcpy and strncasecmp.
  06: Add commit message for why is there a cache in curl.c.

Changes from v1:
   Split to multiple patches. The final status should be identical to v1.

Fam Zheng (10):
  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.

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

 block/curl.c | 578 +++++++++++++++++++++++++++++++++++++----------------------
 1 file changed, 365 insertions(+), 213 deletions(-)

-- 
1.8.2.3

[Qemu-devel] [PATCH v5 01/11] 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 | 31 +++++++++++++++++++++++++++++--
 1 file changed, 29 insertions(+), 2 deletions(-)

diff --git a/block/curl.c b/block/curl.c
index b8935fd..2e42cd1 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;
@@ -90,7 +98,16 @@ static int curl_aio_flush(void *opaque);
 static int curl_sock_cb(CURL *curl, curl_socket_t fd, int action,
                         void *s, void *sp)
 {
+    BDRVCURLState *bs = s;
     DPRINTF("CURL (AIO): Sock action %d on fd %d\n", action, fd);
+    CURLSockInfo *sock = sp;
+    if (!sp) {
+        sock = g_malloc0(sizeof(CURLSockInfo));
+        sock->fd = fd;
+        sock->s = bs;
+        QLIST_INSERT_HEAD(&bs->socks, sock, next);
+        curl_multi_assign(bs->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.2.3

[Qemu-devel] [PATCH v5 02/11] 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 2e42cd1..d0bc66f 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.2.3

[Qemu-devel] [PATCH v5 03/11] 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 d0bc66f..4a56cfd 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.2.3

[Qemu-devel] [PATCH v5 04/11] 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 4a56cfd..fc464ad 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.2.3

[Qemu-devel] [PATCH v5 05/11] 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 fc464ad..4fd5bb9 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 *bs = (BDRVCURLState *)opaque;
+    DPRINTF("curl timeout!\n");
+    curl_multi_socket_action(bs->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 *bs = (BDRVCURLState *)s;
+    DPRINTF("curl multi timer cb, timeout: %ld (ms)\n", timeout_ms);
+    if (timeout_ms < 0) {
+        if (bs->timer) {
+            qemu_del_timer(bs->timer);
+            qemu_free_timer(bs->timer);
+            bs->timer = NULL;
+        }
+    } else if (timeout_ms == 0) {
+        curl_timer_cb(bs);
+    } else {
+        if (!bs->timer) {
+            bs->timer = qemu_new_timer_ms(host_clock, curl_timer_cb, s);
+            assert(bs->timer);
+        }
+        qemu_mod_timer(bs->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.2.3

[Qemu-devel] [PATCH v5 06/11] 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 | 136 +++++++++++++++++++++++++++++------------------------------
 1 file changed, 67 insertions(+), 69 deletions(-)

diff --git a/block/curl.c b/block/curl.c
index 4fd5bb9..e387ae1 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,16 @@ typedef struct CURLAIOCB {
     size_t end;
 } CURLAIOCB;
 
+typedef struct CURLDataCache {
+    char *data;
+    size_t base_pos;
+    size_t data_len;
+    size_t write_pos;
+    /* Ref count for CURLState */
+    int use_count;
+    QLIST_ENTRY(CURLDataCache) next;
+} CURLDataCache;
+
 typedef struct CURLState
 {
     struct BDRVCURLState *s;
@@ -90,6 +96,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 +106,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,
+                                      size_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 *s, void *sp)
 {
@@ -181,6 +202,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)
+{
+    size_t aio_base = acb->sector_num * SECTOR_SIZE;
+    size_t aio_bytes = acb->nb_sectors * SECTOR_SIZE;
+    size_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: %10zd %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 +252,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 +284,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 +482,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 +577,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;
+    size_t aio_base, aio_bytes;
 
     qemu_bh_delete(acb->bh);
     acb->bh = NULL;
 
+    aio_base = acb->sector_num * SECTOR_SIZE;
+    aio_bytes = acb->nb_sectors * SECTOR_SIZE;
+
     size_t start = acb->sector_num * SECTOR_SIZE;
     size_t end;
 
-    // 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;
+    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 +677,18 @@ 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);
+        assert(cache->use_count == 0);
+        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.2.3

[Qemu-devel] [PATCH v5 07/11] 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 | 168 ++++++++++++++++++++++++++++++++---------------------------
 1 file changed, 90 insertions(+), 78 deletions(-)

diff --git a/block/curl.c b/block/curl.c
index e387ae1..4c4752b 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 {
@@ -70,14 +67,10 @@ typedef struct 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;
 
@@ -92,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;
@@ -221,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 = (CURLState *)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) {
+        size_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:
@@ -275,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:
@@ -288,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;
                     }
                 }
 
@@ -317,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;
 
@@ -380,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,
@@ -483,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);
@@ -551,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;
 }
@@ -580,6 +574,7 @@ static void curl_readv_bh_cb(void *p)
     CURLDataCache *cache = NULL;
     CURLAIOCB *acb = p;
     BDRVCURLState *s = acb->common.bs->opaque;
+    int running;
     size_t aio_base, aio_bytes;
 
     qemu_bh_delete(acb->bh);
@@ -588,8 +583,9 @@ static void curl_readv_bh_cb(void *p)
     aio_base = acb->sector_num * SECTOR_SIZE;
     aio_bytes = acb->nb_sectors * SECTOR_SIZE;
 
-    size_t start = acb->sector_num * SECTOR_SIZE;
-    size_t end;
+    if (aio_base + aio_bytes > s->len) {
+        goto err_release;
+    }
 
     cache = curl_find_cache(s, aio_base, aio_bytes);
     if (cache) {
@@ -600,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;
+
 
 }
 
@@ -669,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);
         assert(cache->use_count == 0);
-- 
1.8.2.3

[Qemu-devel] [PATCH v5 08/11] 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 | 96 ++++++++++++++++++++++++++++++++----------------------------
 1 file changed, 51 insertions(+), 45 deletions(-)

diff --git a/block/curl.c b/block/curl.c
index 4c4752b..3ea2552 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,9 +651,13 @@ static BlockDriverAIOCB *curl_aio_readv(BlockDriverState *bs,
 static void curl_close(BlockDriverState *bs)
 {
     BDRVCURLState *s = bs->opaque;
-    int i;
 
     DPRINTF("CURL: Close\n");
+    if (s->timer) {
+        qemu_del_timer(s->timer);
+        qemu_free_timer(s->timer);
+        s->timer = NULL;
+    }
 
     if (s->timer) {
         qemu_del_timer(s->timer);
@@ -670,16 +665,26 @@ 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);
+        acb->common.cb(acb->common.opaque, -EIO);
+        QLIST_REMOVE(acb, next);
+        qemu_aio_release(acb);
+        acb = NULL;
+    }
 
     while (!QLIST_EMPTY(&s->acbs)) {
         CURLAIOCB *acb = QLIST_FIRST(&s->acbs);
@@ -709,6 +714,7 @@ static void curl_close(BlockDriverState *bs)
     }
 
     g_free(s->url);
+    s->url = NULL;
 }
 
 static int64_t curl_getlength(BlockDriverState *bs)
-- 
1.8.2.3

[Qemu-devel] [PATCH v5 09/11] 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 3ea2552..5adbc84 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 (0 && 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.2.3

[Qemu-devel] [PATCH v5 10/11] 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 5adbc84..b6cc5a0 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.2.3

[Qemu-devel] [PATCH v5 11/11] 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 b6cc5a0..4499445 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.2.3

Re: [Qemu-devel] [PATCH v5 00/11] curl: fix curl read

[Richard W.M. Jones]

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


v5 tested and works for me.

Attached is the test script I'm using.

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: 771 bytes --]

Re: [Qemu-devel] [PATCH v5 01/11] curl: introduce CURLSockInfo to BDRVCURLState.

[Stefan Hajnoczi]

On Thu, May 23, 2013 at 11:37:59AM +0800, Fam Zheng wrote:
> @@ -90,7 +98,16 @@ static int curl_aio_flush(void *opaque);
>  static int curl_sock_cb(CURL *curl, curl_socket_t fd, int action,
>                          void *s, void *sp)
>  {
> +    BDRVCURLState *bs = s;

bs is used for BlockDriverState in block/* code.  I find this confusing.
Usually the custom state is called 's'.  Perhaps just rename the void*
arguments s_ and sp_.

Re: [Qemu-devel] [PATCH v5 05/11] curl: add timer to BDRVCURLState

[Stefan Hajnoczi]

On Thu, May 23, 2013 at 11:38:03AM +0800, Fam Zheng wrote:
> diff --git a/block/curl.c b/block/curl.c
> index fc464ad..4fd5bb9 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 *bs = (BDRVCURLState *)opaque;

Please call it 's'.  'bs' is for BlockDriverState*.

Also, there is no need to cast void* to BDRVCURLState*, the conversion
is implicit.

> +    DPRINTF("curl timeout!\n");
> +    curl_multi_socket_action(bs->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 *bs = (BDRVCURLState *)s;

Same here.

Re: [Qemu-devel] [PATCH v5 06/11] curl: introduce CURLDataCache

[Stefan Hajnoczi]

On Thu, May 23, 2013 at 11:38:04AM +0800, Fam Zheng wrote:
> +typedef struct CURLDataCache {
> +    char *data;
> +    size_t base_pos;

Must be int64_t.  QEMU compiled on 32-bit hosts would only allow 4 GB
images with size_t!

> +    size_t data_len;
> +    size_t write_pos;
> +    /* Ref count for CURLState */
> +    int use_count;

It's better to introduce this field when you add code to use it.  When
possible, don't add unused code in a patch, it makes it harder to
review.

> +static void curl_complete_io(BDRVCURLState *bs, CURLAIOCB *acb,
> +                             CURLDataCache *cache)
> +{
> +    size_t aio_base = acb->sector_num * SECTOR_SIZE;

int64_t

> +    size_t aio_bytes = acb->nb_sectors * SECTOR_SIZE;
> +    size_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: %10zd %10zd\n", aio_base, aio_bytes);

PRId64 for 64-bit aio_base

> @@ -589,26 +577,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;
> +    size_t aio_base, aio_bytes;

int64_t aio_base;

Re: [Qemu-devel] [PATCH v5 07/11] curl: make use of CURLDataCache.

[Stefan Hajnoczi]

On Thu, May 23, 2013 at 11:38:05AM +0800, Fam Zheng wrote:
> @@ -221,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 = (CURLState *)opaque;

You can drop the cast completely because opaque is void*.

> +    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;

Why did you add MIN(realsize, c->data_len - c->write_pos)?  The original
code trusts realsize to be within s->orig_buf.

> -
> -    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) {
> +        size_t aio_base = acb->sector_num * SECTOR_SIZE;

int64_t

> @@ -600,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++;

I don't see where you bump the use_count when a cache lookup is
successful.  Maybe I just missed it in the other patches.

Re: [Qemu-devel] [PATCH v5 08/11] curl: use list to store CURLState

[Stefan Hajnoczi]

On Thu, May 23, 2013 at 11:38:06AM +0800, Fam Zheng wrote:
> @@ -660,9 +651,13 @@ static BlockDriverAIOCB *curl_aio_readv(BlockDriverState *bs,
>  static void curl_close(BlockDriverState *bs)
>  {
>      BDRVCURLState *s = bs->opaque;
> -    int i;
>  
>      DPRINTF("CURL: Close\n");
> +    if (s->timer) {
> +        qemu_del_timer(s->timer);
> +        qemu_free_timer(s->timer);
> +        s->timer = NULL;
> +    }

This hunk is duplicated, see the next line :-).

> @@ -670,16 +665,26 @@ 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);
> +        acb->common.cb(acb->common.opaque, -EIO);
> +        QLIST_REMOVE(acb, next);
> +        qemu_aio_release(acb);
> +        acb = NULL;
> +    }

Duplicated?  See line below.

>  
>      while (!QLIST_EMPTY(&s->acbs)) {
>          CURLAIOCB *acb = QLIST_FIRST(&s->acbs);
> @@ -709,6 +714,7 @@ static void curl_close(BlockDriverState *bs)
>      }
>  
>      g_free(s->url);
> +    s->url = NULL;
>  }
>  

Can s->url = NULL be merged into a previous patch which adds
g_free(s->url)?

Re: [Qemu-devel] [PATCH v5 09/11] curl: add cache quota.

[Stefan Hajnoczi]

On Thu, May 23, 2013 at 11:38:07AM +0800, Fam Zheng wrote:
> 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 3ea2552..5adbc84 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 (0 && s->cache_quota <= 0) {

while (0) :(

Re: [Qemu-devel] [PATCH v5 00/11] curl: fix curl read

[Stefan Hajnoczi]

On Thu, May 23, 2013 at 11:37:58AM +0800, Fam Zheng wrote:
> CURL library API has changed, the current curl driver is not working.
> 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.

Looking pretty good.

It's not clear to me if block/curl.c was broken, besides failing with
HTTP servers that do not support Range:, or which part of this series
fixes the bug(s).  Can you clarify that in the patch description?

I'll test the next revision and audit a little more for memory leaks,
since you are introducing several heap-allocated structures and lists.

Re: [Qemu-devel] [PATCH v5 05/11] curl: add timer to BDRVCURLState

[Fam Zheng]

On Thu, 05/23 15:55, Stefan Hajnoczi wrote:
> On Thu, May 23, 2013 at 11:38:03AM +0800, Fam Zheng wrote:
> > diff --git a/block/curl.c b/block/curl.c
> > index fc464ad..4fd5bb9 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 *bs = (BDRVCURLState *)opaque;
> 
> Please call it 's'.  'bs' is for BlockDriverState*.
> 
> Also, there is no need to cast void* to BDRVCURLState*, the conversion
> is implicit.
> 
> > +    DPRINTF("curl timeout!\n");
> > +    curl_multi_socket_action(bs->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 *bs = (BDRVCURLState *)s;
> 
> Same here.
> 

OK.

-- 
Fam

Re: [Qemu-devel] [PATCH v5 06/11] curl: introduce CURLDataCache

[Fam Zheng]

On Thu, 05/23 16:09, Stefan Hajnoczi wrote:
> On Thu, May 23, 2013 at 11:38:04AM +0800, Fam Zheng wrote:
> > +typedef struct CURLDataCache {
> > +    char *data;
> > +    size_t base_pos;
> 
> Must be int64_t.  QEMU compiled on 32-bit hosts would only allow 4 GB
> images with size_t!

OK.

> 
> > +    size_t data_len;
> > +    size_t write_pos;
> > +    /* Ref count for CURLState */
> > +    int use_count;
> 
> It's better to introduce this field when you add code to use it.  When
> possible, don't add unused code in a patch, it makes it harder to
> review.

Moving to later patch.

> 
> > +static void curl_complete_io(BDRVCURLState *bs, CURLAIOCB *acb,
> > +                             CURLDataCache *cache)
> > +{
> > +    size_t aio_base = acb->sector_num * SECTOR_SIZE;
> 
> int64_t
> 
> > +    size_t aio_bytes = acb->nb_sectors * SECTOR_SIZE;
> > +    size_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: %10zd %10zd\n", aio_base, aio_bytes);
> 
> PRId64 for 64-bit aio_base

OK, thanks.

> 
> > @@ -589,26 +577,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;
> > +    size_t aio_base, aio_bytes;
> 
> int64_t aio_base;

Yes, will change.

-- 
Fam

Re: [Qemu-devel] [PATCH v5 07/11] curl: make use of CURLDataCache.

[Fam Zheng]

On Thu, 05/23 16:23, Stefan Hajnoczi wrote:
> On Thu, May 23, 2013 at 11:38:05AM +0800, Fam Zheng wrote:
> > @@ -221,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 = (CURLState *)opaque;
> 
> You can drop the cast completely because opaque is void*.
> 
> > +    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;
> 
> Why did you add MIN(realsize, c->data_len - c->write_pos)?  The original
> code trusts realsize to be within s->orig_buf.

I don't see an evidence why it's safe here. CURL certainly doesn't know
how much buffer do we have. (man 3 curl_easy_setopt, section
CURLOPT_WRITEFUNCTION)

> 
> > -
> > -    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) {
> > +        size_t aio_base = acb->sector_num * SECTOR_SIZE;
> 
> int64_t
> 
> > @@ -600,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++;
> 
> I don't see where you bump the use_count when a cache lookup is
> successful.  Maybe I just missed it in the other patches.

Use count is for serving as the receiving buffer for submitted CURL
requests. It's not necessary to bump use_count when cache lookup is
successful, since data is immediately copied to guest, no ref to the
cache hold.

-- 
Fam

Re: [Qemu-devel] [PATCH v5 08/11] curl: use list to store CURLState

[Fam Zheng]

On Thu, 05/23 16:32, Stefan Hajnoczi wrote:
> On Thu, May 23, 2013 at 11:38:06AM +0800, Fam Zheng wrote:
> > @@ -660,9 +651,13 @@ static BlockDriverAIOCB *curl_aio_readv(BlockDriverState *bs,
> >  static void curl_close(BlockDriverState *bs)
> >  {
> >      BDRVCURLState *s = bs->opaque;
> > -    int i;
> >  
> >      DPRINTF("CURL: Close\n");
> > +    if (s->timer) {
> > +        qemu_del_timer(s->timer);
> > +        qemu_free_timer(s->timer);
> > +        s->timer = NULL;
> > +    }
> 
> This hunk is duplicated, see the next line :-).

Removing.

> 
> > @@ -670,16 +665,26 @@ 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);
> > +        acb->common.cb(acb->common.opaque, -EIO);
> > +        QLIST_REMOVE(acb, next);
> > +        qemu_aio_release(acb);
> > +        acb = NULL;
> > +    }
> 
> Duplicated?  See line below.

Removing.

> 
> >  
> >      while (!QLIST_EMPTY(&s->acbs)) {
> >          CURLAIOCB *acb = QLIST_FIRST(&s->acbs);
> > @@ -709,6 +714,7 @@ static void curl_close(BlockDriverState *bs)
> >      }
> >  
> >      g_free(s->url);
> > +    s->url = NULL;
> >  }
> >  
> 
> Can s->url = NULL be merged into a previous patch which adds
> g_free(s->url)?

It's introduced before this series. I'll move it to a separate patch.

-- 
Fam

Re: [Qemu-devel] [PATCH v5 07/11] curl: make use of CURLDataCache.

[Stefan Hajnoczi]

On Fri, May 24, 2013 at 11:10:26AM +0800, Fam Zheng wrote:
> On Thu, 05/23 16:23, Stefan Hajnoczi wrote:
> > On Thu, May 23, 2013 at 11:38:05AM +0800, Fam Zheng wrote:
> > > +    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;
> > 
> > Why did you add MIN(realsize, c->data_len - c->write_pos)?  The original
> > code trusts realsize to be within s->orig_buf.
> 
> I don't see an evidence why it's safe here. CURL certainly doesn't know
> how much buffer do we have. (man 3 curl_easy_setopt, section
> CURLOPT_WRITEFUNCTION)

The HTTP request included a Range: header so we should know the total
number of bytes we'll receive.

That said, libcurl may not check so this is defensive programming.  A
malicious server shouldn't be able to overflow our buffer.  A comment or
note in the commit description would be nice to explain semantic changes
like this.

> > > @@ -600,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++;
> > 
> > I don't see where you bump the use_count when a cache lookup is
> > successful.  Maybe I just missed it in the other patches.
> 
> Use count is for serving as the receiving buffer for submitted CURL
> requests. It's not necessary to bump use_count when cache lookup is
> successful, since data is immediately copied to guest, no ref to the
> cache hold.

You're right.

Stefan