Re: [Qemu-devel] [PATCH 2/7] curl: never invoke callbacks with s->mutex held

[Jeff Cody <jcody@redhat.com>]

On Wed, May 10, 2017 at 04:32:00PM +0200, Paolo Bonzini wrote:
> All curl callbacks go through curl_multi_do, and hence are called with
> s->mutex held.  Note that with comments, and make curl_read_cb drop the
> lock before invoking the callback.
> 
> Likewise for curl_find_buf, where the callback can be invoked by the
> caller.
> 
> Cc: qemu-stable@nongnu.org
> Cc: jcody@redhat.com
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  block/curl.c | 12 ++++++++----
>  1 file changed, 8 insertions(+), 4 deletions(-)
> 
> diff --git a/block/curl.c b/block/curl.c
> index 25a301e7b4..9a00fdc28e 100644
> --- a/block/curl.c
> +++ b/block/curl.c
> @@ -147,6 +147,7 @@ static void curl_multi_do(void *arg);
>  static void curl_multi_read(void *arg);
>  
>  #ifdef NEED_CURL_TIMER_CALLBACK
> +/* Called from curl_multi_do_locked, with s->mutex held.  */
>  static int curl_timer_cb(CURLM *multi, long timeout_ms, void *opaque)
>  {
>      BDRVCURLState *s = opaque;
> @@ -163,6 +164,7 @@ static int curl_timer_cb(CURLM *multi, long timeout_ms, void *opaque)
>  }
>  #endif
>  
> +/* Called from curl_multi_do_locked, with s->mutex held.  */
>  static int curl_sock_cb(CURL *curl, curl_socket_t fd, int action,
>                          void *userp, void *sp)
>  {
> @@ -212,6 +214,7 @@ static int curl_sock_cb(CURL *curl, curl_socket_t fd, int action,
>      return 0;
>  }
>  
> +/* Called from curl_multi_do_locked, with s->mutex held.  */
>  static size_t curl_header_cb(void *ptr, size_t size, size_t nmemb, void *opaque)
>  {
>      BDRVCURLState *s = opaque;
> @@ -226,6 +229,7 @@ static size_t curl_header_cb(void *ptr, size_t size, size_t nmemb, void *opaque)
>      return realsize;
>  }
>  
> +/* Called from curl_multi_do_locked, with s->mutex held.  */
>  static size_t curl_read_cb(void *ptr, size_t size, size_t nmemb, void *opaque)
>  {
>      CURLState *s = ((CURLState*)opaque);
> @@ -264,7 +268,9 @@ static size_t curl_read_cb(void *ptr, size_t size, size_t nmemb, void *opaque)
>                                    request_length - offset);
>              }
>  
> +            qemu_mutex_unlock(&s->s->mutex);
>              acb->common.cb(acb->common.opaque, 0);
> +            qemu_mutex_lock(&s->s->mutex);
>              qemu_aio_unref(acb);
>              s->acb[i] = NULL;
>          }
> @@ -305,8 +311,6 @@ static int curl_find_buf(BDRVCURLState *s, size_t start, size_t len,
>              if (clamped_len < len) {
>                  qemu_iovec_memset(acb->qiov, clamped_len, 0, len - clamped_len);
>              }
> -            acb->common.cb(acb->common.opaque, 0);
> -
>              return FIND_RET_OK;
>          }
>  
> @@ -832,8 +836,8 @@ static void curl_readv_bh_cb(void *p)
>      // we can just call the callback and be done.
>      switch (curl_find_buf(s, start, acb->nb_sectors * BDRV_SECTOR_SIZE, acb)) {
>          case FIND_RET_OK:
> -            qemu_aio_unref(acb);
> -            // fall through
> +            ret = 0;
> +            goto out;
>          case FIND_RET_WAIT:
>              goto out;
>          default:
> -- 
> 2.12.2
> 
>

Reviewed-by: Jeff Cody <jcody@redhat.com>