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;
 }
 
-- 
2.21.0

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?

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