| Message ID | 20120306224745.24264.19990.stgit@dhcp-8-167.nay.redhat.com |
|---|---|
| State | New |
| Headers | show |
On Wed, Mar 07, 2012 at 06:47:45AM +0800, Amos Kong wrote: > Introduce tcp_server_start() by moving original code in > tcp_start_incoming_migration(). > > Signed-off-by: Amos Kong <akong@redhat.com> > --- > net.c | 28 ++++++++++++++++++++++++++++ > qemu_socket.h | 2 ++ > 2 files changed, 30 insertions(+), 0 deletions(-) > > diff --git a/net.c b/net.c > index c34474f..e90ff23 100644 > --- a/net.c > +++ b/net.c > @@ -99,6 +99,34 @@ static int get_str_sep(char *buf, int buf_size, const char **pp, int sep) > return 0; > } > > +int tcp_server_start(const char *str, int *fd) > +{ > + int val, ret; > + struct sockaddr_in saddr; > + > + if (parse_host_port(&saddr, str) < 0) { > + error_report("invalid host/port combination: %s", str); > + return -EINVAL; > + } > + > + *fd = qemu_socket(PF_INET, SOCK_STREAM, 0); > + if (fd < 0) { > + perror("socket"); > + return -1; > + } > + socket_set_nonblock(*fd); > + > + /* allow fast reuse */ > + val = 1; > + setsockopt(*fd, SOL_SOCKET, SO_REUSEADDR, (const char *)&val, sizeof(val)); > + > + ret = bind(*fd, (struct sockaddr *)&saddr, sizeof(saddr)); > + if (ret < 0) { > + closesocket(*fd); > + } > + return ret; > +} > + I would combine this with patch 2, since it provides context for why this function is being added. Would also do the same for 3 and 4. I see client the client implementation you need to pass fd back by reference since ret can be set to EINPROGRESS/EWOULDBLOCK on success, but here it looks like fd is only value if ret=0, so might as well just pass back the fd as the function return value. Also, is there any reason we can't re-use qemu-sockets.c:inet_listen()/qemu-sockets.c:inet_connect()? AFAICT they serve the same purpose, and already include some of the work from your PATCH #6. > 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..d612793 100644 > --- a/qemu_socket.h > +++ b/qemu_socket.h > @@ -54,6 +54,8 @@ int unix_listen(const char *path, char *ostr, int olen); > int unix_connect_opts(QemuOpts *opts); > int unix_connect(const char *path); > > +int tcp_server_start(const char *str, int *fd); > + > /* Old, ipv4 only bits. Don't use for new code. */ > int parse_host_port(struct sockaddr_in *saddr, const char *str); > int socket_init(void); > >
On 03/07/2012 12:47 AM, Amos Kong wrote: > Introduce tcp_server_start() by moving original code in > tcp_start_incoming_migration(). > > Signed-off-by: Amos Kong <akong@redhat.com> > --- > net.c | 28 ++++++++++++++++++++++++++++ > qemu_socket.h | 2 ++ > 2 files changed, 30 insertions(+), 0 deletions(-) > > diff --git a/net.c b/net.c > index c34474f..e90ff23 100644 > --- a/net.c > +++ b/net.c > @@ -99,6 +99,34 @@ static int get_str_sep(char *buf, int buf_size, const char **pp, int sep) > return 0; > } > > +int tcp_server_start(const char *str, int *fd) > +{ > + int val, ret; > + struct sockaddr_in saddr; > + > + if (parse_host_port(&saddr, str) < 0) { > + error_report("invalid host/port combination: %s", str); > + return -EINVAL; > + } > + > + *fd = qemu_socket(PF_INET, SOCK_STREAM, 0); > + if (fd < 0) { > + perror("socket"); > + return -1; return -socket_error(); > + } > + socket_set_nonblock(*fd); In incoming migration we don't use non-blocking socket. How about a flag to the function for non-blocking ? > + > + /* allow fast reuse */ > + val = 1; > + setsockopt(*fd, SOL_SOCKET, SO_REUSEADDR, (const char *)&val, sizeof(val)); > + > + ret = bind(*fd, (struct sockaddr *)&saddr, sizeof(saddr)); > + if (ret < 0) { > + closesocket(*fd); > + } > + return ret; if (bind(*fd, (struct sockaddr *)&saddr, sizeof(saddr)) < 0) { closesocket(*fd); return -socket_error(); } return 0; and than you will not need ret > +} > + > 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..d612793 100644 > --- a/qemu_socket.h > +++ b/qemu_socket.h > @@ -54,6 +54,8 @@ int unix_listen(const char *path, char *ostr, int olen); > int unix_connect_opts(QemuOpts *opts); > int unix_connect(const char *path); > > +int tcp_server_start(const char *str, int *fd); > + > /* Old, ipv4 only bits. Don't use for new code. */ > int parse_host_port(struct sockaddr_in *saddr, const char *str); > int socket_init(void); > > Orit
Il 14/03/2012 08:14, Orit Wasserman ha scritto: > if (bind(*fd, (struct sockaddr *)&saddr, sizeof(saddr)) < 0) > { > closesocket(*fd); > return -socket_error(); > } > return 0; > > and than you will not need ret But closesocket could clobber socket_error(), no? Paolo
On 14/03/12 15:27, Paolo Bonzini wrote: > Hi Paolo, > Il 14/03/2012 08:14, Orit Wasserman ha scritto: >> if (bind(*fd, (struct sockaddr *)&saddr, sizeof(saddr))< 0) >> { >> closesocket(*fd); >> return -socket_error(); >> } >> return 0; >> >> and than you will not need ret > > But closesocket could clobber socket_error(), no? Yes, it will effect socket_error() How about this fix ? ret = bind(*fd, (struct sockaddr *)&saddr, sizeof(saddr)); if (ret < 0) { ret = -socket_error(); closesocket(*fd); } return ret; }
Il 14/03/2012 08:51, Amos Kong ha scritto: > > > ret = bind(*fd, (struct sockaddr *)&saddr, sizeof(saddr)); > if (ret < 0) { > ret = -socket_error(); > closesocket(*fd); > } > return ret; > } Looks good. Paolo
On 14/03/12 00:39, Michael Roth wrote: > On Wed, Mar 07, 2012 at 06:47:45AM +0800, Amos Kong wrote: >> Introduce tcp_server_start() by moving original code in >> tcp_start_incoming_migration(). >> >> Signed-off-by: Amos Kong<akong@redhat.com> >> --- >> net.c | 28 ++++++++++++++++++++++++++++ >> qemu_socket.h | 2 ++ >> 2 files changed, 30 insertions(+), 0 deletions(-) >> >> diff --git a/net.c b/net.c >> index c34474f..e90ff23 100644 >> --- a/net.c >> +++ b/net.c >> @@ -99,6 +99,34 @@ static int get_str_sep(char *buf, int buf_size, const char **pp, int sep) >> return 0; >> } >> >> +int tcp_server_start(const char *str, int *fd) >> +{ >> + int val, ret; >> + struct sockaddr_in saddr; >> + >> + if (parse_host_port(&saddr, str)< 0) { >> + error_report("invalid host/port combination: %s", str); >> + return -EINVAL; >> + } >> + >> + *fd = qemu_socket(PF_INET, SOCK_STREAM, 0); >> + if (fd< 0) { >> + perror("socket"); >> + return -1; >> + } >> + socket_set_nonblock(*fd); >> + >> + /* allow fast reuse */ >> + val = 1; >> + setsockopt(*fd, SOL_SOCKET, SO_REUSEADDR, (const char *)&val, sizeof(val)); >> + >> + ret = bind(*fd, (struct sockaddr *)&saddr, sizeof(saddr)); >> + if (ret< 0) { >> + closesocket(*fd); >> + } >> + return ret; >> +} >> + > > I would combine this with patch 2, since it provides context for why > this function is being added. Would also do the same for 3 and 4. > > I see client the client implementation you need to pass fd back by > reference since ret can be set to EINPROGRESS/EWOULDBLOCK on success, ret restores 0 or -socket_error() success: 0, -EINPROGRESS fail : ret < 0 && ret !=-EINTR && ret != -EWOULDBLOCK , it should be -EINPROGRESS + ret = connect(*fd, (struct sockaddr *)&saddr, sizeof(saddr)); + if (ret < 0) { + ret = -socket_error(); + if (ret == -EINPROGRESS) { + break; > but here it looks like fd is only value if ret=0, so might as well just > pass back the fd as the function return value. Passed *fd tells us if socket creation success 'ret' would return 0 or -socket_error() -socket_error() is the error of socket creation/connection, it would be used by other functions. eg: migration-tcp.c: int tcp_start_incoming_migration( ... ret = tcp_server_start(host_port, &s); if (ret < 0) { fprintf(stderr, "tcp_server_start: %s\n", strerror(-ret)); We can also add a error msg in tcp_start_outgoing_migration() when tcp_client_start() fails. > Also, is there any reason we can't re-use > qemu-sockets.c:inet_listen()/qemu-sockets.c:inet_connect()? AFAICT they > serve the same purpose, and already include some of the work from your > PATCH #6. We could not directly use it, there are some difference, such as tcp_start_incoming_migration() doesn't set socket no-blocked, but net_socket_listen_init() sets socket no-blocked. There are some repeated code, so I write a tcp_common_start(), and use it in net_socket_listen_init()/ net_socket_connect_init() and tcp_start_incoming_migration()/ tcp_start_outgoing_migration() >> 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..d612793 100644 >> --- a/qemu_socket.h >> +++ b/qemu_socket.h >> @@ -54,6 +54,8 @@ int unix_listen(const char *path, char *ostr, int olen); >> int unix_connect_opts(QemuOpts *opts); >> int unix_connect(const char *path); >> >> +int tcp_server_start(const char *str, int *fd); >> + >> /* Old, ipv4 only bits. Don't use for new code. */ >> int parse_host_port(struct sockaddr_in *saddr, const char *str); >> int socket_init(void); >> >> >
On 03/14/2012 09:51 AM, Amos Kong wrote: > On 14/03/12 15:27, Paolo Bonzini wrote: >> > > Hi Paolo, > >> Il 14/03/2012 08:14, Orit Wasserman ha scritto: >>> if (bind(*fd, (struct sockaddr *)&saddr, sizeof(saddr))< 0) >>> { >>> closesocket(*fd); >>> return -socket_error(); >>> } >>> return 0; >>> >>> and than you will not need ret >> >> But closesocket could clobber socket_error(), no? Completely correct. > > Yes, it will effect socket_error() > > How about this fix ? > > ret = bind(*fd, (struct sockaddr *)&saddr, sizeof(saddr)); > if (ret < 0) { > ret = -socket_error(); > closesocket(*fd); > } > return ret; > } > Looks good. Orit
Am 14.03.2012 08:51, schrieb Amos Kong: > On 14/03/12 15:27, Paolo Bonzini wrote: >> > > Hi Paolo, > >> Il 14/03/2012 08:14, Orit Wasserman ha scritto: >>> if (bind(*fd, (struct sockaddr *)&saddr, sizeof(saddr))< 0) >>> { >>> closesocket(*fd); >>> return -socket_error(); >>> } >>> return 0; >>> >>> and than you will not need ret >> >> But closesocket could clobber socket_error(), no? > > Yes, it will effect socket_error() > > How about this fix ? > > ret = bind(*fd, (struct sockaddr *)&saddr, sizeof(saddr)); > if (ret < 0) { > ret = -socket_error(); > closesocket(*fd); > } > return ret; > } > But it's still moved (or in this patch copied) code, right? If so, please move it in one patch, and then fix it in another one on top. Kevin
On Wed, Mar 14, 2012 at 04:33:14PM +0800, Amos Kong wrote: > On 14/03/12 00:39, Michael Roth wrote: > >On Wed, Mar 07, 2012 at 06:47:45AM +0800, Amos Kong wrote: > >>Introduce tcp_server_start() by moving original code in > >>tcp_start_incoming_migration(). > >> > >>Signed-off-by: Amos Kong<akong@redhat.com> > >>--- > >> net.c | 28 ++++++++++++++++++++++++++++ > >> qemu_socket.h | 2 ++ > >> 2 files changed, 30 insertions(+), 0 deletions(-) > >> > >>diff --git a/net.c b/net.c > >>index c34474f..e90ff23 100644 > >>--- a/net.c > >>+++ b/net.c > >>@@ -99,6 +99,34 @@ static int get_str_sep(char *buf, int buf_size, const char **pp, int sep) > >> return 0; > >> } > >> > >>+int tcp_server_start(const char *str, int *fd) > >>+{ > >>+ int val, ret; > >>+ struct sockaddr_in saddr; > >>+ > >>+ if (parse_host_port(&saddr, str)< 0) { > >>+ error_report("invalid host/port combination: %s", str); > >>+ return -EINVAL; > >>+ } > >>+ > >>+ *fd = qemu_socket(PF_INET, SOCK_STREAM, 0); > >>+ if (fd< 0) { > >>+ perror("socket"); > >>+ return -1; > >>+ } > >>+ socket_set_nonblock(*fd); > >>+ > >>+ /* allow fast reuse */ > >>+ val = 1; > >>+ setsockopt(*fd, SOL_SOCKET, SO_REUSEADDR, (const char *)&val, sizeof(val)); > >>+ > >>+ ret = bind(*fd, (struct sockaddr *)&saddr, sizeof(saddr)); > >>+ if (ret< 0) { > >>+ closesocket(*fd); > >>+ } > >>+ return ret; > >>+} > >>+ > > > >I would combine this with patch 2, since it provides context for why > >this function is being added. Would also do the same for 3 and 4. > > > >I see client the client implementation you need to pass fd back by > >reference since ret can be set to EINPROGRESS/EWOULDBLOCK on success, > > ret restores 0 or -socket_error() > success: 0, -EINPROGRESS > fail : ret < 0 && ret !=-EINTR && ret != -EWOULDBLOCK > > , it should be -EINPROGRESS I see, I think I was confued by patch #4 where you do a + ret = tcp_client_start(host_port, &s->fd); + if (ret == -EINPROGRESS || ret == -EWOULDBLOCK) { + DPRINTF("connect in progress"); + qemu_set_fd_handler2(s->fd, NULL, NULL, tcp_wait_for_connect, s); If ret == EWOULDBLOCK is a failure (or if the call isn't supposed to return EWOULDBLOCK), we should fail it there rather than passing it on to tcp_wait_for_connect(). > > + ret = connect(*fd, (struct sockaddr *)&saddr, sizeof(saddr)); > + if (ret < 0) { > + ret = -socket_error(); > + if (ret == -EINPROGRESS) { > + break; > > > >but here it looks like fd is only value if ret=0, so might as well just > >pass back the fd as the function return value. > > Passed *fd tells us if socket creation success > 'ret' would return 0 or -socket_error() > > -socket_error() is the error of socket creation/connection, it would > be used by other functions. > > eg: migration-tcp.c: > int tcp_start_incoming_migration( > ... > ret = tcp_server_start(host_port, &s); > if (ret < 0) { > fprintf(stderr, "tcp_server_start: %s\n", strerror(-ret)); > > We can also add a error msg in tcp_start_outgoing_migration() when > tcp_client_start() fails. > > >Also, is there any reason we can't re-use > >qemu-sockets.c:inet_listen()/qemu-sockets.c:inet_connect()? AFAICT they > >serve the same purpose, and already include some of the work from your > >PATCH #6. > > We could not directly use it, there are some difference, > such as tcp_start_incoming_migration() doesn't set socket no-blocked, > but net_socket_listen_init() sets socket no-blocked. I think adding a common function with blocking/non-blocking flag and having inet_listen_opts()/socket_listen_opts() call it with a wrapper would be reasonable. A lot of code is being introduced here to solve problems that are already handled in qemu-sockets.c. inet_listen()/inet_connect() already handles backeted-enclosed ipv6 addrs, getting port numbers when there's more than one colon, getaddrinfo()-based connections, and most importantly it's had ipv6 support from day 1. Not 100% sure it'll work for what you're doing, but qemu-sockets.c was specifically added for this type of use-case and is heavilly used currently (vnc, nbd, Chardev users), so I think we should use it unless there's a good reason not to. > > There are some repeated code, so I write a tcp_common_start(), and > use it in net_socket_listen_init()/ > net_socket_connect_init() and tcp_start_incoming_migration()/ > tcp_start_outgoing_migration() > > > >> 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..d612793 100644 > >>--- a/qemu_socket.h > >>+++ b/qemu_socket.h > >>@@ -54,6 +54,8 @@ int unix_listen(const char *path, char *ostr, int olen); > >> int unix_connect_opts(QemuOpts *opts); > >> int unix_connect(const char *path); > >> > >>+int tcp_server_start(const char *str, int *fd); > >>+ > >> /* Old, ipv4 only bits. Don't use for new code. */ > >> int parse_host_port(struct sockaddr_in *saddr, const char *str); > >> int socket_init(void); > >> > >> > > > > -- > Amos. >
On 14/03/12 22:58, Michael Roth wrote: > On Wed, Mar 14, 2012 at 04:33:14PM +0800, Amos Kong wrote: >> On 14/03/12 00:39, Michael Roth wrote: >>> On Wed, Mar 07, 2012 at 06:47:45AM +0800, Amos Kong wrote: >>>> Introduce tcp_server_start() by moving original code in >>>> tcp_start_incoming_migration(). >>>> >>>> Signed-off-by: Amos Kong<akong@redhat.com> >>>> --- >>>> net.c | 28 ++++++++++++++++++++++++++++ >>>> qemu_socket.h | 2 ++ >>>> 2 files changed, 30 insertions(+), 0 deletions(-) >>>> >>>> +int tcp_server_start(const char *str, int *fd) >>>> +{ >>> I would combine this with patch 2, since it provides context for why >>> this function is being added. Would also do the same for 3 and 4. >>> >>> I see client the client implementation you need to pass fd back by >>> reference since ret can be set to EINPROGRESS/EWOULDBLOCK on success, >> >> ret restores 0 or -socket_error() >> success: 0, -EINPROGRESS >> fail : ret< 0&& ret !=-EINTR&& ret != -EWOULDBLOCK >> >> , it should be -EINPROGRESS > > I see, I think I was confued by patch #4 where you do a > > + ret = tcp_client_start(host_port,&s->fd); > + if (ret == -EINPROGRESS || ret == -EWOULDBLOCK) { > + DPRINTF("connect in progress"); > + qemu_set_fd_handler2(s->fd, NULL, NULL, tcp_wait_for_connect, s); > > If ret == EWOULDBLOCK is a failure (or if the call isn't supposed to > return EWOULDBLOCK), we should fail it there rather than passing it on to > tcp_wait_for_connect(). You are right, it should be : if (ret == -EINPROGRESS) { >>> Also, is there any reason we can't re-use >>> qemu-sockets.c:inet_listen()/qemu-sockets.c:inet_connect()? AFAICT they >>> serve the same purpose, and already include some of the work from your >>> PATCH #6. >> >> We could not directly use it, there are some difference, >> such as tcp_start_incoming_migration() doesn't set socket no-blocked, >> but net_socket_listen_init() sets socket no-blocked. > > I think adding a common function with blocking/non-blocking flag and > having inet_listen_opts()/socket_listen_opts() call it with a wrapper > would be reasonable. > A lot of code is being introduced here to solve problems that are > already handled in qemu-sockets.c. inet_listen()/inet_connect() > already handles backeted-enclosed ipv6 addrs, getting port numbers when > there's more than one colon, getaddrinfo()-based connections, and most > importantly it's had ipv6 support from day 1. > Not 100% sure it'll work for what you're doing, but qemu-sockets.c was > specifically added for this type of use-case and is heavilly used > currently (vnc, nbd, Chardev users), so I think we should use it unless > there's a good reason not to. There are many special request for migration, which is not implemented in inet_listen_opts()/socket_listen_opts(), but many codes can be reused, I would re-write patches. Thanks, Amos
diff --git a/net.c b/net.c index c34474f..e90ff23 100644 --- a/net.c +++ b/net.c @@ -99,6 +99,34 @@ static int get_str_sep(char *buf, int buf_size, const char **pp, int sep) return 0; } +int tcp_server_start(const char *str, int *fd) +{ + int val, ret; + struct sockaddr_in saddr; + + if (parse_host_port(&saddr, str) < 0) { + error_report("invalid host/port combination: %s", str); + return -EINVAL; + } + + *fd = qemu_socket(PF_INET, SOCK_STREAM, 0); + if (fd < 0) { + perror("socket"); + return -1; + } + socket_set_nonblock(*fd); + + /* allow fast reuse */ + val = 1; + setsockopt(*fd, SOL_SOCKET, SO_REUSEADDR, (const char *)&val, sizeof(val)); + + ret = bind(*fd, (struct sockaddr *)&saddr, sizeof(saddr)); + if (ret < 0) { + closesocket(*fd); + } + return ret; +} + 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..d612793 100644 --- a/qemu_socket.h +++ b/qemu_socket.h @@ -54,6 +54,8 @@ int unix_listen(const char *path, char *ostr, int olen); int unix_connect_opts(QemuOpts *opts); int unix_connect(const char *path); +int tcp_server_start(const char *str, int *fd); + /* Old, ipv4 only bits. Don't use for new code. */ int parse_host_port(struct sockaddr_in *saddr, const char *str); int socket_init(void);
Introduce tcp_server_start() by moving original code in tcp_start_incoming_migration(). Signed-off-by: Amos Kong <akong@redhat.com> --- net.c | 28 ++++++++++++++++++++++++++++ qemu_socket.h | 2 ++ 2 files changed, 30 insertions(+), 0 deletions(-)