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 <quintela@xxxxxxxxxx>
> Signed-off-by: Amos Kong <akong@xxxxxxxxxx>
> ---
> 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?)
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.
> 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.
> + 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?
Kevin
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
