Re: [Qemu-devel] [PATCH 04/25] qemu-sockets: add nonblocking connect for Unix sockets

[Paolo Bonzini <pbonzini@redhat.com>]

Il 17/10/2012 15:33, Markus Armbruster ha scritto:
> Paolo Bonzini <pbonzini@redhat.com> writes:
> 
>> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
>> ---
>>         v1->v2: fixed connect_state memory leaks
>>
>>  qemu-char.c    |  2 +-
>>  qemu-sockets.c | 77 ++++++++++++++++++++++++++++++++++++++++++++++++----------
>>  qemu_socket.h  |  6 ++++-
>>  3 file modificati, 70 inserzioni(+), 15 rimozioni(-)
>>
>> diff --git a/qemu-char.c b/qemu-char.c
>> index 3cc6cb5..8ebd582 100644
>> --- a/qemu-char.c
>> +++ b/qemu-char.c
>> @@ -2450,7 +2450,7 @@ static CharDriverState *qemu_chr_open_socket(QemuOpts *opts)
>>          if (is_listen) {
>>              fd = unix_listen_opts(opts, NULL);
>>          } else {
>> -            fd = unix_connect_opts(opts, NULL);
>> +            fd = unix_connect_opts(opts, NULL, NULL, NULL);
>>          }
>>      } else {
>>          if (is_listen) {
>> diff --git a/qemu-sockets.c b/qemu-sockets.c
>> index f7e67b6..6a49429 100644
>> --- a/qemu-sockets.c
>> +++ b/qemu-sockets.c
>> @@ -252,16 +252,19 @@ static void wait_for_connect(void *opaque)
>>      }
>>  
>>      /* try to connect to the next address on the list */
>> -    while (s->current_addr->ai_next != NULL && s->fd < 0) {
>> -        s->current_addr = s->current_addr->ai_next;
>> -        s->fd = inet_connect_addr(s->current_addr, &in_progress, s);
>> -        /* connect in progress */
>> -        if (in_progress) {
>> -            return;
>> +    if (s->current_addr) {
>> +        while (s->current_addr->ai_next != NULL && s->fd < 0) {
>> +            s->current_addr = s->current_addr->ai_next;
>> +            s->fd = inet_connect_addr(s->current_addr, &in_progress, s);
>> +            /* connect in progress */
>> +            if (in_progress) {
>> +                return;
>> +            }
>>          }
>> +
>> +        freeaddrinfo(s->addr_list);
>>      }
>>  
>> -    freeaddrinfo(s->addr_list);
>>      if (s->callback) {
>>          s->callback(s->fd, s->opaque);
>>      }
>> @@ -701,11 +704,13 @@ err:
>>      return -1;
>>  }
>>  
>> -int unix_connect_opts(QemuOpts *opts, Error **errp)
>> +int unix_connect_opts(QemuOpts *opts, Error **errp,
>> +                      NonBlockingConnectHandler *callback, void *opaque)
>>  {
>>      struct sockaddr_un un;
>>      const char *path = qemu_opt_get(opts, "path");
>> -    int sock;
>> +    ConnectState *connect_state = NULL;
>> +    int sock, rc;
>>  
>>      if (NULL == path) {
>>          fprintf(stderr, "unix connect: no path specified\n");
>> @@ -717,16 +722,44 @@ int unix_connect_opts(QemuOpts *opts, Error **errp)
>>          perror("socket(unix)");
>>          return -1;
>>      }
>> +    if (callback != NULL) {
>> +        connect_state = g_malloc0(sizeof(*connect_state));
>> +        connect_state->callback = callback;
>> +        connect_state->opaque = opaque;
>> +        socket_set_nonblock(sock);
>> +    }
>>  
>>      memset(&un, 0, sizeof(un));
>>      un.sun_family = AF_UNIX;
>>      snprintf(un.sun_path, sizeof(un.sun_path), "%s", path);
>> -    if (connect(sock, (struct sockaddr*) &un, sizeof(un)) < 0) {
>> +
>> +    /* connect to peer */
>> +    do {
>> +        rc = 0;
>> +        if (connect(sock, (struct sockaddr *) &un, sizeof(un)) < 0) {
>> +            rc = -socket_error();
>> +        }
>> +    } while (rc == -EINTR);
> 
> Isn't this a silent EINTR bug fix?
> 
>> +
>> +    if (connect_state != NULL && QEMU_SOCKET_RC_INPROGRESS(rc)) {
>> +        connect_state->fd = sock;
>> +        qemu_set_fd_handler2(sock, NULL, NULL, wait_for_connect,
>> +                             connect_state);
>> +        return sock;
>> +    } else {
>> +        /* non blocking socket immediate success, call callback */
>> +        if (callback != NULL) {
>> +            callback(sock, opaque);
>> +        }
>> +    }
>> +
>> +    if (rc < 0) {
>>          fprintf(stderr, "connect(unix:%s): %s\n", path, strerror(errno));
>>          close(sock);
>> -	return -1;
>> +        sock = -1;
>>      }
>>  
>> +    g_free(connect_state);
>>      return sock;
>>  }
>>  
> 
> Unlike inet_connect_opts(), this one runs callback() even when connect()
> fails.  Are you sure that's what you want?

I think it's harmless, but it's worth harmonizing it.

Paolo