[Qemu-devel] [PATCH 0/6] block/curl: Fix hang and potential crash

[Qemu-devel] [PATCH 0/6] block/curl: Fix hang and potential crash

[Max Reitz]

Hi,

As reported in https://bugzilla.redhat.com/show_bug.cgi?id=1740193, our
curl block driver can spontaneously hang.  This becomes visible e.g.
when reading compressed qcow2 images:

$ qemu-img convert -p -O raw -n \
  https://download.cirros-cloud.net/0.4.0/cirros-0.4.0-x86_64-disk.img \
  null-co://

(Hangs at 74.21 %, usually.)

A more direct way is:

$ qemu-img bench -f raw http://download.qemu.org/qemu-4.1.0.tar.xz \
    -d 1 -S 524288 -c 2

(Which simply performs two requests, and the second one hangs.  You can
use any HTTP resource (probably FTP, too) you’d like that is at least
1 MB in size.)

It turns out that this is because cURL 7.59.0 has added a protective
feature against some misuse we had in our code: curl_multi_add_handle()
must not be called from within a cURL callback, but in some cases we
did.  As of 7.59.0, this fails, our new request is not registered and
the I/O request stalls.  This is fixed by patch 5.

Patch 6 makes us check for curl_multi_add_handle()’s return code,
because if we had done that before, debugging would have been much
simpler.


On the way to fixing it, I had a look over the whole cURL code and found
a suspicious QLIST_FOREACH_SAFE() loop that actually does not seem very
safe at all.  I think this may lead to crashes, although I have never
seen any myself.  https://bugzilla.redhat.com/show_bug.cgi?id=1744602#c5
shows one in exactly the function in question, so I think it actually is
a problem.

This is fixed by patch 4, patches 1 through 3 prepare for it.


Max Reitz (6):
  curl: Keep pointer to the CURLState in CURLSocket
  curl: Keep *socket until the end of curl_sock_cb()
  curl: Pass CURLSocket to curl_multi_{do,read}()
  curl: Report only ready sockets
  curl: Handle success in multi_check_completion
  curl: Check curl_multi_add_handle()'s return code

 block/curl.c | 129 +++++++++++++++++++++++++--------------------------
 1 file changed, 63 insertions(+), 66 deletions(-)

-- 
2.21.0

[Qemu-devel] [PATCH 1/6] curl: Keep pointer to the CURLState in CURLSocket

[Max Reitz]

A follow-up patch will make curl_multi_do() and curl_multi_read() take a
CURLSocket instead of the CURLState.  They still need the latter,
though, so add a pointer to it to the former.

Cc: qemu-stable@nongnu.org
Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 block/curl.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/block/curl.c b/block/curl.c
index d4c8e94f3e..92dc2f630e 100644
--- a/block/curl.c
+++ b/block/curl.c
@@ -80,6 +80,7 @@ static CURLMcode __curl_multi_socket_action(CURLM *multi_handle,
 #define CURL_BLOCK_OPT_TIMEOUT_DEFAULT 5
 
 struct BDRVCURLState;
+struct CURLState;
 
 static bool libcurl_initialized;
 
@@ -97,6 +98,7 @@ typedef struct CURLAIOCB {
 
 typedef struct CURLSocket {
     int fd;
+    struct CURLState *state;
     QLIST_ENTRY(CURLSocket) next;
 } CURLSocket;
 
@@ -180,6 +182,7 @@ static int curl_sock_cb(CURL *curl, curl_socket_t fd, int action,
     if (!socket) {
         socket = g_new0(CURLSocket, 1);
         socket->fd = fd;
+        socket->state = state;
         QLIST_INSERT_HEAD(&state->sockets, socket, next);
     }
     socket = NULL;
-- 
2.21.0

[Qemu-devel] [PATCH 2/6] curl: Keep *socket until the end of curl_sock_cb()

[Max Reitz]

This does not really change anything, but it makes the code a bit easier
to follow once we use @socket as the opaque pointer for
aio_set_fd_handler().

(Also, this change stops us from creating new CURLSocket objects when
the cURL library just wants to stop listening on an existing socket that
we do not recognize.  With a well-behaving cURL, that should never
happen anyway.)

Cc: qemu-stable@nongnu.org
Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 block/curl.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/block/curl.c b/block/curl.c
index 92dc2f630e..8a45b371cc 100644
--- a/block/curl.c
+++ b/block/curl.c
@@ -174,18 +174,16 @@ static int curl_sock_cb(CURL *curl, curl_socket_t fd, int action,
         if (socket->fd == fd) {
             if (action == CURL_POLL_REMOVE) {
                 QLIST_REMOVE(socket, next);
-                g_free(socket);
             }
             break;
         }
     }
-    if (!socket) {
+    if (action != CURL_POLL_REMOVE && !socket) {
         socket = g_new0(CURLSocket, 1);
         socket->fd = fd;
         socket->state = state;
         QLIST_INSERT_HEAD(&state->sockets, socket, next);
     }
-    socket = NULL;
 
     trace_curl_sock_cb(action, (int)fd);
     switch (action) {
@@ -207,6 +205,9 @@ static int curl_sock_cb(CURL *curl, curl_socket_t fd, int action,
             break;
     }
 
+    if (action == CURL_POLL_REMOVE) {
+        g_free(socket);
+    }
     return 0;
 }
 
-- 
2.21.0

[Qemu-devel] [PATCH 3/6] curl: Pass CURLSocket to curl_multi_{do, read}()

[Max Reitz]

curl_multi_do_locked() currently marks all sockets as ready.  That is
not only inefficient, but in fact unsafe (the loop is).  A follow-up
patch will change that, but to do so, curl_multi_do_locked() needs to
know exactly which socket is ready; and that is accomplished by this
patch here.

Cc: qemu-stable@nongnu.org
Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 block/curl.c | 29 ++++++++++++++++-------------
 1 file changed, 16 insertions(+), 13 deletions(-)

diff --git a/block/curl.c b/block/curl.c
index 8a45b371cc..05f77a38c2 100644
--- a/block/curl.c
+++ b/block/curl.c
@@ -189,15 +189,15 @@ static int curl_sock_cb(CURL *curl, curl_socket_t fd, int action,
     switch (action) {
         case CURL_POLL_IN:
             aio_set_fd_handler(s->aio_context, fd, false,
-                               curl_multi_read, NULL, NULL, state);
+                               curl_multi_read, NULL, NULL, socket);
             break;
         case CURL_POLL_OUT:
             aio_set_fd_handler(s->aio_context, fd, false,
-                               NULL, curl_multi_do, NULL, state);
+                               NULL, curl_multi_do, NULL, socket);
             break;
         case CURL_POLL_INOUT:
             aio_set_fd_handler(s->aio_context, fd, false,
-                               curl_multi_read, curl_multi_do, NULL, state);
+                               curl_multi_read, curl_multi_do, NULL, socket);
             break;
         case CURL_POLL_REMOVE:
             aio_set_fd_handler(s->aio_context, fd, false,
@@ -394,9 +394,10 @@ static void curl_multi_check_completion(BDRVCURLState *s)
 }
 
 /* Called with s->mutex held.  */
-static void curl_multi_do_locked(CURLState *s)
+static void curl_multi_do_locked(CURLSocket *ready_socket)
 {
     CURLSocket *socket, *next_socket;
+    CURLState *s = socket->state;
     int running;
     int r;
 
@@ -415,21 +416,23 @@ static void curl_multi_do_locked(CURLState *s)
 
 static void curl_multi_do(void *arg)
 {
-    CURLState *s = (CURLState *)arg;
+    CURLSocket *socket = arg;
+    BDRVCURLState *s = socket->state->s;
 
-    qemu_mutex_lock(&s->s->mutex);
-    curl_multi_do_locked(s);
-    qemu_mutex_unlock(&s->s->mutex);
+    qemu_mutex_lock(&s->mutex);
+    curl_multi_do_locked(socket);
+    qemu_mutex_unlock(&s->mutex);
 }
 
 static void curl_multi_read(void *arg)
 {
-    CURLState *s = (CURLState *)arg;
+    CURLSocket *socket = arg;
+    BDRVCURLState *s = socket->state->s;
 
-    qemu_mutex_lock(&s->s->mutex);
-    curl_multi_do_locked(s);
-    curl_multi_check_completion(s->s);
-    qemu_mutex_unlock(&s->s->mutex);
+    qemu_mutex_lock(&s->mutex);
+    curl_multi_do_locked(socket);
+    curl_multi_check_completion(s);
+    qemu_mutex_unlock(&s->mutex);
 }
 
 static void curl_multi_timeout_do(void *arg)
-- 
2.21.0

[Qemu-devel] [PATCH 4/6] curl: Report only ready sockets

[Max Reitz]

Instead of reporting all sockets to cURL, only report the one that has
caused curl_multi_do_locked() to be called.  This lets us get rid of the
QLIST_FOREACH_SAFE() list, which was actually wrong: SAFE foreaches are
only safe when the current element is removed in each iteration.  If it
possible for the list to be concurrently modified, we cannot guarantee
that only the current element will be removed.  Therefore, we must not
use QLIST_FOREACH_SAFE() here.

Fixes: ff5ca1664af85b24a4180d595ea6873fd3deac57
Cc: qemu-stable@nongnu.org
Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 block/curl.c | 17 ++++++-----------
 1 file changed, 6 insertions(+), 11 deletions(-)

diff --git a/block/curl.c b/block/curl.c
index 05f77a38c2..bc70f39fcb 100644
--- a/block/curl.c
+++ b/block/curl.c
@@ -394,24 +394,19 @@ static void curl_multi_check_completion(BDRVCURLState *s)
 }
 
 /* Called with s->mutex held.  */
-static void curl_multi_do_locked(CURLSocket *ready_socket)
+static void curl_multi_do_locked(CURLSocket *socket)
 {
-    CURLSocket *socket, *next_socket;
-    CURLState *s = socket->state;
+    BDRVCURLState *s = socket->state->s;
     int running;
     int r;
 
-    if (!s->s->multi) {
+    if (!s->multi) {
         return;
     }
 
-    /* Need to use _SAFE because curl_multi_socket_action() may trigger
-     * curl_sock_cb() which might modify this list */
-    QLIST_FOREACH_SAFE(socket, &s->sockets, next, next_socket) {
-        do {
-            r = curl_multi_socket_action(s->s->multi, socket->fd, 0, &running);
-        } while (r == CURLM_CALL_MULTI_PERFORM);
-    }
+    do {
+        r = curl_multi_socket_action(s->multi, socket->fd, 0, &running);
+    } while (r == CURLM_CALL_MULTI_PERFORM);
 }
 
 static void curl_multi_do(void *arg)
-- 
2.21.0

[Qemu-devel] [PATCH 5/6] curl: Handle success in multi_check_completion

[Max Reitz]

Background: As of cURL 7.59.0, it verifies that several functions are
not called from within a callback.  Among these functions is
curl_multi_add_handle().

curl_read_cb() is a callback from cURL and not a coroutine.  Waking up
acb->co will lead to entering it then and there, which means the current
request will settle and the caller (if it runs in the same coroutine)
may then issue the next request.  In such a case, we will enter
curl_setup_preadv() effectively from within curl_read_cb().

Calling curl_multi_add_handle() will then fail and the new request will
not be processed.

Fix this by not letting curl_read_cb() wake up acb->co.  Instead, leave
the whole business of settling the AIOCB objects to
curl_multi_check_completion() (which is called from our timer callback
and our FD read handler, so not from any cURL callbacks).

Reported-by: Natalie Gavrielov <ngavrilo@redhat.com>
Buglink: https://bugzilla.redhat.com/show_bug.cgi?id=1740193
Cc: qemu-stable@nongnu.org
Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 block/curl.c | 69 ++++++++++++++++++++++------------------------------
 1 file changed, 29 insertions(+), 40 deletions(-)

diff --git a/block/curl.c b/block/curl.c
index bc70f39fcb..5e0cca601d 100644
--- a/block/curl.c
+++ b/block/curl.c
@@ -231,7 +231,6 @@ static size_t curl_read_cb(void *ptr, size_t size, size_t nmemb, void *opaque)
 {
     CURLState *s = ((CURLState*)opaque);
     size_t realsize = size * nmemb;
-    int i;
 
     trace_curl_read_cb(realsize);
 
@@ -247,32 +246,6 @@ static size_t curl_read_cb(void *ptr, size_t size, size_t nmemb, void *opaque)
     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)) {
-            size_t request_length = acb->bytes;
-
-            qemu_iovec_from_buf(acb->qiov, 0, s->orig_buf + acb->start,
-                                acb->end - acb->start);
-
-            if (acb->end - acb->start < request_length) {
-                size_t offset = acb->end - acb->start;
-                qemu_iovec_memset(acb->qiov, offset, 0,
-                                  request_length - offset);
-            }
-
-            acb->ret = 0;
-            s->acb[i] = NULL;
-            qemu_mutex_unlock(&s->s->mutex);
-            aio_co_wake(acb->co);
-            qemu_mutex_lock(&s->s->mutex);
-        }
-    }
-
 read_end:
     /* curl will error out if we do not return this value */
     return size * nmemb;
@@ -353,13 +326,14 @@ static void curl_multi_check_completion(BDRVCURLState *s)
             break;
 
         if (msg->msg == CURLMSG_DONE) {
+            int i;
             CURLState *state = NULL;
+            bool error = msg->data.result != CURLE_OK;
+
             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) {
-                int i;
+            if (error) {
                 static int errcount = 100;
 
                 /* Don't lose the original error message from curl, since
@@ -371,20 +345,35 @@ static void curl_multi_check_completion(BDRVCURLState *s)
                         error_report("curl: further errors suppressed");
                     }
                 }
+            }
 
-                for (i = 0; i < CURL_NUM_ACB; i++) {
-                    CURLAIOCB *acb = state->acb[i];
+            for (i = 0; i < CURL_NUM_ACB; i++) {
+                CURLAIOCB *acb = state->acb[i];
 
-                    if (acb == NULL) {
-                        continue;
-                    }
+                if (acb == NULL) {
+                    continue;
+                }
+
+                if (!error) {
+                    /* Assert that we have read all data */
+                    assert(state->buf_off >= acb->end);
+
+                    qemu_iovec_from_buf(acb->qiov, 0,
+                                        state->orig_buf + acb->start,
+                                        acb->end - acb->start);
 
-                    acb->ret = -EIO;
-                    state->acb[i] = NULL;
-                    qemu_mutex_unlock(&s->mutex);
-                    aio_co_wake(acb->co);
-                    qemu_mutex_lock(&s->mutex);
+                    if (acb->end - acb->start < acb->bytes) {
+                        size_t offset = acb->end - acb->start;
+                        qemu_iovec_memset(acb->qiov, offset, 0,
+                                          acb->bytes - offset);
+                    }
                 }
+
+                acb->ret = error ? -EIO : 0;
+                state->acb[i] = NULL;
+                qemu_mutex_unlock(&s->mutex);
+                aio_co_wake(acb->co);
+                qemu_mutex_lock(&s->mutex);
             }
 
             curl_clean_state(state);
-- 
2.21.0

[Qemu-devel] [PATCH 6/6] curl: Check curl_multi_add_handle()'s return code

[Max Reitz]

If we had done that all along, debugging would have been much simpler.
(Also, I/O errors are better than hangs.)

Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 block/curl.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/block/curl.c b/block/curl.c
index 5e0cca601d..4a7aff02a6 100644
--- a/block/curl.c
+++ b/block/curl.c
@@ -894,7 +894,13 @@ static void curl_setup_preadv(BlockDriverState *bs, CURLAIOCB *acb)
     trace_curl_setup_preadv(acb->bytes, start, state->range);
     curl_easy_setopt(state->curl, CURLOPT_RANGE, state->range);
 
-    curl_multi_add_handle(s->multi, state->curl);
+    if (curl_multi_add_handle(s->multi, state->curl) != CURLM_OK) {
+        state->acb[0] = NULL;
+        acb->ret = -EIO;
+
+        curl_clean_state(state);
+        goto out;
+    }
 
     /* Tell curl it needs to kick things off */
     curl_multi_socket_action(s->multi, CURL_SOCKET_TIMEOUT, 0, &running);
-- 
2.21.0

Re: [Qemu-devel] [Qemu-block] [PATCH 1/6] curl: Keep pointer to the CURLState in CURLSocket

[John Snow]

On 8/27/19 12:34 PM, Max Reitz wrote:
> A follow-up patch will make curl_multi_do() and curl_multi_read() take a
> CURLSocket instead of the CURLState.  They still need the latter,
> though, so add a pointer to it to the former.
> 
> Cc: qemu-stable@nongnu.org
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
>  block/curl.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/block/curl.c b/block/curl.c
> index d4c8e94f3e..92dc2f630e 100644
> --- a/block/curl.c
> +++ b/block/curl.c
> @@ -80,6 +80,7 @@ static CURLMcode __curl_multi_socket_action(CURLM *multi_handle,

The line that git/diff chooses as context is sometimes so very funny.

>  #define CURL_BLOCK_OPT_TIMEOUT_DEFAULT 5
>  
>  struct BDRVCURLState;
> +struct CURLState;
>  
>  static bool libcurl_initialized;
>  
> @@ -97,6 +98,7 @@ typedef struct CURLAIOCB {
>  
>  typedef struct CURLSocket {
>      int fd;
> +    struct CURLState *state;
>      QLIST_ENTRY(CURLSocket) next;
>  } CURLSocket;
>  
> @@ -180,6 +182,7 @@ static int curl_sock_cb(CURL *curl, curl_socket_t fd, int action,
>      if (!socket) {
>          socket = g_new0(CURLSocket, 1);
>          socket->fd = fd;
> +        socket->state = state;
>          QLIST_INSERT_HEAD(&state->sockets, socket, next);
>      }
>      socket = NULL;
> 

So a State contains a list of sockets, and the socket has a link to the
state that created it. OK.

So far so harmless.

Reviewed-by: John Snow <jsnow@redhat.com>

Re: [Qemu-devel] [Qemu-block] [PATCH 2/6] curl: Keep *socket until the end of curl_sock_cb()

[John Snow]

On 8/27/19 12:34 PM, Max Reitz wrote:
> This does not really change anything, but it makes the code a bit easier
> to follow once we use @socket as the opaque pointer for
> aio_set_fd_handler().
> 
> (Also, this change stops us from creating new CURLSocket objects when
> the cURL library just wants to stop listening on an existing socket that
> we do not recognize.  With a well-behaving cURL, that should never
> happen anyway.)
> 
> Cc: qemu-stable@nongnu.org
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
>  block/curl.c | 7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/block/curl.c b/block/curl.c
> index 92dc2f630e..8a45b371cc 100644
> --- a/block/curl.c
> +++ b/block/curl.c
> @@ -174,18 +174,16 @@ static int curl_sock_cb(CURL *curl, curl_socket_t fd, int action,
>          if (socket->fd == fd) {
>              if (action == CURL_POLL_REMOVE) {
>                  QLIST_REMOVE(socket, next);
> -                g_free(socket);
>              }
>              break;
>          }
>      }

> -    if (!socket) {
> +    if (action != CURL_POLL_REMOVE && !socket) {
>          socket = g_new0(CURLSocket, 1);
>          socket->fd = fd;
>          socket->state = state;
>          QLIST_INSERT_HEAD(&state->sockets, socket, next);
>      }
> -    socket = NULL;
>  
>      trace_curl_sock_cb(action, (int)fd);
>      switch (action) {
> @@ -207,6 +205,9 @@ static int curl_sock_cb(CURL *curl, curl_socket_t fd, int action,
>              break;
>      }
>  
> +    if (action == CURL_POLL_REMOVE) {
> +        g_free(socket);
> +    }
>      return 0;
>  }
>  
> 

Very naive question: why is CURL_POLL_REMOVE handled so early in the
function? Why not handle both QLIST_REMOVE and g_free under the
switch(action) construct entirely?

Re: [Qemu-devel] [Qemu-block] [PATCH 3/6] curl: Pass CURLSocket to curl_multi_{do, read}()

[John Snow]

On 8/27/19 12:34 PM, Max Reitz wrote:
> curl_multi_do_locked() currently marks all sockets as ready.  That is
> not only inefficient, but in fact unsafe (the loop is).  A follow-up
> patch will change that, but to do so, curl_multi_do_locked() needs to
> know exactly which socket is ready; and that is accomplished by this
> patch here.
> 
> Cc: qemu-stable@nongnu.org
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
>  block/curl.c | 29 ++++++++++++++++-------------
>  1 file changed, 16 insertions(+), 13 deletions(-)
> 
> diff --git a/block/curl.c b/block/curl.c
> index 8a45b371cc..05f77a38c2 100644
> --- a/block/curl.c
> +++ b/block/curl.c
> @@ -189,15 +189,15 @@ static int curl_sock_cb(CURL *curl, curl_socket_t fd, int action,
>      switch (action) {
>          case CURL_POLL_IN:
>              aio_set_fd_handler(s->aio_context, fd, false,
> -                               curl_multi_read, NULL, NULL, state);
> +                               curl_multi_read, NULL, NULL, socket);
>              break;
>          case CURL_POLL_OUT:
>              aio_set_fd_handler(s->aio_context, fd, false,
> -                               NULL, curl_multi_do, NULL, state);
> +                               NULL, curl_multi_do, NULL, socket);
>              break;
>          case CURL_POLL_INOUT:
>              aio_set_fd_handler(s->aio_context, fd, false,
> -                               curl_multi_read, curl_multi_do, NULL, state);
> +                               curl_multi_read, curl_multi_do, NULL, socket);
>              break;
>          case CURL_POLL_REMOVE:
>              aio_set_fd_handler(s->aio_context, fd, false,
> @@ -394,9 +394,10 @@ static void curl_multi_check_completion(BDRVCURLState *s)
>  }
>  
>  /* Called with s->mutex held.  */
> -static void curl_multi_do_locked(CURLState *s)
> +static void curl_multi_do_locked(CURLSocket *ready_socket)
>  {
>      CURLSocket *socket, *next_socket;
> +    CURLState *s = socket->state;

Did you mean to use ready_socket here instead?

>      int running;
>      int r;
>  
> @@ -415,21 +416,23 @@ static void curl_multi_do_locked(CURLState *s)
>  
>  static void curl_multi_do(void *arg)
>  {
> -    CURLState *s = (CURLState *)arg;
> +    CURLSocket *socket = arg;
> +    BDRVCURLState *s = socket->state->s;
>  
> -    qemu_mutex_lock(&s->s->mutex);
> -    curl_multi_do_locked(s);
> -    qemu_mutex_unlock(&s->s->mutex);
> +    qemu_mutex_lock(&s->mutex);
> +    curl_multi_do_locked(socket);
> +    qemu_mutex_unlock(&s->mutex);
>  }
>  
>  static void curl_multi_read(void *arg)
>  {
> -    CURLState *s = (CURLState *)arg;
> +    CURLSocket *socket = arg;
> +    BDRVCURLState *s = socket->state->s;
>  
> -    qemu_mutex_lock(&s->s->mutex);
> -    curl_multi_do_locked(s);
> -    curl_multi_check_completion(s->s);
> -    qemu_mutex_unlock(&s->s->mutex);

bye bye &s->s->mutex ! you're very nasty !!

> +    qemu_mutex_lock(&s->mutex);
> +    curl_multi_do_locked(socket);
> +    curl_multi_check_completion(s);
> +    qemu_mutex_unlock(&s->mutex);
>  }
>  
>  static void curl_multi_timeout_do(void *arg)
> 

Re: [Qemu-devel] [Qemu-block] [PATCH 4/6] curl: Report only ready sockets

[John Snow]

On 8/27/19 12:34 PM, Max Reitz wrote:
> Instead of reporting all sockets to cURL, only report the one that has
> caused curl_multi_do_locked() to be called.  This lets us get rid of the
> QLIST_FOREACH_SAFE() list, which was actually wrong: SAFE foreaches are
> only safe when the current element is removed in each iteration.  If it
> possible for the list to be concurrently modified, we cannot guarantee
> that only the current element will be removed.  Therefore, we must not
> use QLIST_FOREACH_SAFE() here.
> 
> Fixes: ff5ca1664af85b24a4180d595ea6873fd3deac57
> Cc: qemu-stable@nongnu.org
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
>  block/curl.c | 17 ++++++-----------
>  1 file changed, 6 insertions(+), 11 deletions(-)
> 
> diff --git a/block/curl.c b/block/curl.c
> index 05f77a38c2..bc70f39fcb 100644
> --- a/block/curl.c
> +++ b/block/curl.c
> @@ -394,24 +394,19 @@ static void curl_multi_check_completion(BDRVCURLState *s)
>  }
>  
>  /* Called with s->mutex held.  */
> -static void curl_multi_do_locked(CURLSocket *ready_socket)
> +static void curl_multi_do_locked(CURLSocket *socket)

Only a momentary hiccup, then.

>  {
> -    CURLSocket *socket, *next_socket;
> -    CURLState *s = socket->state;
> +    BDRVCURLState *s = socket->state->s;
>      int running;
>      int r;
>  
> -    if (!s->s->multi) {
> +    if (!s->multi) {
>          return;
>      }
>  
> -    /* Need to use _SAFE because curl_multi_socket_action() may trigger
> -     * curl_sock_cb() which might modify this list */
> -    QLIST_FOREACH_SAFE(socket, &s->sockets, next, next_socket) {
> -        do {
> -            r = curl_multi_socket_action(s->s->multi, socket->fd, 0, &running);
> -        } while (r == CURLM_CALL_MULTI_PERFORM);
> -    }
> +    do {
> +        r = curl_multi_socket_action(s->multi, socket->fd, 0, &running);
> +    } while (r == CURLM_CALL_MULTI_PERFORM);
>  }
>  
>  static void curl_multi_do(void *arg)
> 

We were just calling this spuriously on whatever sockets before?

Seems like a clear improvement, then.

Re: [Qemu-devel] [Qemu-block] [PATCH 5/6] curl: Handle success in multi_check_completion

[John Snow]

On 8/27/19 12:34 PM, Max Reitz wrote:
> Background: As of cURL 7.59.0, it verifies that several functions are
> not called from within a callback.  Among these functions is
> curl_multi_add_handle().
> 
> curl_read_cb() is a callback from cURL and not a coroutine.  Waking up
> acb->co will lead to entering it then and there, which means the current
> request will settle and the caller (if it runs in the same coroutine)
> may then issue the next request.  In such a case, we will enter
> curl_setup_preadv() effectively from within curl_read_cb().
> 
> Calling curl_multi_add_handle() will then fail and the new request will
> not be processed.
> 
> Fix this by not letting curl_read_cb() wake up acb->co.  Instead, leave
> the whole business of settling the AIOCB objects to
> curl_multi_check_completion() (which is called from our timer callback
> and our FD read handler, so not from any cURL callbacks).
> 
> Reported-by: Natalie Gavrielov <ngavrilo@redhat.com>
> Buglink: https://bugzilla.redhat.com/show_bug.cgi?id=1740193
> Cc: qemu-stable@nongnu.org
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
>  block/curl.c | 69 ++++++++++++++++++++++------------------------------
>  1 file changed, 29 insertions(+), 40 deletions(-)
> 
> diff --git a/block/curl.c b/block/curl.c
> index bc70f39fcb..5e0cca601d 100644
> --- a/block/curl.c
> +++ b/block/curl.c
> @@ -231,7 +231,6 @@ static size_t curl_read_cb(void *ptr, size_t size, size_t nmemb, void *opaque)
>  {
>      CURLState *s = ((CURLState*)opaque);
>      size_t realsize = size * nmemb;
> -    int i;
>  
>      trace_curl_read_cb(realsize);
>  
> @@ -247,32 +246,6 @@ static size_t curl_read_cb(void *ptr, size_t size, size_t nmemb, void *opaque)
>      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)) {

This changes from a conditional,

> -            size_t request_length = acb->bytes;
> -
> -            qemu_iovec_from_buf(acb->qiov, 0, s->orig_buf + acb->start,
> -                                acb->end - acb->start);
> -
> -            if (acb->end - acb->start < request_length) {
> -                size_t offset = acb->end - acb->start;
> -                qemu_iovec_memset(acb->qiov, offset, 0,
> -                                  request_length - offset);
> -            }
> -
> -            acb->ret = 0;
> -            s->acb[i] = NULL;
> -            qemu_mutex_unlock(&s->s->mutex);
> -            aio_co_wake(acb->co);
> -            qemu_mutex_lock(&s->s->mutex);
> -        }
> -    }
> -
>  read_end:
>      /* curl will error out if we do not return this value */
>      return size * nmemb;
> @@ -353,13 +326,14 @@ static void curl_multi_check_completion(BDRVCURLState *s)
>              break;
>  
>          if (msg->msg == CURLMSG_DONE) {
> +            int i;
>              CURLState *state = NULL;
> +            bool error = msg->data.result != CURLE_OK;
> +
>              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) {
> -                int i;
> +            if (error) {
>                  static int errcount = 100;
>  
>                  /* Don't lose the original error message from curl, since
> @@ -371,20 +345,35 @@ static void curl_multi_check_completion(BDRVCURLState *s)
>                          error_report("curl: further errors suppressed");
>                      }
>                  }
> +            }
>  
> -                for (i = 0; i < CURL_NUM_ACB; i++) {
> -                    CURLAIOCB *acb = state->acb[i];
> +            for (i = 0; i < CURL_NUM_ACB; i++) {
> +                CURLAIOCB *acb = state->acb[i];
>  
> -                    if (acb == NULL) {
> -                        continue;
> -                    }
> +                if (acb == NULL) {
> +                    continue;
> +                }
> +
> +                if (!error) {
> +                    /* Assert that we have read all data */
> +                    assert(state->buf_off >= acb->end);

To an assertion. This makes me feel better (What happens if that was
ever false in the old code, we just drop the callback?) but is it
definitely always gonna be true?

Well, you are asserting it is, so I will believe you.

> +
> +                    qemu_iovec_from_buf(acb->qiov, 0,
> +                                        state->orig_buf + acb->start,
> +                                        acb->end - acb->start);
>  
> -                    acb->ret = -EIO;
> -                    state->acb[i] = NULL;
> -                    qemu_mutex_unlock(&s->mutex);
> -                    aio_co_wake(acb->co);
> -                    qemu_mutex_lock(&s->mutex);
> +                    if (acb->end - acb->start < acb->bytes) {
> +                        size_t offset = acb->end - acb->start;
> +                        qemu_iovec_memset(acb->qiov, offset, 0,
> +                                          acb->bytes - offset);
> +                    }
>                  }
> +
> +                acb->ret = error ? -EIO : 0;
> +                state->acb[i] = NULL;
> +                qemu_mutex_unlock(&s->mutex);
> +                aio_co_wake(acb->co);
> +                qemu_mutex_lock(&s->mutex);
>              }
>  
>              curl_clean_state(state);
> 

Only other thing that's not obvious to someone who hasn't programmed
curl before: this action is moving from curl_read_cb to check_completion.

check_completion is only called in curl_multi_read and not
curl_multi_do, but curl_read_cb is arguably called somewhere down the
stack from curl_multi_do_locked.

I assume because this is curl_read_cb that there was no way it was
getting invoked from curl_multi_do, and therefore we're not missing
something on the return trip now.

I think it looks fine but it'd be nice to see you say "Yeah, that's
totally right."

--js

Re: [Qemu-devel] [Qemu-block] [PATCH 6/6] curl: Check curl_multi_add_handle()'s return code

[John Snow]

On 8/27/19 12:34 PM, Max Reitz wrote:
> If we had done that all along, debugging would have been much simpler.
> (Also, I/O errors are better than hangs.)
> 
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
>  block/curl.c | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/block/curl.c b/block/curl.c
> index 5e0cca601d..4a7aff02a6 100644
> --- a/block/curl.c
> +++ b/block/curl.c
> @@ -894,7 +894,13 @@ static void curl_setup_preadv(BlockDriverState *bs, CURLAIOCB *acb)
>      trace_curl_setup_preadv(acb->bytes, start, state->range);
>      curl_easy_setopt(state->curl, CURLOPT_RANGE, state->range);
>  
> -    curl_multi_add_handle(s->multi, state->curl);
> +    if (curl_multi_add_handle(s->multi, state->curl) != CURLM_OK) {
> +        state->acb[0] = NULL;
> +        acb->ret = -EIO;
> +
> +        curl_clean_state(state);
> +        goto out;
> +    }
>  
>      /* Tell curl it needs to kick things off */
>      curl_multi_socket_action(s->multi, CURL_SOCKET_TIMEOUT, 0, &running);
> 

And, well, looks fine. I think there's just a refactoring typo and maybe
explaining some curiosities to me that I am fairly sure are fine, actually.

--js

Re: [Qemu-devel] [Qemu-block] [PATCH 2/6] curl: Keep *socket until the end of curl_sock_cb()

[Max Reitz]

[-- Attachment #1.1: Type: text/plain, Size: 2169 bytes --]

On 09.09.19 22:09, John Snow wrote:
> 
> 
> On 8/27/19 12:34 PM, Max Reitz wrote:
>> This does not really change anything, but it makes the code a bit easier
>> to follow once we use @socket as the opaque pointer for
>> aio_set_fd_handler().
>>
>> (Also, this change stops us from creating new CURLSocket objects when
>> the cURL library just wants to stop listening on an existing socket that
>> we do not recognize.  With a well-behaving cURL, that should never
>> happen anyway.)
>>
>> Cc: qemu-stable@nongnu.org
>> Signed-off-by: Max Reitz <mreitz@redhat.com>
>> ---
>>  block/curl.c | 7 ++++---
>>  1 file changed, 4 insertions(+), 3 deletions(-)
>>
>> diff --git a/block/curl.c b/block/curl.c
>> index 92dc2f630e..8a45b371cc 100644
>> --- a/block/curl.c
>> +++ b/block/curl.c
>> @@ -174,18 +174,16 @@ static int curl_sock_cb(CURL *curl, curl_socket_t fd, int action,
>>          if (socket->fd == fd) {
>>              if (action == CURL_POLL_REMOVE) {
>>                  QLIST_REMOVE(socket, next);
>> -                g_free(socket);
>>              }
>>              break;
>>          }
>>      }
> 
>> -    if (!socket) {
>> +    if (action != CURL_POLL_REMOVE && !socket) {
>>          socket = g_new0(CURLSocket, 1);
>>          socket->fd = fd;
>>          socket->state = state;
>>          QLIST_INSERT_HEAD(&state->sockets, socket, next);
>>      }
>> -    socket = NULL;
>>  
>>      trace_curl_sock_cb(action, (int)fd);
>>      switch (action) {
>> @@ -207,6 +205,9 @@ static int curl_sock_cb(CURL *curl, curl_socket_t fd, int action,
>>              break;
>>      }
>>  
>> +    if (action == CURL_POLL_REMOVE) {
>> +        g_free(socket);
>> +    }
>>      return 0;
>>  }
>>  
>>
> 
> Very naive question: why is CURL_POLL_REMOVE handled so early in the
> function? Why not handle both QLIST_REMOVE and g_free under the
> switch(action) construct entirely?

I don’t know how that’s a naive question, it’s just a different way to
approach this problem.  Sure, we can do that.  It is probably more
intuitive to everyone who hasn’t seen the state before this series.

Max


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [Qemu-devel] [Qemu-block] [PATCH 3/6] curl: Pass CURLSocket to curl_multi_{do, read}()

[Max Reitz]

[-- Attachment #1.1: Type: text/plain, Size: 3426 bytes --]

On 09.09.19 22:10, John Snow wrote:
> 
> 
> On 8/27/19 12:34 PM, Max Reitz wrote:
>> curl_multi_do_locked() currently marks all sockets as ready.  That is
>> not only inefficient, but in fact unsafe (the loop is).  A follow-up
>> patch will change that, but to do so, curl_multi_do_locked() needs to
>> know exactly which socket is ready; and that is accomplished by this
>> patch here.
>>
>> Cc: qemu-stable@nongnu.org
>> Signed-off-by: Max Reitz <mreitz@redhat.com>
>> ---
>>  block/curl.c | 29 ++++++++++++++++-------------
>>  1 file changed, 16 insertions(+), 13 deletions(-)
>>
>> diff --git a/block/curl.c b/block/curl.c
>> index 8a45b371cc..05f77a38c2 100644
>> --- a/block/curl.c
>> +++ b/block/curl.c
>> @@ -189,15 +189,15 @@ static int curl_sock_cb(CURL *curl, curl_socket_t fd, int action,
>>      switch (action) {
>>          case CURL_POLL_IN:
>>              aio_set_fd_handler(s->aio_context, fd, false,
>> -                               curl_multi_read, NULL, NULL, state);
>> +                               curl_multi_read, NULL, NULL, socket);
>>              break;
>>          case CURL_POLL_OUT:
>>              aio_set_fd_handler(s->aio_context, fd, false,
>> -                               NULL, curl_multi_do, NULL, state);
>> +                               NULL, curl_multi_do, NULL, socket);
>>              break;
>>          case CURL_POLL_INOUT:
>>              aio_set_fd_handler(s->aio_context, fd, false,
>> -                               curl_multi_read, curl_multi_do, NULL, state);
>> +                               curl_multi_read, curl_multi_do, NULL, socket);
>>              break;
>>          case CURL_POLL_REMOVE:
>>              aio_set_fd_handler(s->aio_context, fd, false,
>> @@ -394,9 +394,10 @@ static void curl_multi_check_completion(BDRVCURLState *s)
>>  }
>>  
>>  /* Called with s->mutex held.  */
>> -static void curl_multi_do_locked(CURLState *s)
>> +static void curl_multi_do_locked(CURLSocket *ready_socket)
>>  {
>>      CURLSocket *socket, *next_socket;
>> +    CURLState *s = socket->state;
> 
> Did you mean to use ready_socket here instead?

Oops...  Yes, I suppose so.

(An artifact from pulling apart one large patch, sorry.)

Max

>>      int running;
>>      int r;
>>  
>> @@ -415,21 +416,23 @@ static void curl_multi_do_locked(CURLState *s)
>>  
>>  static void curl_multi_do(void *arg)
>>  {
>> -    CURLState *s = (CURLState *)arg;
>> +    CURLSocket *socket = arg;
>> +    BDRVCURLState *s = socket->state->s;
>>  
>> -    qemu_mutex_lock(&s->s->mutex);
>> -    curl_multi_do_locked(s);
>> -    qemu_mutex_unlock(&s->s->mutex);
>> +    qemu_mutex_lock(&s->mutex);
>> +    curl_multi_do_locked(socket);
>> +    qemu_mutex_unlock(&s->mutex);
>>  }
>>  
>>  static void curl_multi_read(void *arg)
>>  {
>> -    CURLState *s = (CURLState *)arg;
>> +    CURLSocket *socket = arg;
>> +    BDRVCURLState *s = socket->state->s;
>>  
>> -    qemu_mutex_lock(&s->s->mutex);
>> -    curl_multi_do_locked(s);
>> -    curl_multi_check_completion(s->s);
>> -    qemu_mutex_unlock(&s->s->mutex);
> 
> bye bye &s->s->mutex ! you're very nasty !!
> 
>> +    qemu_mutex_lock(&s->mutex);
>> +    curl_multi_do_locked(socket);
>> +    curl_multi_check_completion(s);
>> +    qemu_mutex_unlock(&s->mutex);
>>  }
>>  
>>  static void curl_multi_timeout_do(void *arg)
>>



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [Qemu-devel] [Qemu-block] [PATCH 4/6] curl: Report only ready sockets

[Max Reitz]

[-- Attachment #1.1: Type: text/plain, Size: 2363 bytes --]

On 09.09.19 22:16, John Snow wrote:
> 
> 
> On 8/27/19 12:34 PM, Max Reitz wrote:
>> Instead of reporting all sockets to cURL, only report the one that has
>> caused curl_multi_do_locked() to be called.  This lets us get rid of the
>> QLIST_FOREACH_SAFE() list, which was actually wrong: SAFE foreaches are
>> only safe when the current element is removed in each iteration.  If it
>> possible for the list to be concurrently modified, we cannot guarantee
>> that only the current element will be removed.  Therefore, we must not
>> use QLIST_FOREACH_SAFE() here.
>>
>> Fixes: ff5ca1664af85b24a4180d595ea6873fd3deac57
>> Cc: qemu-stable@nongnu.org
>> Signed-off-by: Max Reitz <mreitz@redhat.com>
>> ---
>>  block/curl.c | 17 ++++++-----------
>>  1 file changed, 6 insertions(+), 11 deletions(-)
>>
>> diff --git a/block/curl.c b/block/curl.c
>> index 05f77a38c2..bc70f39fcb 100644
>> --- a/block/curl.c
>> +++ b/block/curl.c
>> @@ -394,24 +394,19 @@ static void curl_multi_check_completion(BDRVCURLState *s)
>>  }
>>  
>>  /* Called with s->mutex held.  */
>> -static void curl_multi_do_locked(CURLSocket *ready_socket)
>> +static void curl_multi_do_locked(CURLSocket *socket)
> 
> Only a momentary hiccup, then.
> 
>>  {
>> -    CURLSocket *socket, *next_socket;
>> -    CURLState *s = socket->state;
>> +    BDRVCURLState *s = socket->state->s;
>>      int running;
>>      int r;
>>  
>> -    if (!s->s->multi) {
>> +    if (!s->multi) {
>>          return;
>>      }
>>  
>> -    /* Need to use _SAFE because curl_multi_socket_action() may trigger
>> -     * curl_sock_cb() which might modify this list */
>> -    QLIST_FOREACH_SAFE(socket, &s->sockets, next, next_socket) {
>> -        do {
>> -            r = curl_multi_socket_action(s->s->multi, socket->fd, 0, &running);
>> -        } while (r == CURLM_CALL_MULTI_PERFORM);
>> -    }
>> +    do {
>> +        r = curl_multi_socket_action(s->multi, socket->fd, 0, &running);
>> +    } while (r == CURLM_CALL_MULTI_PERFORM);
>>  }
>>  
>>  static void curl_multi_do(void *arg)
>>
> 
> We were just calling this spuriously on whatever sockets before?

Yep.  I was to blame; but to my defense, before then we only called it
for a single socket (which doesn’t work that well for FTP).

Max

> Seems like a clear improvement, then.
> 



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [Qemu-devel] [Qemu-block] [PATCH 5/6] curl: Handle success in multi_check_completion

[Max Reitz]

[-- Attachment #1.1: Type: text/plain, Size: 9178 bytes --]

On 09.09.19 22:30, John Snow wrote:
> 
> 
> On 8/27/19 12:34 PM, Max Reitz wrote:
>> Background: As of cURL 7.59.0, it verifies that several functions are
>> not called from within a callback.  Among these functions is
>> curl_multi_add_handle().
>>
>> curl_read_cb() is a callback from cURL and not a coroutine.  Waking up
>> acb->co will lead to entering it then and there, which means the current
>> request will settle and the caller (if it runs in the same coroutine)
>> may then issue the next request.  In such a case, we will enter
>> curl_setup_preadv() effectively from within curl_read_cb().
>>
>> Calling curl_multi_add_handle() will then fail and the new request will
>> not be processed.
>>
>> Fix this by not letting curl_read_cb() wake up acb->co.  Instead, leave
>> the whole business of settling the AIOCB objects to
>> curl_multi_check_completion() (which is called from our timer callback
>> and our FD read handler, so not from any cURL callbacks).
>>
>> Reported-by: Natalie Gavrielov <ngavrilo@redhat.com>
>> Buglink: https://bugzilla.redhat.com/show_bug.cgi?id=1740193
>> Cc: qemu-stable@nongnu.org
>> Signed-off-by: Max Reitz <mreitz@redhat.com>
>> ---
>>  block/curl.c | 69 ++++++++++++++++++++++------------------------------
>>  1 file changed, 29 insertions(+), 40 deletions(-)
>>
>> diff --git a/block/curl.c b/block/curl.c
>> index bc70f39fcb..5e0cca601d 100644
>> --- a/block/curl.c
>> +++ b/block/curl.c
>> @@ -231,7 +231,6 @@ static size_t curl_read_cb(void *ptr, size_t size, size_t nmemb, void *opaque)
>>  {
>>      CURLState *s = ((CURLState*)opaque);
>>      size_t realsize = size * nmemb;
>> -    int i;
>>  
>>      trace_curl_read_cb(realsize);
>>  
>> @@ -247,32 +246,6 @@ static size_t curl_read_cb(void *ptr, size_t size, size_t nmemb, void *opaque)
>>      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)) {
> 
> This changes from a conditional,

It doesn’t really change.  This code here is the read_cb.  We may have
incomplete data still.

>> -            size_t request_length = acb->bytes;
>> -
>> -            qemu_iovec_from_buf(acb->qiov, 0, s->orig_buf + acb->start,
>> -                                acb->end - acb->start);
>> -
>> -            if (acb->end - acb->start < request_length) {
>> -                size_t offset = acb->end - acb->start;
>> -                qemu_iovec_memset(acb->qiov, offset, 0,
>> -                                  request_length - offset);
>> -            }
>> -
>> -            acb->ret = 0;
>> -            s->acb[i] = NULL;
>> -            qemu_mutex_unlock(&s->s->mutex);
>> -            aio_co_wake(acb->co);
>> -            qemu_mutex_lock(&s->s->mutex);
>> -        }
>> -    }
>> -
>>  read_end:
>>      /* curl will error out if we do not return this value */
>>      return size * nmemb;
>> @@ -353,13 +326,14 @@ static void curl_multi_check_completion(BDRVCURLState *s)
>>              break;
>>  
>>          if (msg->msg == CURLMSG_DONE) {
>> +            int i;
>>              CURLState *state = NULL;
>> +            bool error = msg->data.result != CURLE_OK;
>> +
>>              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) {
>> -                int i;
>> +            if (error) {
>>                  static int errcount = 100;
>>  
>>                  /* Don't lose the original error message from curl, since
>> @@ -371,20 +345,35 @@ static void curl_multi_check_completion(BDRVCURLState *s)
>>                          error_report("curl: further errors suppressed");
>>                      }
>>                  }
>> +            }
>>  
>> -                for (i = 0; i < CURL_NUM_ACB; i++) {
>> -                    CURLAIOCB *acb = state->acb[i];
>> +            for (i = 0; i < CURL_NUM_ACB; i++) {
>> +                CURLAIOCB *acb = state->acb[i];
>>  
>> -                    if (acb == NULL) {
>> -                        continue;
>> -                    }
>> +                if (acb == NULL) {
>> +                    continue;
>> +                }
>> +
>> +                if (!error) {
>> +                    /* Assert that we have read all data */
>> +                    assert(state->buf_off >= acb->end);
> 
> To an assertion. This makes me feel better (What happens if that was
> ever false in the old code, we just drop the callback?) but is it
> definitely always gonna be true?

No, in the old code, the read_cb may have been called multiple times per
request.  Of course, it would only really complete the last time, and
then we’d invoke the callback.

But here we just look at all requests that are done (CURLMSG_DONE from
curl_multi_info_read()).  So we can now assert that we’ve actually read
all data.

You could argue that it’s up to the library to behave correctly for this
assertion not to blow up, which may not be to everyone’s liking.

> Well, you are asserting it is, so I will believe you.
> 
>> +
>> +                    qemu_iovec_from_buf(acb->qiov, 0,
>> +                                        state->orig_buf + acb->start,
>> +                                        acb->end - acb->start);
>>  
>> -                    acb->ret = -EIO;
>> -                    state->acb[i] = NULL;
>> -                    qemu_mutex_unlock(&s->mutex);
>> -                    aio_co_wake(acb->co);
>> -                    qemu_mutex_lock(&s->mutex);
>> +                    if (acb->end - acb->start < acb->bytes) {
>> +                        size_t offset = acb->end - acb->start;
>> +                        qemu_iovec_memset(acb->qiov, offset, 0,
>> +                                          acb->bytes - offset);
>> +                    }
>>                  }
>> +
>> +                acb->ret = error ? -EIO : 0;
>> +                state->acb[i] = NULL;
>> +                qemu_mutex_unlock(&s->mutex);
>> +                aio_co_wake(acb->co);
>> +                qemu_mutex_lock(&s->mutex);
>>              }
>>  
>>              curl_clean_state(state);
>>
> 
> Only other thing that's not obvious to someone who hasn't programmed
> curl before: this action is moving from curl_read_cb to check_completion.

What you should ask here is why we ever did handle it in read_cb rather
than here.  That’s simply because the code in read_cb is older than
curl_multi_check_completion().  (f785a5ae36c added the error handling;
including the “ACBs for successful messages get completed in
curl_read_cb” comment.)

> check_completion is only called in curl_multi_read and not
> curl_multi_do, but curl_read_cb is arguably called somewhere down the
> stack from curl_multi_do_locked.
> 
> I assume because this is curl_read_cb that there was no way it was
> getting invoked from curl_multi_do, and therefore we're not missing
> something on the return trip now.

Two things:

First, we do all error handling in check_completion, so we already rely
on it being invoked at some point (or failed requests would stall).

Second, I cannot give you a guarantee.  It isn’t like I have cURL in my
blood.  But from what I gather, it doesn’t do anything in the
background, at least if the user (that is us) handles all the timer and
FD stuff.  So it should indeed only invoke the read_cb from whenever we
tell it that new data is available on some FD (or maybe when some timer
fires).  We do call curl_multi_check_completion() whenever new data is
available for reading (or when a timer fires), so it looks OK to me.
We just don’t call it whenever cURL can write to the socket.  But it
can’t read anything from it then, so it should be OK.


Now that’s an awful lot of “should” and “looks OK”.  We can do one of
two things to improve the situation, I think: curl_multi_socket_action()
actually takes an argument that tells it whether the socket is available
for reading or writing (currently we always pass 0 to it, which means
“figure it out yourself”).  This could ensure that it doesn’t read data
when we think the socket is only available for writing and thus won’t
call check_completion.

But there’s a problem: cURL doesn’t only accept IN and OUT there, but
also ERR.  When do we pass that?  Both in _read and _do?  But if it gets
ERR, it will probably return an error, so we should then call
check_completion.  So should we pass IN | ERR from _read and just OUT
from _do?

It gets a bit icky and I don’t want to add a bug here.

So there’s of course something else we could do which is to simply
invoke check_completion in _do for good measure.  (And then _do and
_read would be the same, so we could even save some LoC.)

What do you think?

Max


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]