On 09/13/2012 04:22 PM, Markus Armbruster wrote:
> Orit Wasserman <owass...@redhat.com> writes:
>
>> 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 <owass...@redhat.com>
>> Signed-off-by: Michael S. Tsirkin <m...@redhat.com>
>> ---
>> migration-tcp.c | 29 +++-------
>> qemu-sockets.c | 169
>> +++++++++++++++++++++++++++++++++++++++++--------------
>> qemu_socket.h | 9 ++-
>> 3 files changed, 142 insertions(+), 65 deletions(-)
>>
>> diff --git a/migration-tcp.c b/migration-tcp.c
>> index 7f6ad98..cadea36 100644
>> --- a/migration-tcp.c
>> +++ b/migration-tcp.c
>> @@ -53,29 +53,18 @@ static int tcp_close(MigrationState *s)
>> return r;
>> }
>>
>> -static void tcp_wait_for_connect(void *opaque)
>> +static void tcp_wait_for_connect(int fd, void *opaque)
>> {
>> MigrationState *s = opaque;
>> - int val, ret;
>> - socklen_t valsize = sizeof(val);
>>
>> - DPRINTF("connect completed\n");
>> - do {
>> - ret = getsockopt(s->fd, SOL_SOCKET, SO_ERROR, (void *) &val,
>> &valsize);
>> - } while (ret == -1 && (socket_error()) == EINTR);
>> -
>> - if (ret < 0) {
>> + if (fd < 0) {
>> + DPRINTF("migrate connect error\n");
>> + s->fd = -1;
>> migrate_fd_error(s);
>> - return;
>> - }
>> -
>> - qemu_set_fd_handler2(s->fd, NULL, NULL, NULL, NULL);
>> -
>> - if (val == 0)
>> + } else {
>> + DPRINTF("migrate connect success\n");
>> + s->fd = fd;
>> migrate_fd_connect(s);
>> - else {
>> - DPRINTF("error connecting %d\n", val);
>> - migrate_fd_error(s);
>> }
>> }
>>
>> @@ -88,7 +77,8 @@ int tcp_start_outgoing_migration(MigrationState *s, const
>> char *host_port,
>> s->write = socket_write;
>> s->close = tcp_close;
>>
>> - s->fd = inet_nonblocking_connect(host_port, &in_progress, errp);
>> + s->fd = inet_nonblocking_connect(host_port, tcp_wait_for_connect, s,
>> + &in_progress, errp);
>> if (error_is_set(errp)) {
>> migrate_fd_error(s);
>> return -1;
>> @@ -96,7 +86,6 @@ int tcp_start_outgoing_migration(MigrationState *s, const
>> char *host_port,
>>
>> if (in_progress) {
>> DPRINTF("connect in progress\n");
>> - qemu_set_fd_handler2(s->fd, NULL, NULL, tcp_wait_for_connect, s);
>> } else {
>> migrate_fd_connect(s);
>> }
>> diff --git a/qemu-sockets.c b/qemu-sockets.c
>> index 4f5eca8..f5d64c8 100644
>> --- a/qemu-sockets.c
>> +++ b/qemu-sockets.c
>> @@ -24,6 +24,7 @@
>>
>> #include "qemu_socket.h"
>> #include "qemu-common.h" /* for qemu_isdigit */
>> +#include "main-loop.h"
>>
>> #ifndef AI_ADDRCONFIG
>> # define AI_ADDRCONFIG 0
>> @@ -209,41 +210,27 @@ listen:
>> return slisten;
>> }
>>
>> -static struct addrinfo *inet_parse_connect_opts(QemuOpts *opts, Error
>> **errp)
>> -{
>> - struct addrinfo ai, *res;
>> - int rc;
>> - const char *addr;
>> - const char *port;
>> -
>> - memset(&ai,0, sizeof(ai));
>> - ai.ai_flags = AI_CANONNAME | AI_ADDRCONFIG;
>> - ai.ai_family = PF_UNSPEC;
>> - ai.ai_socktype = SOCK_STREAM;
>> -
>> - addr = qemu_opt_get(opts, "host");
>> - port = qemu_opt_get(opts, "port");
>> - if (addr == NULL || port == NULL) {
>> - fprintf(stderr,
>> - "inet_parse_connect_opts: host and/or port not
>> specified\n");
>> - error_set(errp, QERR_SOCKET_CREATE_FAILED);
>> - return NULL;
>> - }
>> -
>> - if (qemu_opt_get_bool(opts, "ipv4", 0))
>> - ai.ai_family = PF_INET;
>> - if (qemu_opt_get_bool(opts, "ipv6", 0))
>> - ai.ai_family = PF_INET6;
>> +#ifdef _WIN32
>> +#define QEMU_SOCKET_RC_INPROGRESS(rc) \
>> + ((rc) == -EINPROGRESS || rc == -EWOULDBLOCK || rc == -WSAEALREADY)
>> +#else
>> +#define QEMU_SOCKET_RC_INPROGRESS(rc) \
>> + ((rc) == -EINPROGRESS)
>> +#endif
>>
>> - /* lookup */
>> - if (0 != (rc = getaddrinfo(addr, port, &ai, &res))) {
>> - fprintf(stderr,"getaddrinfo(%s,%s): %s\n", addr, port,
>> - gai_strerror(rc));
>> - error_set(errp, QERR_SOCKET_CREATE_FAILED);
>> - return NULL;
>> - }
>> - return res;
>> -}
>
> inet_parse_connect_opts() is only moved, not changed, I think. I like
> to keep code motion well away from code changes (separate patch) to
> facilitate review.
>
>> +/* Struct to store connect state for non blocking connect */
>> +typedef struct ConnectState {
>> + int fd;
>> + struct addrinfo *addr_list;
>> + struct addrinfo *current_addr;
>> + ConnectHandler *callback;
>> + void *opaque;
>> + Error *errp;
>> +} ConnectState;
>> +
>> +static ConnectState connect_state = {
>> + .fd = -1,
>> +};
>
> Why does a single connect_state suffice?
>
>>
>> #ifdef _WIN32
>> #define QEMU_SOCKET_RC_INPROGRESS(rc) \
>> @@ -254,12 +241,17 @@ static struct addrinfo
>> *inet_parse_connect_opts(QemuOpts *opts, Error **errp)
>> #endif
>>
>> static int inet_connect_addr(struct addrinfo *addr, bool block,
>> - bool *in_progress, Error **errp)
>> + IOHandler *handler, bool *in_progress,
>> + Error **errp)
>
> Sure you want a handler parameter? It's always wait_for_connect()...
>
>> {
>> char uaddr[INET6_ADDRSTRLEN + 1];
>> char uport[33];
>> int sock, rc;
>>
>> + if (in_progress) {
>> + *in_progress = false;
>> + }
>> +
>> if (getnameinfo((struct sockaddr *)addr->ai_addr, addr->ai_addrlen,
>> uaddr, INET6_ADDRSTRLEN, uport, 32,
>> NI_NUMERICHOST | NI_NUMERICSERV)) {
>> @@ -285,6 +277,8 @@ static int inet_connect_addr(struct addrinfo *addr, bool
>> block,
>> } while (rc == -EINTR);
>>
>> if (!block && QEMU_SOCKET_RC_INPROGRESS(rc)) {
>> + connect_state.fd = sock;
>> + qemu_set_fd_handler2(sock, NULL, NULL, handler, &connect_state);
>> if (in_progress) {
>> *in_progress = true;
>> }
>> @@ -295,6 +289,94 @@ static int inet_connect_addr(struct addrinfo *addr,
>> bool block,
>> return sock;
>> }
>>
>> +static void wait_for_connect(void *opaque)
>> +{
>> + ConnectState *s = opaque;
>> + int val = 0, rc = 0;
>> + socklen_t valsize = sizeof(val);
>> + bool in_progress = false;
>> +
>> + do {
>> + rc = getsockopt(s->fd, SOL_SOCKET, SO_ERROR, (void *) &val,
>> &valsize);
>> + } while (rc == -1 && (socket_error()) == EINTR);
>
> Either parenthesize both operands of && or none. I prefer none.
>
>> +
>> + /* connect succeded */
>> + if (!rc && !val) {
>> + qemu_set_fd_handler2(s->fd, NULL, NULL, NULL, NULL);
>> + freeaddrinfo(s->addr_list);
>> + if (s->callback) {
>> + s->callback(s->fd, s->opaque);
>> + }
>> + return;
>> + }
>> +
>> + if (!rc && val) {
>> + rc = -val;
>> + }
>> +
>> + qemu_set_fd_handler2(s->fd, NULL, NULL, NULL, NULL);
>> + closesocket(s->fd);
>> + if (s->current_addr != NULL && s->current_addr->ai_next != NULL) {
>
> How can s->current_addr == NULL happen?
can't see a scenario , but it never hurt to check.
>
>> + s->current_addr = s->current_addr->ai_next;
>> + s->fd = inet_connect_addr(s->current_addr, false, wait_for_connect,
>> + &in_progress, &s->errp);
>
> inet_connect_addr() either
>
> 1. completes connect (returns valid fd, sets in_progress to false), or
>
> 2. starts connect (returns valid fd, sets in_progress to true), or
>
> 3. fails (returns -1 and sets in_progress to false).
>
>> + if (in_progress) {
>> + return;
>
> Case 2.
>
>> + }
>> + }
>
> To reach this point, we either ran out of addresses (if not entered), or
> connect to the current address completed (case 1), or connect failed
> (case 3).
>
>> +
>> + qemu_set_fd_handler2(s->fd, NULL, NULL, NULL, NULL);
>> +
>> + closesocket(s->fd);
>> + s->fd = rc;
>> + freeaddrinfo(s->addr_list);
>> + /* connect failed */
>> + if (s->callback) {
>> + s->callback(s->fd, s->opaque);
>> + }
>> + return;
>
> Either I'm confused, or this treats completed connect() as failure,
> which is wrong.
>
I think you caught a bug , i will fix it ...
Orit
>> +}
>> +
>> +static struct addrinfo *inet_parse_connect_opts(QemuOpts *opts, Error
>> **errp)
>> +{
>> + struct addrinfo ai, *res;
>> + int rc;
>> + const char *addr;
>> + const char *port;
>> +
>> + memset(&ai, 0, sizeof(ai));
>> + ai.ai_flags = AI_CANONNAME | AI_ADDRCONFIG;
>> + ai.ai_family = PF_UNSPEC;
>> + ai.ai_socktype = SOCK_STREAM;
>> +
>> + addr = qemu_opt_get(opts, "host");
>> + port = qemu_opt_get(opts, "port");
>> + if (addr == NULL || port == NULL) {
>> + fprintf(stderr,
>> + "inet_parse_connect_opts: host and/or port not
>> specified\n");
>> + error_set(errp, QERR_SOCKET_CREATE_FAILED);
>> + return NULL;
>> + }
>> +
>> + if (qemu_opt_get_bool(opts, "ipv4", 0)) {
>> + ai.ai_family = PF_INET;
>> + }
>> + if (qemu_opt_get_bool(opts, "ipv6", 0)) {
>> + ai.ai_family = PF_INET6;
>> + }
>> +
>> + /* lookup */
>> + rc = getaddrinfo(addr, port, &ai, &res);
>> + if (rc != 0) {
>> + fprintf(stderr, "getaddrinfo(%s,%s): %s\n", addr, port,
>> + gai_strerror(rc));
>> + error_set(errp, QERR_SOCKET_CREATE_FAILED);
>> + return NULL;
>> + }
>> + return res;
>> +}
>> +
>> +
>> int inet_connect_opts(QemuOpts *opts, bool *in_progress, Error **errp)
>> {
>> struct addrinfo *res, *e;
>> @@ -306,12 +388,12 @@ int inet_connect_opts(QemuOpts *opts, bool
>> *in_progress, Error **errp)
>> 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, errp);
>> + if (!block) {
>> + connect_state.addr_list = res;
>> + connect_state.current_addr = e;
>> + }
>> + sock = inet_connect_addr(e, block, wait_for_connect, in_progress,
>> errp);
>> if (in_progress && *in_progress) {
>> return sock;
>> } else if (sock >= 0) {
>> @@ -534,9 +616,8 @@ int inet_connect(const char *str, Error **errp)
>> return sock;
>> }
>>
>> -
>> -int inet_nonblocking_connect(const char *str, bool *in_progress,
>> - Error **errp)
>> +int inet_nonblocking_connect(const char *str, ConnectHandler *callback,
>> + void *opaque, bool *in_progress, Error **errp)
>> {
>> QemuOpts *opts;
>> int sock = -1;
>> @@ -544,6 +625,8 @@ int inet_nonblocking_connect(const char *str, bool
>> *in_progress,
>> opts = qemu_opts_create(&dummy_opts, NULL, 0, NULL);
>> if (inet_parse(opts, str) == 0) {
>> qemu_opt_set(opts, "block", "off");
>> + connect_state.callback = callback;
>> + connect_state.opaque = opaque;
>> sock = inet_connect_opts(opts, in_progress, errp);
>> } else {
>> error_set(errp, QERR_SOCKET_CREATE_FAILED);
>> diff --git a/qemu_socket.h b/qemu_socket.h
>> index c47f2b0..c8431eb 100644
>> --- a/qemu_socket.h
>> +++ b/qemu_socket.h
>> @@ -38,14 +38,19 @@ void socket_set_block(int fd);
>> void socket_set_nonblock(int fd);
>> int send_all(int fd, const void *buf, int len1);
>>
>> +/* callback function for nonblocking connect
>> + * vaild fd on success, negative error code on failure
>> + */
>> +typedef void ConnectHandler(int fd, void *opaque);
>> +
>> /* New, ipv6-ready socket helper functions, see qemu-sockets.c */
>> int inet_listen_opts(QemuOpts *opts, int port_offset, Error **errp);
>> int inet_listen(const char *str, char *ostr, int olen,
>> int socktype, int port_offset, Error **errp);
>> int inet_connect_opts(QemuOpts *opts, bool *in_progress, Error **errp);
>> int inet_connect(const char *str, Error **errp);
>> -int inet_nonblocking_connect(const char *str, bool *in_progress,
>> - Error **errp);
>> +int inet_nonblocking_connect(const char *str, ConnectHandler *callback,
>> + void *opaque, bool *in_progress, Error **errp);
>> int inet_dgram_opts(QemuOpts *opts);
>> const char *inet_strfamily(int family);
>
> Good stuff!
>
