On 10.09.19 18:11, Maxim Levitsky wrote: > 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.
I can imagine that maybe it wants to send some data first (to close the connection) before it really marks the request as done. > 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? No, I don’t. As John said, this is just to ensure that we always call curl_multi_check_completion() after the read_cb might have been invoked (and once the request is marked as “done”). > I don't really know the CURL library, so I probably missed > something important. I’d wager a guess that nobody really does because otherwise block/curl.c wouldn’t be in the “Odd Fixes” category (with no dedicated maintainer). Max > 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); > >
signature.asc
Description: OpenPGP digital signature