All of lore.kernel.org
 help / color / mirror / Atom feed
* [Qemu-devel] [PATCH v3 00/10] curl: fix curl read
@ 2013-05-20  7:03 Fam Zheng
  2013-05-20  7:03 ` [Qemu-devel] [PATCH v3 01/10] curl: introduce CURLSockInfo to BDRVCURLState Fam Zheng
                   ` (10 more replies)
  0 siblings, 11 replies; 16+ messages in thread
From: Fam Zheng @ 2013-05-20  7:03 UTC (permalink / raw)
  To: qemu-devel; +Cc: kwolf, jcody, stefanha

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 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.

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

-- 
1.8.1.4

^ permalink raw reply	[flat|nested] 16+ messages in thread

* [Qemu-devel] [PATCH v3 01/10] curl: introduce CURLSockInfo to BDRVCURLState.
  2013-05-20  7:03 [Qemu-devel] [PATCH v3 00/10] curl: fix curl read Fam Zheng
@ 2013-05-20  7:03 ` Fam Zheng
  2013-05-20  7:03 ` [Qemu-devel] [PATCH v3 02/10] curl: change magic number to sizeof Fam Zheng
                   ` (9 subsequent siblings)
  10 siblings, 0 replies; 16+ messages in thread
From: Fam Zheng @ 2013-05-20  7:03 UTC (permalink / raw)
  To: qemu-devel; +Cc: kwolf, jcody, Fam Zheng, stefanha

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 | 29 +++++++++++++++++++++++++++--
 1 file changed, 27 insertions(+), 2 deletions(-)

diff --git a/block/curl.c b/block/curl.c
index b8935fd..0aede58 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);
@@ -462,8 +479,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 +620,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.1.4

^ permalink raw reply related	[flat|nested] 16+ messages in thread

* [Qemu-devel] [PATCH v3 02/10] curl: change magic number to sizeof
  2013-05-20  7:03 [Qemu-devel] [PATCH v3 00/10] curl: fix curl read Fam Zheng
  2013-05-20  7:03 ` [Qemu-devel] [PATCH v3 01/10] curl: introduce CURLSockInfo to BDRVCURLState Fam Zheng
@ 2013-05-20  7:03 ` Fam Zheng
  2013-05-20  7:03 ` [Qemu-devel] [PATCH v3 03/10] curl: change curl_multi_do to curl_fd_handler Fam Zheng
                   ` (8 subsequent siblings)
  10 siblings, 0 replies; 16+ messages in thread
From: Fam Zheng @ 2013-05-20  7:03 UTC (permalink / raw)
  To: qemu-devel; +Cc: kwolf, jcody, Fam Zheng, stefanha

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 0aede58..901deb2 100644
--- a/block/curl.c
+++ b/block/curl.c
@@ -567,7 +567,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.1.4

^ permalink raw reply related	[flat|nested] 16+ messages in thread

* [Qemu-devel] [PATCH v3 03/10] curl: change curl_multi_do to curl_fd_handler
  2013-05-20  7:03 [Qemu-devel] [PATCH v3 00/10] curl: fix curl read Fam Zheng
  2013-05-20  7:03 ` [Qemu-devel] [PATCH v3 01/10] curl: introduce CURLSockInfo to BDRVCURLState Fam Zheng
  2013-05-20  7:03 ` [Qemu-devel] [PATCH v3 02/10] curl: change magic number to sizeof Fam Zheng
@ 2013-05-20  7:03 ` Fam Zheng
  2013-05-20  7:03 ` [Qemu-devel] [PATCH v3 04/10] curl: fix curl_open Fam Zheng
                   ` (7 subsequent siblings)
  10 siblings, 0 replies; 16+ messages in thread
From: Fam Zheng @ 2013-05-20  7:03 UTC (permalink / raw)
  To: qemu-devel; +Cc: kwolf, jcody, Fam Zheng, stefanha

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 901deb2..dea325e 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);
 
@@ -481,7 +489,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;
@@ -573,7 +580,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.1.4

^ permalink raw reply related	[flat|nested] 16+ messages in thread

* [Qemu-devel] [PATCH v3 04/10] curl: fix curl_open
  2013-05-20  7:03 [Qemu-devel] [PATCH v3 00/10] curl: fix curl read Fam Zheng
                   ` (2 preceding siblings ...)
  2013-05-20  7:03 ` [Qemu-devel] [PATCH v3 03/10] curl: change curl_multi_do to curl_fd_handler Fam Zheng
@ 2013-05-20  7:03 ` Fam Zheng
  2013-05-20  7:03 ` [Qemu-devel] [PATCH v3 05/10] curl: add timer to BDRVCURLState Fam Zheng
                   ` (6 subsequent siblings)
  10 siblings, 0 replies; 16+ messages in thread
From: Fam Zheng @ 2013-05-20  7:03 UTC (permalink / raw)
  To: qemu-devel; +Cc: kwolf, jcody, Fam Zheng, stefanha

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 dea325e..c77b917 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;
 
@@ -467,16 +470,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);
@@ -485,10 +501,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;
@@ -498,8 +517,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.1.4

^ permalink raw reply related	[flat|nested] 16+ messages in thread

* [Qemu-devel] [PATCH v3 05/10] curl: add timer to BDRVCURLState
  2013-05-20  7:03 [Qemu-devel] [PATCH v3 00/10] curl: fix curl read Fam Zheng
                   ` (3 preceding siblings ...)
  2013-05-20  7:03 ` [Qemu-devel] [PATCH v3 04/10] curl: fix curl_open Fam Zheng
@ 2013-05-20  7:03 ` Fam Zheng
  2013-05-20  7:03 ` [Qemu-devel] [PATCH v3 06/10] curl: introduce CURLDataCache Fam Zheng
                   ` (5 subsequent siblings)
  10 siblings, 0 replies; 16+ messages in thread
From: Fam Zheng @ 2013-05-20  7:03 UTC (permalink / raw)
  To: qemu-devel; +Cc: kwolf, jcody, Fam Zheng, stefanha

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 c77b917..2f828ed 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);
@@ -507,6 +540,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);
@@ -632,6 +667,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.1.4

^ permalink raw reply related	[flat|nested] 16+ messages in thread

* [Qemu-devel] [PATCH v3 06/10] curl: introduce CURLDataCache
  2013-05-20  7:03 [Qemu-devel] [PATCH v3 00/10] curl: fix curl read Fam Zheng
                   ` (4 preceding siblings ...)
  2013-05-20  7:03 ` [Qemu-devel] [PATCH v3 05/10] curl: add timer to BDRVCURLState Fam Zheng
@ 2013-05-20  7:03 ` Fam Zheng
  2013-05-20  7:03 ` [Qemu-devel] [PATCH v3 07/10] curl: make use of CURLDataCache Fam Zheng
                   ` (4 subsequent siblings)
  10 siblings, 0 replies; 16+ messages in thread
From: Fam Zheng @ 2013-05-20  7:03 UTC (permalink / raw)
  To: qemu-devel; +Cc: kwolf, jcody, Fam Zheng, stefanha

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 | 135 +++++++++++++++++++++++++++++------------------------------
 1 file changed, 66 insertions(+), 69 deletions(-)

diff --git a/block/curl.c b/block/curl.c
index 2f828ed..be9c32e 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) {
@@ -587,26 +574,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
@@ -689,6 +674,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.1.4

^ permalink raw reply related	[flat|nested] 16+ messages in thread

* [Qemu-devel] [PATCH v3 07/10] curl: make use of CURLDataCache.
  2013-05-20  7:03 [Qemu-devel] [PATCH v3 00/10] curl: fix curl read Fam Zheng
                   ` (5 preceding siblings ...)
  2013-05-20  7:03 ` [Qemu-devel] [PATCH v3 06/10] curl: introduce CURLDataCache Fam Zheng
@ 2013-05-20  7:03 ` Fam Zheng
  2013-05-20  7:03 ` [Qemu-devel] [PATCH v3 08/10] curl: use list to store CURLState Fam Zheng
                   ` (3 subsequent siblings)
  10 siblings, 0 replies; 16+ messages in thread
From: Fam Zheng @ 2013-05-20  7:03 UTC (permalink / raw)
  To: qemu-devel; +Cc: kwolf, jcody, Fam Zheng, stefanha

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 | 156 +++++++++++++++++++++++++++++------------------------------
 1 file changed, 78 insertions(+), 78 deletions(-)

diff --git a/block/curl.c b/block/curl.c
index be9c32e..7336cdb 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,30 +215,31 @@ 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;
+    QLIST_FOREACH(acb, &s->s->acbs, next) {
+        size_t aio_base = acb->sector_num * SECTOR_SIZE;
+        size_t aio_len = acb->nb_sectors * SECTOR_SIZE;
+        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);
         }
     }
 
@@ -275,10 +270,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 +285,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 +312,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 +372,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,
@@ -548,14 +544,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;
 }
@@ -577,6 +567,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);
@@ -585,8 +576,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) {
@@ -597,29 +589,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;
+
 
 }
 
@@ -666,10 +670,6 @@ 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);
-- 
1.8.1.4

^ permalink raw reply related	[flat|nested] 16+ messages in thread

* [Qemu-devel] [PATCH v3 08/10] curl: use list to store CURLState
  2013-05-20  7:03 [Qemu-devel] [PATCH v3 00/10] curl: fix curl read Fam Zheng
                   ` (6 preceding siblings ...)
  2013-05-20  7:03 ` [Qemu-devel] [PATCH v3 07/10] curl: make use of CURLDataCache Fam Zheng
@ 2013-05-20  7:03 ` Fam Zheng
  2013-05-20  7:03 ` [Qemu-devel] [PATCH v3 09/10] curl: add cache quota Fam Zheng
                   ` (2 subsequent siblings)
  10 siblings, 0 replies; 16+ messages in thread
From: Fam Zheng @ 2013-05-20  7:03 UTC (permalink / raw)
  To: qemu-devel; +Cc: kwolf, jcody, Fam Zheng, stefanha

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 | 95 ++++++++++++++++++++++++++++++++----------------------------
 1 file changed, 50 insertions(+), 45 deletions(-)

diff --git a/block/curl.c b/block/curl.c
index 7336cdb..f7358de 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;
@@ -300,6 +298,9 @@ static void curl_fd_handler(void *arg)
                 }
 
                 curl_clean_state(state);
+                QLIST_REMOVE(state, next);
+                g_free(state);
+                state = NULL;
                 break;
             }
             default:
@@ -311,29 +312,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);
@@ -359,19 +348,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);
@@ -513,7 +502,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!
@@ -603,6 +593,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++;
@@ -624,7 +615,6 @@ err_release:
     qemu_aio_release(acb);
     return;
 
-
 }
 
 static BlockDriverAIOCB *curl_aio_readv(BlockDriverState *bs,
@@ -653,9 +643,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);
@@ -663,16 +657,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);
+        g_free(acb);
+        acb = NULL;
+    }
 
     while (!QLIST_EMPTY(&s->cache)) {
         CURLDataCache *cache = QLIST_FIRST(&s->cache);
@@ -694,6 +698,7 @@ static void curl_close(BlockDriverState *bs)
     }
 
     g_free(s->url);
+    s->url = NULL;
 }
 
 static int64_t curl_getlength(BlockDriverState *bs)
-- 
1.8.1.4

^ permalink raw reply related	[flat|nested] 16+ messages in thread

* [Qemu-devel] [PATCH v3 09/10] curl: add cache quota.
  2013-05-20  7:03 [Qemu-devel] [PATCH v3 00/10] curl: fix curl read Fam Zheng
                   ` (7 preceding siblings ...)
  2013-05-20  7:03 ` [Qemu-devel] [PATCH v3 08/10] curl: use list to store CURLState Fam Zheng
@ 2013-05-20  7:03 ` Fam Zheng
  2013-05-20  7:03 ` [Qemu-devel] [PATCH v3 10/10] curl: introduce ssl_no_cert runtime option Fam Zheng
  2013-05-20  8:41 ` [Qemu-devel] [PATCH v3 00/10] curl: fix curl read Richard W.M. Jones
  10 siblings, 0 replies; 16+ messages in thread
From: Fam Zheng @ 2013-05-20  7:03 UTC (permalink / raw)
  To: qemu-devel; +Cc: kwolf, jcody, Fam Zheng, stefanha

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 f7358de..f4e2571 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;
@@ -518,6 +521,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:
@@ -587,6 +592,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,
@@ -597,6 +623,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.1.4

^ permalink raw reply related	[flat|nested] 16+ messages in thread

* [Qemu-devel] [PATCH v3 10/10] curl: introduce ssl_no_cert runtime option.
  2013-05-20  7:03 [Qemu-devel] [PATCH v3 00/10] curl: fix curl read Fam Zheng
                   ` (8 preceding siblings ...)
  2013-05-20  7:03 ` [Qemu-devel] [PATCH v3 09/10] curl: add cache quota Fam Zheng
@ 2013-05-20  7:03 ` Fam Zheng
  2013-05-20  8:41 ` [Qemu-devel] [PATCH v3 00/10] curl: fix curl read Richard W.M. Jones
  10 siblings, 0 replies; 16+ messages in thread
From: Fam Zheng @ 2013-05-20  7:03 UTC (permalink / raw)
  To: qemu-devel; +Cc: kwolf, jcody, Fam Zheng, stefanha

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 f4e2571..9352c6c 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);
@@ -336,6 +338,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
@@ -426,7 +430,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 */ }
     },
 };
 
@@ -464,6 +473,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.1.4

^ permalink raw reply related	[flat|nested] 16+ messages in thread

* Re: [Qemu-devel] [PATCH v3 00/10] curl: fix curl read
  2013-05-20  7:03 [Qemu-devel] [PATCH v3 00/10] curl: fix curl read Fam Zheng
                   ` (9 preceding siblings ...)
  2013-05-20  7:03 ` [Qemu-devel] [PATCH v3 10/10] curl: introduce ssl_no_cert runtime option Fam Zheng
@ 2013-05-20  8:41 ` Richard W.M. Jones
  2013-05-20  8:49   ` Richard W.M. Jones
  10 siblings, 1 reply; 16+ messages in thread
From: Richard W.M. Jones @ 2013-05-20  8:41 UTC (permalink / raw)
  To: Fam Zheng; +Cc: kwolf, jcody, qemu-devel, stefanha

On Mon, May 20, 2013 at 03:03:34PM +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. 

I tried this, but it segfaults:

Program terminated with signal 11, Segmentation fault.
#0  curl_read_cb (ptr=<optimized out>, size=<optimized out>, 
    nmemb=<optimized out>, opaque=0x7f09d2975340) at block/curl.c:240
240         size_t aio_base = acb->sector_num * SECTOR_SIZE;
Missing separate debuginfos, use: debuginfo-install SDL-1.2.15-3.fc18.x86_64 bluez-libs-4.101-6.fc18.x86_64 brlapi-0.5.6-12.fc18.x86_64 celt051-0.5.1.3-5.fc18.x86_64 ceph-devel-0.56.3-1.fc18.x86_64 ceph-libs-0.56.3-1.fc18.x86_64 cryptopp-5.6.2-2.fc18.x86_64 cyrus-sasl-lib-2.1.25-2.fc18.x86_64 leveldb-1.7.0-4.fc18.x86_64 libfdt-1.3.0-5.fc18.x86_64 libseccomp-1.0.1-0.fc18.x86_64 libselinux-2.1.12-7.3.fc18.x86_64 libusbx-1.0.14-1.fc18.x86_64 nss-mdns-0.10-11.fc18.x86_64 snappy-1.0.5-2.fc18.x86_64 spice-server-0.12.2-3.fc18.x86_64 usbredir-0.6-1.fc18.x86_64 xen-libs-4.2.2-3.fc18.x86_64
(gdb) bt
#0  curl_read_cb (ptr=<optimized out>, size=<optimized out>, 
    nmemb=<optimized out>, opaque=0x7f09d2975340) at block/curl.c:240
#1  0x00007f09cc7ee0e8 in Curl_client_write (conn=conn@entry=0x7f09d2996c80, 
    type=type@entry=1, ptr=0x7f09d298e8f0 "", len=2046) at sendf.c:449
#2  0x00007f09cc801c52 in readwrite_data (done=0x7ffff70dac77, 
    didwhat=<synthetic pointer>, k=0x7f09d298e080, conn=0x7f09d2996c80, data=
    0x7f09d298e050) at transfer.c:705
#3  Curl_readwrite (conn=0x7f09d2996c80, done=done@entry=0x7ffff70dac77)
    at transfer.c:1023
#4  0x00007f09cc80a4d2 in multi_runsingle (multi=multi@entry=0x7f09d29815b0, 
    now=..., easy=0x7f09d29756a0) at multi.c:1430
#5  0x00007f09cc80b559 in multi_socket (multi=multi@entry=0x7f09d29815b0, 
    checkall=checkall@entry=false, s=10, ev_bitmask=3, 
    running_handles=running_handles@entry=0x7ffff70dad88) at multi.c:2140
#6  0x00007f09cc80b64f in curl_multi_socket_action (multi_handle=
    0x7f09d29815b0, s=<optimized out>, ev_bitmask=<optimized out>, 
    running_handles=running_handles@entry=0x7ffff70dad88) at multi.c:2258
#7  0x00007f09d0077043 in curl_fd_handler (arg=0x7f09d2997410)
    at block/curl.c:265
#8  0x00007f09d005abbb in aio_dispatch (ctx=0x7f09d296e3f0, ctx=0x7f09d296e3f0)
    at aio-posix.c:149
#9  0x00007f09d005b0b1 in aio_poll (ctx=0x7f09d296e3f0, 
    blocking=blocking@entry=true) at aio-posix.c:248
#10 0x00007f09d019f9c9 in qemu_aio_wait () at main-loop.c:484
#11 0x00007f09d0070b65 in bdrv_rwv_co (bs=bs@entry=0x7f09d29746e0, 
    sector_num=sector_num@entry=0, qiov=qiov@entry=0x7ffff70daea0, 
    is_write=is_write@entry=false) at block.c:2215
#12 0x00007f09d0070c90 in bdrv_rw_co (is_write=false, 
    nb_sectors=<optimized out>, buf=<optimized out>, sector_num=0, bs=
    0x7f09d29746e0) at block.c:2234
#13 bdrv_read (bs=bs@entry=0x7f09d29746e0, sector_num=sector_num@entry=0, 
    buf=buf@entry=0x7ffff70db190 "3\300\216м", nb_sectors=nb_sectors@entry=4)
    at block.c:2241
#14 0x00007f09d0070d82 in bdrv_pread (bs=0x7f09d29746e0, offset=offset@entry=
    0, buf=buf@entry=0x7ffff70db190, count1=count1@entry=2048) at block.c:2303
#15 0x00007f09d0071460 in find_image_format (pdrv=<synthetic pointer>, 
    filename=0x7f09d2971b40 "http://192.168.0.249/scratch/winxp.img", 
    bs=<optimized out>) at block.c:533
#16 bdrv_open (bs=0x7f09d2971cc0, filename=filename@entry=
    0x7f09d2971b40 "http://192.168.0.249/scratch/winxp.img", options=
    0x7f09d29726a0, options@entry=0x7f09d2970980, flags=8258, drv=drv@entry=
    0x0) at block.c:1047
#17 0x00007f09d00a4093 in drive_init (all_opts=0x7f09d296d420, 
    block_default_type=IF_NONE) at blockdev.c:698
#18 0x00007f09d020e54b in drive_init_func (opts=<optimized out>, 
    opaque=<optimized out>) at vl.c:1117
#19 0x00007f09d03386f3 in qemu_opts_foreach (list=<optimized out>, 
    func=func@entry=0x7f09d020e530 <drive_init_func>, opaque=opaque@entry=
    0x7f09d07318f0 <pc_i440fx_machine_v1_5+48>, 
    abort_on_failure=abort_on_failure@entry=1) at util/qemu-option.c:1162
#20 0x00007f09d0055b89 in main (argc=<optimized out>, argv=<optimized out>, 
    envp=<optimized out>) at vl.c:4201

I am using qemu from git + your patches, and curl-7_30_0-147-gae26ee3.

	- - - -

A second, unrelated problem with the curl driver is to do with the way
it resolves hostnames.  I have a host which has IPv6 and IPv4 records
in DNS (ie. AAAA and A).  For some reason the IPv6 address isn't
reachable, but this doesn't matter for most clients since they fall
back to using the IPv4 address after a brief timeout.  However the
qemu curl driver does *not* fallback.  It gives up after the IPv6
address fails.

It'd be nice if this problem could be fixed too.

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
virt-top is 'top' for virtual machines.  Tiny program with many
powerful monitoring features, net stats, disk stats, logging, etc.
http://people.redhat.com/~rjones/virt-top

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [Qemu-devel] [PATCH v3 00/10] curl: fix curl read
  2013-05-20  8:41 ` [Qemu-devel] [PATCH v3 00/10] curl: fix curl read Richard W.M. Jones
@ 2013-05-20  8:49   ` Richard W.M. Jones
  2013-05-21  1:54     ` Fam Zheng
  0 siblings, 1 reply; 16+ messages in thread
From: Richard W.M. Jones @ 2013-05-20  8:49 UTC (permalink / raw)
  To: Fam Zheng; +Cc: kwolf, jcody, qemu-devel, stefanha

On Mon, May 20, 2013 at 09:41:06AM +0100, Richard W.M. Jones wrote:
> On Mon, May 20, 2013 at 03:03:34PM +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. 
> 
> I tried this, but it segfaults:
> 
> Program terminated with signal 11, Segmentation fault.

That stack trace was wrong.  I was testing against the version of
libcurl in Fedora which is known to be broken.

Here is the stack trace, this time really running against
curl-7_30_0-147-gae26ee3:

Program terminated with signal 11, Segmentation fault.
#0  curl_read_cb (ptr=<optimized out>, size=<optimized out>, 
    nmemb=<optimized out>, opaque=0x7f63d48ba340) at block/curl.c:240
240         size_t aio_base = acb->sector_num * SECTOR_SIZE;
Missing separate debuginfos, use: debuginfo-install SDL-1.2.15-3.fc18.x86_64 bluez-libs-4.101-6.fc18.x86_64 brlapi-0.5.6-12.fc18.x86_64 celt051-0.5.1.3-5.fc18.x86_64 ceph-devel-0.56.3-1.fc18.x86_64 ceph-libs-0.56.3-1.fc18.x86_64 cryptopp-5.6.2-2.fc18.x86_64 cyrus-sasl-lib-2.1.25-2.fc18.x86_64 leveldb-1.7.0-4.fc18.x86_64 libfdt-1.3.0-5.fc18.x86_64 libseccomp-1.0.1-0.fc18.x86_64 libselinux-2.1.12-7.3.fc18.x86_64 libusbx-1.0.14-1.fc18.x86_64 nss-mdns-0.10-11.fc18.x86_64 snappy-1.0.5-2.fc18.x86_64 spice-server-0.12.2-3.fc18.x86_64 usbredir-0.6-1.fc18.x86_64 xen-libs-4.2.2-3.fc18.x86_64
(gdb) bt
#0  curl_read_cb (ptr=<optimized out>, size=<optimized out>, 
    nmemb=<optimized out>, opaque=0x7f63d48ba340) at block/curl.c:240
#1  0x00007f63cef51cc8 in Curl_client_write ()
   from /home/rjones/d/curl/lib/.libs/libcurl.so.4
#2  0x00007f63cef697ef in Curl_readwrite ()
   from /home/rjones/d/curl/lib/.libs/libcurl.so.4
#3  0x00007f63cef710b0 in multi_runsingle ()
   from /home/rjones/d/curl/lib/.libs/libcurl.so.4
#4  0x00007f63cef720f7 in multi_socket ()
   from /home/rjones/d/curl/lib/.libs/libcurl.so.4
#5  0x00007f63cef721df in curl_multi_socket_action ()
   from /home/rjones/d/curl/lib/.libs/libcurl.so.4
#6  0x00007f63d27d9043 in curl_fd_handler (arg=0x7f63d48e16c0)
    at block/curl.c:265
#7  0x00007f63d27bcbbb in aio_dispatch (ctx=0x7f63d48b33f0, ctx=0x7f63d48b33f0)
    at aio-posix.c:149
#8  0x00007f63d27bd0b1 in aio_poll (ctx=0x7f63d48b33f0, 
    blocking=blocking@entry=true) at aio-posix.c:248
#9  0x00007f63d29019c9 in qemu_aio_wait () at main-loop.c:484
#10 0x00007f63d27d2b65 in bdrv_rwv_co (bs=bs@entry=0x7f63d48b96e0, 
    sector_num=sector_num@entry=0, qiov=qiov@entry=0x7fff2c127e10, 
    is_write=is_write@entry=false) at block.c:2215
#11 0x00007f63d27d2c90 in bdrv_rw_co (is_write=false, 
    nb_sectors=<optimized out>, buf=<optimized out>, sector_num=0, bs=
    0x7f63d48b96e0) at block.c:2234
#12 bdrv_read (bs=bs@entry=0x7f63d48b96e0, sector_num=sector_num@entry=0, 
    buf=buf@entry=0x7fff2c128100 "3\300\216м", nb_sectors=nb_sectors@entry=4)
    at block.c:2241
#13 0x00007f63d27d2d82 in bdrv_pread (bs=0x7f63d48b96e0, offset=offset@entry=
    0, buf=buf@entry=0x7fff2c128100, count1=count1@entry=2048) at block.c:2303
#14 0x00007f63d27d3460 in find_image_format (pdrv=<synthetic pointer>, 
    filename=0x7f63d48b6b40 "http://192.168.0.249/scratch/winxp.img", 
    bs=<optimized out>) at block.c:533
#15 bdrv_open (bs=0x7f63d48b6cc0, filename=filename@entry=
    0x7f63d48b6b40 "http://192.168.0.249/scratch/winxp.img", options=
    0x7f63d48b76a0, options@entry=0x7f63d48b5980, flags=8258, drv=drv@entry=
    0x0) at block.c:1047
#16 0x00007f63d2806093 in drive_init (all_opts=0x7f63d48b2420, 
    block_default_type=IF_NONE) at blockdev.c:698
#17 0x00007f63d297054b in drive_init_func (opts=<optimized out>, 
    opaque=<optimized out>) at vl.c:1117
#18 0x00007f63d2a9a6f3 in qemu_opts_foreach (list=<optimized out>, 
    func=func@entry=0x7f63d2970530 <drive_init_func>, opaque=opaque@entry=
    0x7f63d2e938f0 <pc_i440fx_machine_v1_5+48>, 
    abort_on_failure=abort_on_failure@entry=1) at util/qemu-option.c:1162
#19 0x00007f63d27b7b89 in main (argc=<optimized out>, argv=<optimized out>, 
    envp=<optimized out>) at vl.c:4201

---

For completeness:

(1) qemu from git (without your patches) works.

(2) I'm testing using the following command:

$ LD_LIBRARY_PATH=~/d/curl/lib/.libs \
  LIBGUESTFS_BACKEND=direct \
  LIBGUESTFS_QEMU=~/d/qemu/qemu.wrapper \
  ./run ./fish/guestfish -a http://192.168.0.249/scratch/winxp.img -i -v

where:

(a) ~/d/libguestfs contains libguestfs from git
(b) ~/d/curl contains curl-7_30_0-147-gae26ee3
(c) http://192.168.0.249/scratch/winxp.img is a Windows XP image
(d) qemu.wrapper is:

----------------------------------------------------------------------
#!/bin/sh -
qemudir=/home/rjones/d/qemu
exec $qemudir/x86_64-softmmu/qemu-system-x86_64 -L $qemudir/pc-bios "$@"
----------------------------------------------------------------------

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

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [Qemu-devel] [PATCH v3 00/10] curl: fix curl read
  2013-05-20  8:49   ` Richard W.M. Jones
@ 2013-05-21  1:54     ` Fam Zheng
  2013-05-21  7:39       ` Richard W.M. Jones
  0 siblings, 1 reply; 16+ messages in thread
From: Fam Zheng @ 2013-05-21  1:54 UTC (permalink / raw)
  To: Richard W.M. Jones; +Cc: kwolf, jcody, qemu-devel, stefanha

On Mon, 05/20 09:49, Richard W.M. Jones wrote:
> On Mon, May 20, 2013 at 09:41:06AM +0100, Richard W.M. Jones wrote:
> > On Mon, May 20, 2013 at 03:03:34PM +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. 
> > 
> > I tried this, but it segfaults:
> > 
> > Program terminated with signal 11, Segmentation fault.
> 
> That stack trace was wrong.  I was testing against the version of
> libcurl in Fedora which is known to be broken.
> 
> Here is the stack trace, this time really running against
> curl-7_30_0-147-gae26ee3:
> 
> Program terminated with signal 11, Segmentation fault.
> #0  curl_read_cb (ptr=<optimized out>, size=<optimized out>, 
>     nmemb=<optimized out>, opaque=0x7f63d48ba340) at block/curl.c:240
> 240         size_t aio_base = acb->sector_num * SECTOR_SIZE;

Looks like a memory corrupt (QLIST head is invalid pointer). But I can't
reproduce here with your steps. Can you try qemu-io?

$LD_LIBRARY_PATH=~/d/curl/lib/.libs ~/d/qemu/qemu-io http://192.168.0.249/scratch/winxp.img -c 'read 0 512'

Thanks.

-- 
Fam

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [Qemu-devel] [PATCH v3 00/10] curl: fix curl read
  2013-05-21  1:54     ` Fam Zheng
@ 2013-05-21  7:39       ` Richard W.M. Jones
  2013-05-22  2:52         ` Fam Zheng
  0 siblings, 1 reply; 16+ messages in thread
From: Richard W.M. Jones @ 2013-05-21  7:39 UTC (permalink / raw)
  To: qemu-devel, kwolf, jcody, stefanha

On Tue, May 21, 2013 at 09:54:15AM +0800, Fam Zheng wrote:
> On Mon, 05/20 09:49, Richard W.M. Jones wrote:
> > On Mon, May 20, 2013 at 09:41:06AM +0100, Richard W.M. Jones wrote:
> > > On Mon, May 20, 2013 at 03:03:34PM +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. 
> > > 
> > > I tried this, but it segfaults:
> > > 
> > > Program terminated with signal 11, Segmentation fault.
> > 
> > That stack trace was wrong.  I was testing against the version of
> > libcurl in Fedora which is known to be broken.
> > 
> > Here is the stack trace, this time really running against
> > curl-7_30_0-147-gae26ee3:
> > 
> > Program terminated with signal 11, Segmentation fault.
> > #0  curl_read_cb (ptr=<optimized out>, size=<optimized out>, 
> >     nmemb=<optimized out>, opaque=0x7f63d48ba340) at block/curl.c:240
> > 240         size_t aio_base = acb->sector_num * SECTOR_SIZE;
> 
> Looks like a memory corrupt (QLIST head is invalid pointer). But I can't
> reproduce here with your steps. Can you try qemu-io?
> 
> $LD_LIBRARY_PATH=~/d/curl/lib/.libs ~/d/qemu/qemu-io http://192.168.0.249/scratch/winxp.img -c 'read 0 512'

This command is successful:

$ LD_LIBRARY_PATH=~/d/curl/lib/.libs ~/d/qemu/qemu-io http://192.168.0.249/scratch/winxp.img -c 'read 0 512'
read 512/512 bytes at offset 0
512 bytes, 1 ops; 0.0000 sec (32.552 MiB/sec and 66666.6667 ops/sec)
$ echo $?
0

Here's another go with guestfish:

$ ulimit -c unlimited
$ LIBGUESTFS_DEBUG=1 LIBGUESTFS_TRACE=1 LIBGUESTFS_BACKEND=direct LIBGUESTFS_QEMU=~/d/qemu/qemu.wrapper LD_LIBRARY_PATH=~/d/curl/lib/.libs PATH=~/d/qemu:$PATH ./run ./fish/guestfish -a http://192.168.0.249/scratch/winxp.img -i
[...]
[00159ms] /home/rjones/d/qemu/qemu.wrapper \
    -global virtio-blk-pci.scsi=off \
    -nodefconfig \
    -nodefaults \
    -nographic \
    -device virtio-scsi-pci,id=scsi \
    -drive file=http://192.168.0.249/scratch/winxp.img,id=hd0,if=none \
    -device scsi-hd,drive=hd0 \
    -drive file=/home/rjones/d/libguestfs/tmp/.guestfs-1000/root.15535,snapshot=on,id=appliance,if=none,cache=unsafe \
    -device scsi-hd,drive=appliance \
    -machine accel=kvm:tcg \
    -m 500 \
    -no-reboot \
    -no-hpet \
    -device virtio-serial \
    -serial stdio \
    -device sga \
    -chardev socket,path=/home/rjones/d/libguestfs/tmp/libguestfsk9fu9P/guestfsd.sock,id=channel0 \
    -device virtserialport,chardev=channel0,name=org.libguestfs.channel.0 \
    -kernel /home/rjones/d/libguestfs/tmp/.guestfs-1000/kernel.15535 \
    -initrd /home/rjones/d/libguestfs/tmp/.guestfs-1000/initrd.15535 \
    -append 'panic=1 console=ttyS0 udevtimeout=600 no_timer_check acpi=off printk.time=1 cgroup_disable=memory root=/dev/sdb selinux=0 guestfs_verbose=1 TERM=xterm-256color'libguestfs: error: appliance closed the connection unexpectedly, see earlier error messages
libguestfs: child_cleanup: 0x1db0090: child process died
libguestfs: sending SIGTERM to process 15600
libguestfs: error: /home/rjones/d/qemu/qemu.wrapper killed by signal 11 (Segmentation fault), see debug messages above
libguestfs: error: guestfs_launch failed, see earlier error messages
libguestfs: trace: launch = -1 (error)
[...]

$ file /tmp/core.15600
/tmp/core.15600: ELF 64-bit LSB core file x86-64, version 1 (SYSV), SVR4-style, from '/home/rjones/d/qemu/x86_64-softmmu/qemu-system-x86_64 -L /home/rjones/d/qemu/pc'

$ gdb /home/rjones/d/qemu/x86_64-softmmu/qemu-system-x86_64 /tmp/core.15600

[stack trace is the same as before]

#0  curl_read_cb (ptr=<optimized out>, size=<optimized out>, 
    nmemb=<optimized out>, opaque=0x7f4d3c769360) at block/curl.c:240
240         size_t aio_base = acb->sector_num * SECTOR_SIZE;
(gdb) print acb
$1 = (CURLAIOCB *) 0x7575757575757575

Looks like use-after-free?

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
virt-top is 'top' for virtual machines.  Tiny program with many
powerful monitoring features, net stats, disk stats, logging, etc.
http://people.redhat.com/~rjones/virt-top

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [Qemu-devel] [PATCH v3 00/10] curl: fix curl read
  2013-05-21  7:39       ` Richard W.M. Jones
@ 2013-05-22  2:52         ` Fam Zheng
  0 siblings, 0 replies; 16+ messages in thread
From: Fam Zheng @ 2013-05-22  2:52 UTC (permalink / raw)
  To: Richard W.M. Jones; +Cc: kwolf, jcody, qemu-devel, stefanha

On Tue, 05/21 08:39, Richard W.M. Jones wrote:
> On Tue, May 21, 2013 at 09:54:15AM +0800, Fam Zheng wrote:
> > On Mon, 05/20 09:49, Richard W.M. Jones wrote:
> > > On Mon, May 20, 2013 at 09:41:06AM +0100, Richard W.M. Jones wrote:
> > > > On Mon, May 20, 2013 at 03:03:34PM +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. 
> > > > 
> > > > I tried this, but it segfaults:
> > > > 
> > > > Program terminated with signal 11, Segmentation fault.
> > > 
> > > That stack trace was wrong.  I was testing against the version of
> > > libcurl in Fedora which is known to be broken.
> > > 
> > > Here is the stack trace, this time really running against
> > > curl-7_30_0-147-gae26ee3:
> > > 
> > > Program terminated with signal 11, Segmentation fault.
> > > #0  curl_read_cb (ptr=<optimized out>, size=<optimized out>, 
> > >     nmemb=<optimized out>, opaque=0x7f63d48ba340) at block/curl.c:240
> > > 240         size_t aio_base = acb->sector_num * SECTOR_SIZE;
> > 
> > Looks like a memory corrupt (QLIST head is invalid pointer). But I can't
> > reproduce here with your steps. Can you try qemu-io?
> > 
> > $LD_LIBRARY_PATH=~/d/curl/lib/.libs ~/d/qemu/qemu-io http://192.168.0.249/scratch/winxp.img -c 'read 0 512'
> 
> This command is successful:
> 
> $ LD_LIBRARY_PATH=~/d/curl/lib/.libs ~/d/qemu/qemu-io http://192.168.0.249/scratch/winxp.img -c 'read 0 512'
> read 512/512 bytes at offset 0
> 512 bytes, 1 ops; 0.0000 sec (32.552 MiB/sec and 66666.6667 ops/sec)
> $ echo $?
> 0
> 
> Here's another go with guestfish:
> 
> $ ulimit -c unlimited
> $ LIBGUESTFS_DEBUG=1 LIBGUESTFS_TRACE=1 LIBGUESTFS_BACKEND=direct LIBGUESTFS_QEMU=~/d/qemu/qemu.wrapper LD_LIBRARY_PATH=~/d/curl/lib/.libs PATH=~/d/qemu:$PATH ./run ./fish/guestfish -a http://192.168.0.249/scratch/winxp.img -i
> [...]
> [00159ms] /home/rjones/d/qemu/qemu.wrapper \
>     -global virtio-blk-pci.scsi=off \
>     -nodefconfig \
>     -nodefaults \
>     -nographic \
>     -device virtio-scsi-pci,id=scsi \
>     -drive file=http://192.168.0.249/scratch/winxp.img,id=hd0,if=none \
>     -device scsi-hd,drive=hd0 \
>     -drive file=/home/rjones/d/libguestfs/tmp/.guestfs-1000/root.15535,snapshot=on,id=appliance,if=none,cache=unsafe \
>     -device scsi-hd,drive=appliance \
>     -machine accel=kvm:tcg \
>     -m 500 \
>     -no-reboot \
>     -no-hpet \
>     -device virtio-serial \
>     -serial stdio \
>     -device sga \
>     -chardev socket,path=/home/rjones/d/libguestfs/tmp/libguestfsk9fu9P/guestfsd.sock,id=channel0 \
>     -device virtserialport,chardev=channel0,name=org.libguestfs.channel.0 \
>     -kernel /home/rjones/d/libguestfs/tmp/.guestfs-1000/kernel.15535 \
>     -initrd /home/rjones/d/libguestfs/tmp/.guestfs-1000/initrd.15535 \
>     -append 'panic=1 console=ttyS0 udevtimeout=600 no_timer_check acpi=off printk.time=1 cgroup_disable=memory root=/dev/sdb selinux=0 guestfs_verbose=1 TERM=xterm-256color'libguestfs: error: appliance closed the connection unexpectedly, see earlier error messages
> libguestfs: child_cleanup: 0x1db0090: child process died
> libguestfs: sending SIGTERM to process 15600
> libguestfs: error: /home/rjones/d/qemu/qemu.wrapper killed by signal 11 (Segmentation fault), see debug messages above
> libguestfs: error: guestfs_launch failed, see earlier error messages
> libguestfs: trace: launch = -1 (error)
> [...]
> 
> $ file /tmp/core.15600
> /tmp/core.15600: ELF 64-bit LSB core file x86-64, version 1 (SYSV), SVR4-style, from '/home/rjones/d/qemu/x86_64-softmmu/qemu-system-x86_64 -L /home/rjones/d/qemu/pc'
> 
> $ gdb /home/rjones/d/qemu/x86_64-softmmu/qemu-system-x86_64 /tmp/core.15600
> 
> [stack trace is the same as before]
> 
> #0  curl_read_cb (ptr=<optimized out>, size=<optimized out>, 
>     nmemb=<optimized out>, opaque=0x7f4d3c769360) at block/curl.c:240
> 240         size_t aio_base = acb->sector_num * SECTOR_SIZE;
> (gdb) print acb
> $1 = (CURLAIOCB *) 0x7575757575757575
> 
> Looks like use-after-free?

Yes, thank you a lot. Will post another version to fix this.

-- 
Fam

^ permalink raw reply	[flat|nested] 16+ messages in thread

end of thread, other threads:[~2013-05-22  3:02 UTC | newest]

Thread overview: 16+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2013-05-20  7:03 [Qemu-devel] [PATCH v3 00/10] curl: fix curl read Fam Zheng
2013-05-20  7:03 ` [Qemu-devel] [PATCH v3 01/10] curl: introduce CURLSockInfo to BDRVCURLState Fam Zheng
2013-05-20  7:03 ` [Qemu-devel] [PATCH v3 02/10] curl: change magic number to sizeof Fam Zheng
2013-05-20  7:03 ` [Qemu-devel] [PATCH v3 03/10] curl: change curl_multi_do to curl_fd_handler Fam Zheng
2013-05-20  7:03 ` [Qemu-devel] [PATCH v3 04/10] curl: fix curl_open Fam Zheng
2013-05-20  7:03 ` [Qemu-devel] [PATCH v3 05/10] curl: add timer to BDRVCURLState Fam Zheng
2013-05-20  7:03 ` [Qemu-devel] [PATCH v3 06/10] curl: introduce CURLDataCache Fam Zheng
2013-05-20  7:03 ` [Qemu-devel] [PATCH v3 07/10] curl: make use of CURLDataCache Fam Zheng
2013-05-20  7:03 ` [Qemu-devel] [PATCH v3 08/10] curl: use list to store CURLState Fam Zheng
2013-05-20  7:03 ` [Qemu-devel] [PATCH v3 09/10] curl: add cache quota Fam Zheng
2013-05-20  7:03 ` [Qemu-devel] [PATCH v3 10/10] curl: introduce ssl_no_cert runtime option Fam Zheng
2013-05-20  8:41 ` [Qemu-devel] [PATCH v3 00/10] curl: fix curl read Richard W.M. Jones
2013-05-20  8:49   ` Richard W.M. Jones
2013-05-21  1:54     ` Fam Zheng
2013-05-21  7:39       ` Richard W.M. Jones
2013-05-22  2:52         ` Fam Zheng

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.