Am 02.03.2012 04:33, schrieb Amos Kong: > 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<quint...@redhat.com> >>> Signed-off-by: Amos Kong<ak...@redhat.com> >>> --- >>> 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.
The new parse_host_port() could return an address list. If it's the right thing to use it here, it's probably right to use it in all other places as well. >> 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. This is an implementation detail of tcp_common_start(). What does -EINVAL mean at the tcp_client_start() level? Should it be documented with a comment? Why must migrate_fd_error() not be called when getaddrinfo() failed? >>> 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 Noticed it later. That's why I suggested reordering the patches in my reply to PATCH 0/4. Kevin -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html