On Tue, 2019-09-10 at 14:41 +0200, Max Reitz wrote:
> While it is more likely that transfers complete after some file
> descriptor has data ready to read, we probably should not rely on it.
> Better be safe than sorry and call curl_multi_check_completion() in
> curl_multi_do(), too, just like it is done in curl_multi_read().
>
> With this change, curl_multi_do() and curl_multi_read() are actually the
> same, so drop curl_multi_read() and use curl_multi_do() as the sole FD
> handler.
I understand the reasoning, but I still a bit worry that this
could paper over some bug/race in the future.
If curl asks us only to deal with write, that would mean
that it doesn't expect any data to be received.
Do you by a chance have an example, of this patch
affecting the code? Maybe when a unexpected error reply
is received from the server?
I don't really know the CURL library, so I probably missed
something important.
Other than that,
Reviewed-by: Maxim Levitsky <mlevi...@redhat.com>
Best regards,
Maxim Levitsky
>
> Signed-off-by: Max Reitz <mre...@redhat.com>
> ---
> block/curl.c | 14 ++------------
> 1 file changed, 2 insertions(+), 12 deletions(-)
>
> diff --git a/block/curl.c b/block/curl.c
> index 95d7b77dc0..5838afef99 100644
> --- a/block/curl.c
> +++ b/block/curl.c
> @@ -139,7 +139,6 @@ typedef struct BDRVCURLState {
>
> static void curl_clean_state(CURLState *s);
> 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. */
> @@ -186,7 +185,7 @@ static int curl_sock_cb(CURL *curl, curl_socket_t fd, int
> action,
> switch (action) {
> case CURL_POLL_IN:
> aio_set_fd_handler(s->aio_context, fd, false,
> - curl_multi_read, NULL, NULL, state);
> + curl_multi_do, NULL, NULL, state);
> break;
> case CURL_POLL_OUT:
> aio_set_fd_handler(s->aio_context, fd, false,
> @@ -194,7 +193,7 @@ static int curl_sock_cb(CURL *curl, curl_socket_t fd, int
> action,
> break;
> case CURL_POLL_INOUT:
> aio_set_fd_handler(s->aio_context, fd, false,
> - curl_multi_read, curl_multi_do, NULL, state);
> + curl_multi_do, curl_multi_do, NULL, state);
> break;
> case CURL_POLL_REMOVE:
> aio_set_fd_handler(s->aio_context, fd, false,
> @@ -416,15 +415,6 @@ static void curl_multi_do(void *arg)
> {
> CURLState *s = (CURLState *)arg;
>
> - qemu_mutex_lock(&s->s->mutex);
> - curl_multi_do_locked(s);
> - qemu_mutex_unlock(&s->s->mutex);
> -}
> -
> -static void curl_multi_read(void *arg)
> -{
> - CURLState *s = (CURLState *)arg;
> -
> qemu_mutex_lock(&s->s->mutex);
> curl_multi_do_locked(s);
> curl_multi_check_completion(s->s);
