Re: [Qemu-devel] [Qemu-block] [PATCH v2 3/7] curl: Check completion in curl_multi_do()

[Max Reitz]

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 <address@hidden>
> 
> 
> Best regards,
>       Maxim Levitsky
> 
> 
>>
>> Signed-off-by: Max Reitz <address@hidden>
>> ---
>>  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