Re: [Qemu-devel] [PATCH 1/4] Use getaddrinfo for migration

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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<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?)

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.
--
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


[Index of Archives]     [KVM ARM]     [KVM ia64]     [KVM ppc]     [Virtualization Tools]     [Spice Development]     [Libvirt]     [Libvirt Users]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite Questions]     [Linux Kernel]     [Linux SCSI]     [XFree86]
  Powered by Linux