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?
> + 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.
> +}
> +
> +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!
