From mboxrd@z Thu Jan 1 00:00:00 1970 From: Amos Kong Subject: Re: [PATCH 1/4] Use getaddrinfo for migration Date: Fri, 02 Mar 2012 11:33:11 +0800 Message-ID: <4F503F77.4010003@redhat.com> References: <20120210062608.13397.43361.stgit@dhcp-8-167.nay.redhat.com> <20120210062700.13397.6305.stgit@dhcp-8-167.nay.redhat.com> <4F475378.1090809@redhat.com> Mime-Version: 1.0 Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Cc: aliguori@us.ibm.com, kvm@vger.kernel.org, quintela@redhat.com, jasowang@redhat.com, qemu-devel@nongnu.org, laine@redhat.com To: Kevin Wolf Return-path: In-Reply-To: <4F475378.1090809@redhat.com> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: qemu-devel-bounces+gceq-qemu-devel=gmane.org@nongnu.org Sender: qemu-devel-bounces+gceq-qemu-devel=gmane.org@nongnu.org List-Id: kvm.vger.kernel.org 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 >> Signed-off-by: Amos Kong >> --- >> 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.