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-stable@nongnu.org
Signed-off-by: Max Reitz <mreitz@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)
 {
-    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)
-- 
2.21.0

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-stable@nongnu.org
> Signed-off-by: Max Reitz <mreitz@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?

Seems like a clear improvement, then.

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-stable@nongnu.org
>> Signed-off-by: Max Reitz <mreitz@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.
>