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

[Maxim Levitsky <mlevitsk@redhat.com>]

On Tue, 2019-09-10 at 14:41 +0200, 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 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 fd70f1ebc4..c343c7ed3d 100644
> --- a/block/curl.c
> +++ b/block/curl.c
> @@ -229,7 +229,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);
>  
> @@ -245,32 +244,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;
> @@ -351,13 +324,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
> @@ -369,20 +343,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);
> +                    }
Original code was memsetting the tail of the buffer before waking up the coroutine.
Is this change intended?

aio_co_wake doesn't enter the co-routine if already in coroutine, but
I think that this is an aio fd handler with isn't run in co-routine itself,
so the callback could run with not yet ready data.


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


Best regards,
	Maxim Levitsky