| Message ID | 20120210062700.13397.6305.stgit@dhcp-8-167.nay.redhat.com |
|---|---|
| State | Superseded |
| Headers | show |
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@redhat.com> > Signed-off-by: Amos Kong <akong@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?) 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
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@redhat.com> > Signed-off-by: Amos Kong <akong@redhat.com> > --- > migration-tcp.c | 60 ++++++++----------------------- > net.c | 108 +++++++++++++++++++++++++++++++++++++++++++++++++++++++ > qemu_socket.h | 3 ++ > 3 files changed, 127 insertions(+), 44 deletions(-) > @@ -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; > - } Oh, and this case doesn't print an error message any more now. Kevin
On 24/02/12 17:34, 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@redhat.com> >> Signed-off-by: Amos Kong<akong@redhat.com> >> --- >> migration-tcp.c | 60 ++++++++----------------------- >> net.c | 108 +++++++++++++++++++++++++++++++++++++++++++++++++++++++ >> qemu_socket.h | 3 ++ >> 3 files changed, 127 insertions(+), 44 deletions(-) > >> @@ -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; >> - } > > Oh, and this case doesn't print an error message any more now. The check work is done in tcp_start_common() tcp_start_incoming_migration() -> tcp_client_start() -> tcp_start_common()
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@redhat.com> >> Signed-off-by: Amos Kong<akong@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. > 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
Am 02.03.2012 03:50, schrieb Amos Kong: > On 24/02/12 17:34, 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@redhat.com> >>> Signed-off-by: Amos Kong<akong@redhat.com> >>> --- >>> migration-tcp.c | 60 ++++++++----------------------- >>> net.c | 108 +++++++++++++++++++++++++++++++++++++++++++++++++++++++ >>> qemu_socket.h | 3 ++ >>> 3 files changed, 127 insertions(+), 44 deletions(-) >> >>> @@ -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; >>> - } >> >> Oh, and this case doesn't print an error message any more now. > > The check work is done in tcp_start_common() > > tcp_start_incoming_migration() > -> tcp_client_start() > -> tcp_start_common() Yes, but it only return -EINVAL without printing an error message, so the failure case is silent now. Kevin
Not a reply to the patch but a general observation. I noticed that the tcp migration uses gethostname (or getaddrinfo after this patch) from the main thread - is it really the way to go? Note that DNS query which is done may block for a large amount of time. Is it really safe in this context? Should it resolve the name in a separate thread, allowing guest to run while it is doing that? This question is important for me because right now I'm evaluating a network-connected block device driver which should do failover, so it will have to resolve alternative name(s) at runtime (especially since list of available targets is dynamic). From one point, _usually_, the delay there is very small since it is unlikely you'll do migration or failover overseas, so most likely you'll have the answer from DNS handy. But from another point, if the DNS is malfunctioning right at that time (eg, one of the two DNS resolvers is being rebooted), the delay even from local DNS may be noticeable. Thanks, /mjt
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<quintela@redhat.com> >>> Signed-off-by: Amos Kong<akong@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
On Fri, Mar 02, 2012 at 02:25:36PM +0400, Michael Tokarev wrote: > Not a reply to the patch but a general observation. > > I noticed that the tcp migration uses gethostname > (or getaddrinfo after this patch) from the main > thread - is it really the way to go? Note that > DNS query which is done may block for a large amount > of time. Is it really safe in this context? Should > it resolve the name in a separate thread, allowing > guest to run while it is doing that? > > This question is important for me because right now > I'm evaluating a network-connected block device driver > which should do failover, so it will have to resolve > alternative name(s) at runtime (especially since list > of available targets is dynamic). > > From one point, _usually_, the delay there is very > small since it is unlikely you'll do migration or > failover overseas, so most likely you'll have the > answer from DNS handy. But from another point, if > the DNS is malfunctioning right at that time (eg, > one of the two DNS resolvers is being rebooted), > the delay even from local DNS may be noticeable. Yes, I think you are correct - QEMU should take care to ensure that DNS resolution can not block the QEMU event loop thread. There is the GLib extension (getaddrinfo_a) which does async DNS resolution, but for sake of portability it is probably better to use a thread to do it. Regards, Daniel
On 02/03/12 18:41, Daniel P. Berrange wrote: > On Fri, Mar 02, 2012 at 02:25:36PM +0400, Michael Tokarev wrote: >> Not a reply to the patch but a general observation. >> >> I noticed that the tcp migration uses gethostname >> (or getaddrinfo after this patch) from the main >> thread - is it really the way to go? Note that >> DNS query which is done may block for a large amount >> of time. Is it really safe in this context? Should >> it resolve the name in a separate thread, allowing >> guest to run while it is doing that? >> >> This question is important for me because right now >> I'm evaluating a network-connected block device driver >> which should do failover, so it will have to resolve >> alternative name(s) at runtime (especially since list >> of available targets is dynamic). >> >> From one point, _usually_, the delay there is very >> small since it is unlikely you'll do migration or >> failover overseas, so most likely you'll have the >> answer from DNS handy. But from another point, if >> the DNS is malfunctioning right at that time (eg, >> one of the two DNS resolvers is being rebooted), >> the delay even from local DNS may be noticeable. > > Yes, I think you are correct - QEMU should take care to ensure that > DNS resolution can not block the QEMU event loop thread. > > There is the GLib extension (getaddrinfo_a) which does async DNS > resolution, but for sake of portability it is probably better > to use a thread to do it. I've prepared a V2 according to Kevin's comment, https://github.com/kongove/qemu/commits/master But I don't know how to process the getaddrinfo issue, which steps should be done by a thread? anyone can give a hint? thanks. == migrate steps == 0. main_loop, qemu_iohandler_poll 1. get migration command from qemu monitor 2. parse host/port, get an address list by getaddrinfo() 3. connect server 4. check status and return to main_loop (step 0) (VMstate data is transmitted in background) main_loop_wait() ... \- do_migrate() \- tcp_start_outgoing_migration() \- tcp_client_start() \- parse_host_port_info() \- getaddrinfo()
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); + } 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; + } + 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)); + return -EINVAL; + } + + /* getaddrinfo() returns a list of address structures. + Try each address until we successfully bind/connect). + If socket(2) (or bind/connect(2)) fails, we (close the socket + and) try the next address. */ + + for (rp = result; rp != NULL; rp = rp->ai_next) { + sfd = qemu_socket(rp->ai_family, rp->ai_socktype, rp->ai_protocol); + if (sfd == -1) { + ret = -errno; + continue; + } + socket_set_nonblock(sfd); + if (server) { + ret = tcp_server_bind(sfd, rp); + } else { + ret = tcp_client_connect(sfd, rp); + } + if (ret >= 0 || ret == -EINPROGRESS || ret == -EWOULDBLOCK) { + *fd = sfd; + break; /* Success */ + } + close(sfd); + } + + freeaddrinfo(result); + return ret; +} + +int tcp_server_start(const char *str, int *fd) +{ + return tcp_start_common(str, fd, true); +} + +int tcp_client_start(const char *str, int *fd) +{ + return tcp_start_common(str, fd, false); +} + int parse_host_port(struct sockaddr_in *saddr, const char *str) { char buf[512]; diff --git a/qemu_socket.h b/qemu_socket.h index fe4cf6c..6024496 100644 --- a/qemu_socket.h +++ b/qemu_socket.h @@ -58,4 +58,7 @@ int unix_connect(const char *path); int parse_host_port(struct sockaddr_in *saddr, const char *str); int socket_init(void); +int tcp_client_start(const char *str, int *fd); +int tcp_server_start(const char *str, int *fd); + #endif /* QEMU_SOCKET_H */