Re: [Qemu-devel] [PATCH v4 3/4] Fix address handling in inet_nonblocking_connect

[Orit Wasserman <owasserm@redhat.com>]

On 09/24/2012 12:05 PM, Amos Kong wrote:
> On 23/09/12 22:49, Orit Wasserman wrote:
>> getaddrinfo can give us a list of addresses, but we only try to
>> connect to the first one. If that fails we never proceed to
>> the next one.  This is common on desktop setups that often have ipv6
>> configured but not actually working.
>>
>> To fix this make inet_connect_nonblocking retry connection with a different
>> address.
>> callers on inet_nonblocking_connect register a callback function that will
>> be called when connect opertion completes, in case of failure the fd will have
>> a negative value
>>
>> Signed-off-by: Orit Wasserman <owasserm@redhat.com>
>> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
>> ---
>>   migration-tcp.c |   37 +++++--------------
>>   qemu-char.c     |    2 +-
>>   qemu-sockets.c  |  108 ++++++++++++++++++++++++++++++++++++++++++++++---------
>>   qemu_socket.h   |   33 ++++++++++++-----
>>   4 files changed, 125 insertions(+), 55 deletions(-)
> 
> ...
> 
>> -int inet_connect_opts(QemuOpts *opts, bool block, bool *in_progress,
>> -                      Error **errp)
>> +int inet_connect_opts(QemuOpts *opts, Error **errp, ConnectHandler *callback,
>> +                      void *opaque)
>>   {
>>       struct addrinfo *res, *e;
>>       int sock = -1;
>> +    bool in_progress = false;
>> +    ConnectState *connect_state = NULL;
>>
>>       res = inet_parse_connect_opts(opts, errp);
>>       if (!res) {
>>           return -1;
>>       }
>>
>> -    if (in_progress) {
>> -        *in_progress = false;
>> -    }
>> -
>>       for (e = res; e != NULL; e = e->ai_next) {
>> -        sock = inet_connect_addr(e, block, in_progress);
>> -        if (sock >= 0) {
>> +        if (callback != NULL) {
>> +            connect_state = g_malloc0(sizeof(*connect_state));
> 
> Repeatedly malloc insider for loop without releasing.
> 
oops ...
I will move it outside of the loop.
>> +            connect_state->addr_list = res;
>> +            connect_state->current_addr = e;
>> +            connect_state->callback = callback;
>> +            connect_state->opaque = opaque;
>> +        }
>> +        sock = inet_connect_addr(e, &in_progress, connect_state);
>> +        if (in_progress) {
>> +            return sock;
>> +        } else if (sock >= 0) {
>> +            /* non blocking socket immediate success, call callback */
>> +            if (callback != NULL) {
>> +                callback(sock, opaque);
>> +            }
>>               break;
>>           }
> 
> We should free() connect_state here, or moving allocate code before for loop (don't forget to check callback != NULL ;)
> 
right.
> Amos.
> 
>>       }
>>       if (sock < 0) {
>>           error_set(errp, QERR_SOCKET_CONNECT_FAILED);
>>       }
>> +    g_free(connect_state);
>>       freeaddrinfo(res);
>>       return sock;
>>   }
> 
>