On 09.09.19 22:16, John Snow wrote: > > > On 8/27/19 12:34 PM, Max Reitz wrote: >> Instead of reporting all sockets to cURL, only report the one that has >> caused curl_multi_do_locked() to be called. This lets us get rid of the >> QLIST_FOREACH_SAFE() list, which was actually wrong: SAFE foreaches are >> only safe when the current element is removed in each iteration. If it >> possible for the list to be concurrently modified, we cannot guarantee >> that only the current element will be removed. Therefore, we must not >> use QLIST_FOREACH_SAFE() here. >> >> Fixes: ff5ca1664af85b24a4180d595ea6873fd3deac57 >> Cc: qemu-sta...@nongnu.org >> Signed-off-by: Max Reitz <mre...@redhat.com> >> --- >> block/curl.c | 17 ++++++----------- >> 1 file changed, 6 insertions(+), 11 deletions(-) >> >> diff --git a/block/curl.c b/block/curl.c >> index 05f77a38c2..bc70f39fcb 100644 >> --- a/block/curl.c >> +++ b/block/curl.c >> @@ -394,24 +394,19 @@ static void curl_multi_check_completion(BDRVCURLState >> *s) >> } >> >> /* Called with s->mutex held. */ >> -static void curl_multi_do_locked(CURLSocket *ready_socket) >> +static void curl_multi_do_locked(CURLSocket *socket) > > Only a momentary hiccup, then. > >> { >> - CURLSocket *socket, *next_socket; >> - CURLState *s = socket->state; >> + BDRVCURLState *s = socket->state->s; >> int running; >> int r; >> >> - if (!s->s->multi) { >> + if (!s->multi) { >> return; >> } >> >> - /* Need to use _SAFE because curl_multi_socket_action() may trigger >> - * curl_sock_cb() which might modify this list */ >> - QLIST_FOREACH_SAFE(socket, &s->sockets, next, next_socket) { >> - do { >> - r = curl_multi_socket_action(s->s->multi, socket->fd, 0, >> &running); >> - } while (r == CURLM_CALL_MULTI_PERFORM); >> - } >> + do { >> + r = curl_multi_socket_action(s->multi, socket->fd, 0, &running); >> + } while (r == CURLM_CALL_MULTI_PERFORM); >> } >> >> static void curl_multi_do(void *arg) >> > > We were just calling this spuriously on whatever sockets before?
Yep. I was to blame; but to my defense, before then we only called it for a single socket (which doesn’t work that well for FTP). Max > Seems like a clear improvement, then. >
signature.asc
Description: OpenPGP digital signature