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

[John Snow <jsnow@redhat.com>]

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.