Re: [Qemu-devel] [Qemu-block] [PATCH 4/6] curl: Report only ready sockets

[Max Reitz]

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: address@hidden
>> Signed-off-by: Max Reitz <address@hidden>
>> ---
>>  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