qemu-devel.nongnu.org archive mirror
 help / color / mirror / Atom feed
From: John Snow <jsnow@redhat.com>
To: Max Reitz <mreitz@redhat.com>, qemu-block@nongnu.org
Cc: Kevin Wolf <kwolf@redhat.com>, qemu-devel@nongnu.org
Subject: Re: [Qemu-devel] [Qemu-block] [PATCH 5/6] curl: Handle success in multi_check_completion
Date: Mon, 9 Sep 2019 16:30:00 -0400	[thread overview]
Message-ID: <691099b9-dd24-447e-8e7a-ea949fc75737@redhat.com> (raw)
In-Reply-To: <20190827163439.16686-6-mreitz@redhat.com>



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


  reply	other threads:[~2019-09-09 20:31 UTC|newest]

Thread overview: 17+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-08-27 16:34 [Qemu-devel] [PATCH 0/6] block/curl: Fix hang and potential crash Max Reitz
2019-08-27 16:34 ` [Qemu-devel] [PATCH 1/6] curl: Keep pointer to the CURLState in CURLSocket Max Reitz
2019-09-09 20:05   ` [Qemu-devel] [Qemu-block] " John Snow
2019-08-27 16:34 ` [Qemu-devel] [PATCH 2/6] curl: Keep *socket until the end of curl_sock_cb() Max Reitz
2019-09-09 20:09   ` [Qemu-devel] [Qemu-block] " John Snow
2019-09-10  7:50     ` Max Reitz
2019-08-27 16:34 ` [Qemu-devel] [PATCH 3/6] curl: Pass CURLSocket to curl_multi_{do, read}() Max Reitz
2019-09-09 20:10   ` [Qemu-devel] [Qemu-block] " John Snow
2019-09-10  7:52     ` Max Reitz
2019-08-27 16:34 ` [Qemu-devel] [PATCH 4/6] curl: Report only ready sockets Max Reitz
2019-09-09 20:16   ` [Qemu-devel] [Qemu-block] " John Snow
2019-09-10  7:53     ` Max Reitz
2019-08-27 16:34 ` [Qemu-devel] [PATCH 5/6] curl: Handle success in multi_check_completion Max Reitz
2019-09-09 20:30   ` John Snow [this message]
2019-09-10  8:17     ` [Qemu-devel] [Qemu-block] " Max Reitz
2019-08-27 16:34 ` [Qemu-devel] [PATCH 6/6] curl: Check curl_multi_add_handle()'s return code Max Reitz
2019-09-09 20:32   ` [Qemu-devel] [Qemu-block] " John Snow

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=691099b9-dd24-447e-8e7a-ea949fc75737@redhat.com \
    --to=jsnow@redhat.com \
    --cc=kwolf@redhat.com \
    --cc=mreitz@redhat.com \
    --cc=qemu-block@nongnu.org \
    --cc=qemu-devel@nongnu.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).