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

[Maxim Levitsky <mlevitsk@redhat.com>]

On Fri, 2019-09-13 at 13:20 +0200, Max Reitz wrote:
> On 10.09.19 18:13, Maxim Levitsky wrote:
> > 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.
> 
> (Sorry, I don’t know why I forgot to reply.)
> 
> I don’t see how anything changes.  aio_co_wake() is still called after
> the qemu_iovec_memset().
Oops, in this specific areas of the patch, I totally missed the diff markers.

So,

Reviewed-by: Maxim Levitsky <mlevitsk@redhat.com>


Best regards,
	Maxim Levitsky