|
From: | Amos Kong |
Subject: | Re: [Qemu-devel] [PATCH 1/4] Use getaddrinfo for migration |
Date: | Fri, 02 Mar 2012 11:33:11 +0800 |
User-agent: | Mozilla/5.0 (X11; Linux x86_64; rv:7.0.1) Gecko/20110930 Thunderbird/7.0.1 |
On 24/02/12 17:08, Kevin Wolf wrote:
Am 10.02.2012 07:27, schrieb Amos Kong:This allows us to use ipv4/ipv6 for migration addresses. Once there, it also uses /etc/services names (it came free). Signed-off-by: Juan Quintela<address@hidden> Signed-off-by: Amos Kong<address@hidden> --- migration-tcp.c | 60 ++++++++----------------------- net.c | 108 +++++++++++++++++++++++++++++++++++++++++++++++++++++++ qemu_socket.h | 3 ++ 3 files changed, 127 insertions(+), 44 deletions(-)This patch is making too many changes at once: 1. Move code around 2. Remove duplicated code between client and server 3. Replace parse_host_port() with an open-coded version
4. Modify the open-coded parse_host_port() to use getaddrinfo instead of inet_aton/gethostbyname (Why can't we just change parse_host_port? Are there valid reasons to use the old implementation? Which?)
tcp_common_start() needs to use the address list which is got by getaddrinfo(),
But I agree with verifying host/port by getaddrinfo() in parse_host_port.
This makes it rather hard to review.diff --git a/migration-tcp.c b/migration-tcp.c index 35a5781..644bb8f 100644 --- a/migration-tcp.c +++ b/migration-tcp.c @@ -81,43 +81,27 @@ static void tcp_wait_for_connect(void *opaque) int tcp_start_outgoing_migration(MigrationState *s, const char *host_port) { - struct sockaddr_in addr; int ret; - - ret = parse_host_port(&addr, host_port); - if (ret< 0) { - return ret; - } + int fd; s->get_error = socket_errno; s->write = socket_write; s->close = tcp_close; - s->fd = qemu_socket(PF_INET, SOCK_STREAM, 0); - if (s->fd == -1) { - DPRINTF("Unable to open socket"); - return -socket_error(); - } - - socket_set_nonblock(s->fd); - - do { - ret = connect(s->fd, (struct sockaddr *)&addr, sizeof(addr)); - if (ret == -1) { - ret = -socket_error(); - } - if (ret == -EINPROGRESS || ret == -EWOULDBLOCK) { - qemu_set_fd_handler2(s->fd, NULL, NULL, tcp_wait_for_connect, s); - return 0; - } - } while (ret == -EINTR); - - if (ret< 0) { + ret = tcp_client_start(host_port,&fd); + s->fd = fd; + if (ret == -EINPROGRESS || ret == -EWOULDBLOCK) { + DPRINTF("connect in progress"); + qemu_set_fd_handler2(s->fd, NULL, NULL, tcp_wait_for_connect, s); + } else if (ret< 0) { DPRINTF("connect failed\n"); - migrate_fd_error(s); + if (ret != -EINVAL) { + migrate_fd_error(s); + }This looks odd. It is probably needed to keep the old behaviour where a failed parse_host_port() wouldn't call migrate_fd_error(). Is this really required? Maybe I'm missing something, but the caller can't know which failure case we had and if migrate_fd_error() has been called.
getaddrinfo() is used in tcp_common_start(), -EINVAL will be returned when getaddrinfo() doesn't return 0.
return ret; + } else { + migrate_fd_connect(s); } - migrate_fd_connect(s); return 0; } @@ -157,28 +141,16 @@ out2: int tcp_start_incoming_migration(const char *host_port) { - struct sockaddr_in addr; - int val; + int ret; int s; DPRINTF("Attempting to start an incoming migration\n"); - if (parse_host_port(&addr, host_port)< 0) { - fprintf(stderr, "invalid host/port combination: %s\n", host_port); - return -EINVAL; - } - - s = qemu_socket(PF_INET, SOCK_STREAM, 0); - if (s == -1) { - return -socket_error(); + ret = tcp_server_start(host_port,&s); + if (ret< 0) { + return ret; } - val = 1; - setsockopt(s, SOL_SOCKET, SO_REUSEADDR, (const char *)&val, sizeof(val)); - - if (bind(s, (struct sockaddr *)&addr, sizeof(addr)) == -1) { - goto err; - } if (listen(s, 1) == -1) { goto err; } diff --git a/net.c b/net.c index c34474f..f63014c 100644 --- a/net.c +++ b/net.c @@ -99,6 +99,114 @@ static int get_str_sep(char *buf, int buf_size, const char **pp, int sep) return 0; } +static int tcp_server_bind(int fd, struct addrinfo *rp) +{ + int ret; + int val = 1; + + /* allow fast reuse */ + setsockopt(fd, SOL_SOCKET, SO_REUSEADDR, (const char *)&val, sizeof(val)); + + ret = bind(fd, rp->ai_addr, rp->ai_addrlen); + + if (ret == -1) { + ret = -socket_error(); + } + return ret; + +} + +static int tcp_client_connect(int fd, struct addrinfo *rp) +{ + int ret; + + do { + ret = connect(fd, rp->ai_addr, rp->ai_addrlen); + if (ret == -1) { + ret = -socket_error(); + } + } while (ret == -EINTR); + + return ret; +} + + +static int tcp_start_common(const char *str, int *fd, bool server) +{ + char hostname[512]; + const char *service; + const char *name; + struct addrinfo hints; + struct addrinfo *result, *rp; + int s; + int sfd; + int ret = -EINVAL; + + *fd = -1; + service = str; + if (get_str_sep(hostname, sizeof(hostname),&service, ':')< 0) { + return -EINVAL; + }Separating host name and port at the first colon looks wrong for IPv6.
I fixed this in [PATCH 3/4] net: split hostname and service by last colon
+ if (server&& strlen(hostname) == 0) { + name = NULL; + } else { + name = hostname; + } + + /* Obtain address(es) matching host/port */ + + memset(&hints, 0, sizeof(struct addrinfo)); + hints.ai_family = AF_UNSPEC; /* Allow IPv4 or IPv6 */ + hints.ai_socktype = SOCK_STREAM; /* Datagram socket */ + + if (server) { + hints.ai_flags = AI_PASSIVE; + } + + s = getaddrinfo(name, service,&hints,&result); + if (s != 0) { + fprintf(stderr, "qemu: getaddrinfo: %s\n", gai_strerror(s));error_report?
yes
Kevin
-- Amos.
[Prev in Thread] | Current Thread | [Next in Thread] |