Message ID | 20170510143205.32013-3-pbonzini@redhat.com |
---|---|
State | New |
Headers | show |
On 10.05.2017 16:32, 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(-) Reviewed-by: Max Reitz <mreitz@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>
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:
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(-)