Re: [Qemu-devel] [Qemu-block] [PATCH 2/6] curl: Keep *socket until the end of curl_sock_cb()

[Max Reitz <mreitz@redhat.com>]

[-- Attachment #1.1: Type: text/plain, Size: 2169 bytes --]

On 09.09.19 22:09, John Snow wrote:
> 
> 
> On 8/27/19 12:34 PM, Max Reitz wrote:
>> This does not really change anything, but it makes the code a bit easier
>> to follow once we use @socket as the opaque pointer for
>> aio_set_fd_handler().
>>
>> (Also, this change stops us from creating new CURLSocket objects when
>> the cURL library just wants to stop listening on an existing socket that
>> we do not recognize.  With a well-behaving cURL, that should never
>> happen anyway.)
>>
>> Cc: qemu-stable@nongnu.org
>> Signed-off-by: Max Reitz <mreitz@redhat.com>
>> ---
>>  block/curl.c | 7 ++++---
>>  1 file changed, 4 insertions(+), 3 deletions(-)
>>
>> diff --git a/block/curl.c b/block/curl.c
>> index 92dc2f630e..8a45b371cc 100644
>> --- a/block/curl.c
>> +++ b/block/curl.c
>> @@ -174,18 +174,16 @@ static int curl_sock_cb(CURL *curl, curl_socket_t fd, int action,
>>          if (socket->fd == fd) {
>>              if (action == CURL_POLL_REMOVE) {
>>                  QLIST_REMOVE(socket, next);
>> -                g_free(socket);
>>              }
>>              break;
>>          }
>>      }
> 
>> -    if (!socket) {
>> +    if (action != CURL_POLL_REMOVE && !socket) {
>>          socket = g_new0(CURLSocket, 1);
>>          socket->fd = fd;
>>          socket->state = state;
>>          QLIST_INSERT_HEAD(&state->sockets, socket, next);
>>      }
>> -    socket = NULL;
>>  
>>      trace_curl_sock_cb(action, (int)fd);
>>      switch (action) {
>> @@ -207,6 +205,9 @@ static int curl_sock_cb(CURL *curl, curl_socket_t fd, int action,
>>              break;
>>      }
>>  
>> +    if (action == CURL_POLL_REMOVE) {
>> +        g_free(socket);
>> +    }
>>      return 0;
>>  }
>>  
>>
> 
> Very naive question: why is CURL_POLL_REMOVE handled so early in the
> function? Why not handle both QLIST_REMOVE and g_free under the
> switch(action) construct entirely?

I don’t know how that’s a naive question, it’s just a different way to
approach this problem.  Sure, we can do that.  It is probably more
intuitive to everyone who hasn’t seen the state before this series.

Max


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]