| Message ID | 20120417145401.22756.32812.stgit@dhcp-8-167.nay.redhat.com |
|---|---|
| State | New |
| Headers | show |
On Tue, Apr 17, 2012 at 10:54:01PM +0800, Amos Kong wrote: > Add a bool argument to inet_connect() to assign if set socket > to block/nonblock, and delete original argument 'socktype' > that is unused. > > Retry to connect when following errors are got: > -EINTR > -EWOULDBLOCK (win32) > Connect's successful for nonblock socket when following > errors are got, user should wait for connecting by select(): > -EINPROGRESS > -WSAEALREADY (win32) How does the user access these? Via errno/socket_error()? I'm not sure how that would work, I don't think we can rely on the value of errno unless a function indicates an error, so for the case where we're successful but have an EINPROGRESS, I don't think we have a way of reliably obtaining that (errno value might be stale) except maybe something like getsockopt(). I think it would be better to just merge this with your patch #3, which handles all this nicely via Error. > > Change nbd, vnc to use new interface. > > Signed-off-by: Amos Kong <akong@redhat.com> > --- > nbd.c | 2 +- > qemu-sockets.c | 58 +++++++++++++++++++++++++++++++++++++++++++------------- > qemu_socket.h | 2 +- > ui/vnc.c | 2 +- > 4 files changed, 48 insertions(+), 16 deletions(-) > > diff --git a/nbd.c b/nbd.c > index 406e555..b4e68a9 100644 > --- a/nbd.c > +++ b/nbd.c > @@ -146,7 +146,7 @@ int tcp_socket_outgoing(const char *address, uint16_t port) > > int tcp_socket_outgoing_spec(const char *address_and_port) > { > - return inet_connect(address_and_port, SOCK_STREAM); > + return inet_connect(address_and_port, true); > } > > int tcp_socket_incoming(const char *address, uint16_t port) > diff --git a/qemu-sockets.c b/qemu-sockets.c > index 6bcb8e3..e886195 100644 > --- a/qemu-sockets.c > +++ b/qemu-sockets.c > @@ -51,6 +51,9 @@ static QemuOptsList dummy_opts = { > },{ > .name = "ipv6", > .type = QEMU_OPT_BOOL, > + },{ > + .name = "block", > + .type = QEMU_OPT_BOOL, > }, > { /* end if list */ } > }, > @@ -201,7 +204,8 @@ int inet_connect_opts(QemuOpts *opts) > const char *port; > char uaddr[INET6_ADDRSTRLEN+1]; > char uport[33]; > - int sock,rc; > + int sock, rc, err; > + bool block; > > memset(&ai,0, sizeof(ai)); > ai.ai_flags = AI_CANONNAME | AI_ADDRCONFIG; > @@ -210,6 +214,7 @@ int inet_connect_opts(QemuOpts *opts) > > addr = qemu_opt_get(opts, "host"); > port = qemu_opt_get(opts, "port"); > + block = qemu_opt_get_bool(opts, "block", 0); > if (addr == NULL || port == NULL) { > fprintf(stderr, "inet_connect: host and/or port not specified\n"); > return -1; > @@ -241,21 +246,44 @@ int inet_connect_opts(QemuOpts *opts) > continue; > } > setsockopt(sock,SOL_SOCKET,SO_REUSEADDR,(void*)&on,sizeof(on)); > - > + if (!block) { > + socket_set_nonblock(sock); > + } > /* connect to peer */ > - if (connect(sock,e->ai_addr,e->ai_addrlen) < 0) { > - if (NULL == e->ai_next) > - fprintf(stderr, "%s: connect(%s,%s,%s,%s): %s\n", __FUNCTION__, > - inet_strfamily(e->ai_family), > - e->ai_canonname, uaddr, uport, strerror(errno)); > - closesocket(sock); > - continue; > + do { > + err = 0; > + if (connect(sock, e->ai_addr, e->ai_addrlen) < 0) { > + err = -socket_error(); > + } > +#ifndef _WIN32 > + } while (err == -EINTR || err == -EWOULDBLOCK); > +#else > + } while (err == -EINTR); > +#endif > + > + if (err >= 0) { > + goto success; > + } else if (!block && err == -EINPROGRESS) { > + goto success; > +#ifdef _WIN32 > + } else if (!block && err == -WSAEALREADY) { > + goto success; > +#endif > } > - freeaddrinfo(res); > - return sock; > + > + if (NULL == e->ai_next) { > + fprintf(stderr, "%s: connect(%s,%s,%s,%s): %s\n", __FUNCTION__, > + inet_strfamily(e->ai_family), > + e->ai_canonname, uaddr, uport, strerror(errno)); > + } > + closesocket(sock); > } > freeaddrinfo(res); > return -1; > + > +success: > + freeaddrinfo(res); > + return sock; > } > > int inet_dgram_opts(QemuOpts *opts) > @@ -449,14 +477,18 @@ int inet_listen(const char *str, char *ostr, int olen, > return sock; > } > > -int inet_connect(const char *str, int socktype) > +int inet_connect(const char *str, bool block) > { > QemuOpts *opts; > int sock = -1; > > opts = qemu_opts_create(&dummy_opts, NULL, 0); > - if (inet_parse(opts, str) == 0) > + if (inet_parse(opts, str) == 0) { > + if (block) { > + qemu_opt_set(opts, "block", "on"); > + } > sock = inet_connect_opts(opts); > + } > qemu_opts_del(opts); > return sock; > } > diff --git a/qemu_socket.h b/qemu_socket.h > index a5d0a84..f73e26d 100644 > --- a/qemu_socket.h > +++ b/qemu_socket.h > @@ -41,7 +41,7 @@ int inet_listen_opts(QemuOpts *opts, int port_offset); > int inet_listen(const char *str, char *ostr, int olen, > int socktype, int port_offset); > int inet_connect_opts(QemuOpts *opts); > -int inet_connect(const char *str, int socktype); > +int inet_connect(const char *str, bool block); > int inet_dgram_opts(QemuOpts *opts); > const char *inet_strfamily(int family); > > diff --git a/ui/vnc.c b/ui/vnc.c > index deb9ecd..4a96153 100644 > --- a/ui/vnc.c > +++ b/ui/vnc.c > @@ -3068,7 +3068,7 @@ int vnc_display_open(DisplayState *ds, const char *display) > if (strncmp(display, "unix:", 5) == 0) > vs->lsock = unix_connect(display+5); > else > - vs->lsock = inet_connect(display, SOCK_STREAM); > + vs->lsock = inet_connect(display, true); > if (-1 == vs->lsock) { > g_free(vs->display); > vs->display = NULL; > >
On 18/04/12 10:10, Michael Roth wrote: > On Tue, Apr 17, 2012 at 10:54:01PM +0800, Amos Kong wrote: >> Add a bool argument to inet_connect() to assign if set socket >> to block/nonblock, and delete original argument 'socktype' >> that is unused. >> >> Retry to connect when following errors are got: >> -EINTR >> -EWOULDBLOCK (win32) >> Connect's successful for nonblock socket when following >> errors are got, user should wait for connecting by select(): >> -EINPROGRESS >> -WSAEALREADY (win32) > > How does the user access these? Via errno/socket_error()? Yes, connect error is got by socket_error() > I'm not sure > how that would work, I don't think we can rely on the value of errno > unless a function indicates an error, so for the case where we're > successful but have an EINPROGRESS, I don't think we have a way of > reliably obtaining that (errno value might be stale) except maybe > something like getsockopt(). > I think it would be better to just merge this with your patch #3, which > handles all this nicely via Error. We use Error to pass meaningful error, but we need to process original socket error, which is got by socket_error(). In qemu, we used socket_error() to get socket error in many place. If you are confused with how that would work, we need to fix socket_error. I don't want to fix this in the migration patchset. What's your opinion? >> Change nbd, vnc to use new interface. >> >> Signed-off-by: Amos Kong<akong@redhat.com> >> --- >> nbd.c | 2 +- >> qemu-sockets.c | 58 +++++++++++++++++++++++++++++++++++++++++++------------- >> qemu_socket.h | 2 +- >> ui/vnc.c | 2 +- >> 4 files changed, 48 insertions(+), 16 deletions(-) >> >> diff --git a/nbd.c b/nbd.c >> index 406e555..b4e68a9 100644 >> --- a/nbd.c >> +++ b/nbd.c >> @@ -146,7 +146,7 @@ int tcp_socket_outgoing(const char *address, uint16_t port) >> >> int tcp_socket_outgoing_spec(const char *address_and_port) >> { >> - return inet_connect(address_and_port, SOCK_STREAM); >> + return inet_connect(address_and_port, true); >> } >> >> int tcp_socket_incoming(const char *address, uint16_t port) >> diff --git a/qemu-sockets.c b/qemu-sockets.c >> index 6bcb8e3..e886195 100644 >> --- a/qemu-sockets.c >> +++ b/qemu-sockets.c >> @@ -51,6 +51,9 @@ static QemuOptsList dummy_opts = { >> },{ >> .name = "ipv6", >> .type = QEMU_OPT_BOOL, >> + },{ >> + .name = "block", >> + .type = QEMU_OPT_BOOL, >> }, >> { /* end if list */ } >> }, >> @@ -201,7 +204,8 @@ int inet_connect_opts(QemuOpts *opts) >> const char *port; >> char uaddr[INET6_ADDRSTRLEN+1]; >> char uport[33]; >> - int sock,rc; >> + int sock, rc, err; >> + bool block; >> >> memset(&ai,0, sizeof(ai)); >> ai.ai_flags = AI_CANONNAME | AI_ADDRCONFIG; >> @@ -210,6 +214,7 @@ int inet_connect_opts(QemuOpts *opts) >> >> addr = qemu_opt_get(opts, "host"); >> port = qemu_opt_get(opts, "port"); >> + block = qemu_opt_get_bool(opts, "block", 0); >> if (addr == NULL || port == NULL) { >> fprintf(stderr, "inet_connect: host and/or port not specified\n"); >> return -1; >> @@ -241,21 +246,44 @@ int inet_connect_opts(QemuOpts *opts) >> continue; >> } >> setsockopt(sock,SOL_SOCKET,SO_REUSEADDR,(void*)&on,sizeof(on)); >> - >> + if (!block) { >> + socket_set_nonblock(sock); >> + } >> /* connect to peer */ >> - if (connect(sock,e->ai_addr,e->ai_addrlen)< 0) { >> - if (NULL == e->ai_next) >> - fprintf(stderr, "%s: connect(%s,%s,%s,%s): %s\n", __FUNCTION__, >> - inet_strfamily(e->ai_family), >> - e->ai_canonname, uaddr, uport, strerror(errno)); >> - closesocket(sock); >> - continue; >> + do { >> + err = 0; >> + if (connect(sock, e->ai_addr, e->ai_addrlen)< 0) { >> + err = -socket_error(); connect error is got here >> + } >> +#ifndef _WIN32 >> + } while (err == -EINTR || err == -EWOULDBLOCK); >> +#else >> + } while (err == -EINTR); >> +#endif >> + >> + if (err>= 0) { >> + goto success; >> + } else if (!block&& err == -EINPROGRESS) { >> + goto success; >> +#ifdef _WIN32 >> + } else if (!block&& err == -WSAEALREADY) { >> + goto success; >> +#endif >> } >> - freeaddrinfo(res); >> - return sock; >> + >> + if (NULL == e->ai_next) { >> + fprintf(stderr, "%s: connect(%s,%s,%s,%s): %s\n", __FUNCTION__, >> + inet_strfamily(e->ai_family), >> + e->ai_canonname, uaddr, uport, strerror(errno)); >> + } >> + closesocket(sock); >> } >> freeaddrinfo(res); >> return -1; >> + >> +success: >> + freeaddrinfo(res); >> + return sock; >> } >> >> int inet_dgram_opts(QemuOpts *opts) >> @@ -449,14 +477,18 @@ int inet_listen(const char *str, char *ostr, int olen, >> return sock; >> } >> >> -int inet_connect(const char *str, int socktype) >> +int inet_connect(const char *str, bool block) >> { >> QemuOpts *opts; >> int sock = -1; >> >> opts = qemu_opts_create(&dummy_opts, NULL, 0); >> - if (inet_parse(opts, str) == 0) >> + if (inet_parse(opts, str) == 0) { >> + if (block) { >> + qemu_opt_set(opts, "block", "on"); >> + } >> sock = inet_connect_opts(opts); >> + } >> qemu_opts_del(opts); >> return sock; >> } >> diff --git a/qemu_socket.h b/qemu_socket.h >> index a5d0a84..f73e26d 100644 >> --- a/qemu_socket.h >> +++ b/qemu_socket.h >> @@ -41,7 +41,7 @@ int inet_listen_opts(QemuOpts *opts, int port_offset); >> int inet_listen(const char *str, char *ostr, int olen, >> int socktype, int port_offset); >> int inet_connect_opts(QemuOpts *opts); >> -int inet_connect(const char *str, int socktype); >> +int inet_connect(const char *str, bool block); >> int inet_dgram_opts(QemuOpts *opts); >> const char *inet_strfamily(int family); >> >> diff --git a/ui/vnc.c b/ui/vnc.c >> index deb9ecd..4a96153 100644 >> --- a/ui/vnc.c >> +++ b/ui/vnc.c >> @@ -3068,7 +3068,7 @@ int vnc_display_open(DisplayState *ds, const char *display) >> if (strncmp(display, "unix:", 5) == 0) >> vs->lsock = unix_connect(display+5); >> else >> - vs->lsock = inet_connect(display, SOCK_STREAM); >> + vs->lsock = inet_connect(display, true); >> if (-1 == vs->lsock) { >> g_free(vs->display); >> vs->display = NULL; >> >> >
On 04/17/2012 05:54 PM, Amos Kong wrote: > Add a bool argument to inet_connect() to assign if set socket > to block/nonblock, and delete original argument 'socktype' > that is unused. > > Retry to connect when following errors are got: > -EINTR > -EWOULDBLOCK (win32) > Connect's successful for nonblock socket when following > errors are got, user should wait for connecting by select(): > -EINPROGRESS > -WSAEALREADY (win32) > > Change nbd, vnc to use new interface. > > Signed-off-by: Amos Kong <akong@redhat.com> > --- > nbd.c | 2 +- > qemu-sockets.c | 58 +++++++++++++++++++++++++++++++++++++++++++------------- > qemu_socket.h | 2 +- > ui/vnc.c | 2 +- > 4 files changed, 48 insertions(+), 16 deletions(-) > > diff --git a/nbd.c b/nbd.c > index 406e555..b4e68a9 100644 > --- a/nbd.c > +++ b/nbd.c > @@ -146,7 +146,7 @@ int tcp_socket_outgoing(const char *address, uint16_t port) > > int tcp_socket_outgoing_spec(const char *address_and_port) > { > - return inet_connect(address_and_port, SOCK_STREAM); > + return inet_connect(address_and_port, true); > } > > int tcp_socket_incoming(const char *address, uint16_t port) > diff --git a/qemu-sockets.c b/qemu-sockets.c > index 6bcb8e3..e886195 100644 > --- a/qemu-sockets.c > +++ b/qemu-sockets.c > @@ -51,6 +51,9 @@ static QemuOptsList dummy_opts = { > },{ > .name = "ipv6", > .type = QEMU_OPT_BOOL, > + },{ > + .name = "block", > + .type = QEMU_OPT_BOOL, > }, > { /* end if list */ } > }, > @@ -201,7 +204,8 @@ int inet_connect_opts(QemuOpts *opts) > const char *port; > char uaddr[INET6_ADDRSTRLEN+1]; > char uport[33]; > - int sock,rc; > + int sock, rc, err; > + bool block; > > memset(&ai,0, sizeof(ai)); > ai.ai_flags = AI_CANONNAME | AI_ADDRCONFIG; > @@ -210,6 +214,7 @@ int inet_connect_opts(QemuOpts *opts) > > addr = qemu_opt_get(opts, "host"); > port = qemu_opt_get(opts, "port"); > + block = qemu_opt_get_bool(opts, "block", 0); > if (addr == NULL || port == NULL) { > fprintf(stderr, "inet_connect: host and/or port not specified\n"); > return -1; > @@ -241,21 +246,44 @@ int inet_connect_opts(QemuOpts *opts) > continue; > } > setsockopt(sock,SOL_SOCKET,SO_REUSEADDR,(void*)&on,sizeof(on)); > - > + if (!block) { > + socket_set_nonblock(sock); > + } > /* connect to peer */ > - if (connect(sock,e->ai_addr,e->ai_addrlen) < 0) { > - if (NULL == e->ai_next) > - fprintf(stderr, "%s: connect(%s,%s,%s,%s): %s\n", __FUNCTION__, > - inet_strfamily(e->ai_family), > - e->ai_canonname, uaddr, uport, strerror(errno)); > - closesocket(sock); > - continue; > + do { > + err = 0; > + if (connect(sock, e->ai_addr, e->ai_addrlen) < 0) { > + err = -socket_error(); > + } > +#ifndef _WIN32 > + } while (err == -EINTR || err == -EWOULDBLOCK); > +#else > + } while (err == -EINTR); > +#endif We shouldn't retry to connect for a blocking socket, please add a check for !block. According to msn docs in WIN32 if we get EWOULDBLOCK , we should do select http://msdn.microsoft.com/en-us/library/windows/desktop/ms737625(v=vs.85).aspx so I think we only need to retry for -EINTR. > + > + if (err >= 0) { > + goto success; > + } else if (!block && err == -EINPROGRESS) { > + goto success; > +#ifdef _WIN32 > + } else if (!block && err == -WSAEALREADY) { Also EWOULDBLOCK This is more a style comment as I feel to code doesn't need the go to. Check for an error path so the rest of the function looks like: if (err < 0) { if ( block || #ifndef __WIN32 err != -EINPROGRESS ) { #else (err != -EWOULDBLOCK && err != -WASALREADY) ) { #endif if (NULL == e->ai_next) { fprintf(stderr, "%s: connect(%s,%s,%s,%s): %s\n", __FUNCTION__, inet_strfamily(e->ai_family), e->ai_canonname, uaddr, uport, strerror(errno)); } closesocket(sock); sock = -1; } freeaddrinfo(res); return sock; } > + goto success; > +#endif > } > - freeaddrinfo(res); > - return sock; > + > + if (NULL == e->ai_next) { > + fprintf(stderr, "%s: connect(%s,%s,%s,%s): %s\n", __FUNCTION__, > + inet_strfamily(e->ai_family), > + e->ai_canonname, uaddr, uport, strerror(errno)); > + } > + closesocket(sock); > } > freeaddrinfo(res); > return -1; > + > +success: > + freeaddrinfo(res); > + return sock; > } > > int inet_dgram_opts(QemuOpts *opts) > @@ -449,14 +477,18 @@ int inet_listen(const char *str, char *ostr, int olen, > return sock; > } > > -int inet_connect(const char *str, int socktype) > +int inet_connect(const char *str, bool block) > { > QemuOpts *opts; > int sock = -1; > > opts = qemu_opts_create(&dummy_opts, NULL, 0); > - if (inet_parse(opts, str) == 0) > + if (inet_parse(opts, str) == 0) { > + if (block) { > + qemu_opt_set(opts, "block", "on"); > + } > sock = inet_connect_opts(opts); > + } > qemu_opts_del(opts); > return sock; > } > diff --git a/qemu_socket.h b/qemu_socket.h > index a5d0a84..f73e26d 100644 > --- a/qemu_socket.h > +++ b/qemu_socket.h > @@ -41,7 +41,7 @@ int inet_listen_opts(QemuOpts *opts, int port_offset); > int inet_listen(const char *str, char *ostr, int olen, > int socktype, int port_offset); > int inet_connect_opts(QemuOpts *opts); > -int inet_connect(const char *str, int socktype); > +int inet_connect(const char *str, bool block); > int inet_dgram_opts(QemuOpts *opts); > const char *inet_strfamily(int family); > > diff --git a/ui/vnc.c b/ui/vnc.c > index deb9ecd..4a96153 100644 > --- a/ui/vnc.c > +++ b/ui/vnc.c > @@ -3068,7 +3068,7 @@ int vnc_display_open(DisplayState *ds, const char *display) > if (strncmp(display, "unix:", 5) == 0) > vs->lsock = unix_connect(display+5); > else > - vs->lsock = inet_connect(display, SOCK_STREAM); > + vs->lsock = inet_connect(display, true); > if (-1 == vs->lsock) { > g_free(vs->display); > vs->display = NULL; >
On 18/04/12 14:52, Orit Wasserman wrote: > On 04/17/2012 05:54 PM, Amos Kong wrote: >> Add a bool argument to inet_connect() to assign if set socket >> to block/nonblock, and delete original argument 'socktype' >> that is unused. >> >> Retry to connect when following errors are got: >> -EINTR >> -EWOULDBLOCK (win32) >> Connect's successful for nonblock socket when following >> errors are got, user should wait for connecting by select(): >> -EINPROGRESS >> -WSAEALREADY (win32) >> >> Change nbd, vnc to use new interface. >> >> Signed-off-by: Amos Kong<akong@redhat.com> >> --- >> nbd.c | 2 +- >> qemu-sockets.c | 58 +++++++++++++++++++++++++++++++++++++++++++------------- >> qemu_socket.h | 2 +- >> ui/vnc.c | 2 +- >> 4 files changed, 48 insertions(+), 16 deletions(-) >> >> diff --git a/nbd.c b/nbd.c >> index 406e555..b4e68a9 100644 >> --- a/nbd.c >> +++ b/nbd.c >> @@ -146,7 +146,7 @@ int tcp_socket_outgoing(const char *address, uint16_t port) >> >> int tcp_socket_outgoing_spec(const char *address_and_port) >> { >> - return inet_connect(address_and_port, SOCK_STREAM); >> + return inet_connect(address_and_port, true); >> } >> >> int tcp_socket_incoming(const char *address, uint16_t port) >> diff --git a/qemu-sockets.c b/qemu-sockets.c >> index 6bcb8e3..e886195 100644 >> --- a/qemu-sockets.c >> +++ b/qemu-sockets.c >> @@ -51,6 +51,9 @@ static QemuOptsList dummy_opts = { >> },{ >> .name = "ipv6", >> .type = QEMU_OPT_BOOL, >> + },{ >> + .name = "block", >> + .type = QEMU_OPT_BOOL, >> }, >> { /* end if list */ } >> }, >> @@ -201,7 +204,8 @@ int inet_connect_opts(QemuOpts *opts) >> const char *port; >> char uaddr[INET6_ADDRSTRLEN+1]; >> char uport[33]; >> - int sock,rc; >> + int sock, rc, err; >> + bool block; >> >> memset(&ai,0, sizeof(ai)); >> ai.ai_flags = AI_CANONNAME | AI_ADDRCONFIG; >> @@ -210,6 +214,7 @@ int inet_connect_opts(QemuOpts *opts) >> >> addr = qemu_opt_get(opts, "host"); >> port = qemu_opt_get(opts, "port"); >> + block = qemu_opt_get_bool(opts, "block", 0); >> if (addr == NULL || port == NULL) { >> fprintf(stderr, "inet_connect: host and/or port not specified\n"); >> return -1; >> @@ -241,21 +246,44 @@ int inet_connect_opts(QemuOpts *opts) >> continue; >> } >> setsockopt(sock,SOL_SOCKET,SO_REUSEADDR,(void*)&on,sizeof(on)); >> - >> + if (!block) { >> + socket_set_nonblock(sock); >> + } >> /* connect to peer */ >> - if (connect(sock,e->ai_addr,e->ai_addrlen)< 0) { >> - if (NULL == e->ai_next) >> - fprintf(stderr, "%s: connect(%s,%s,%s,%s): %s\n", __FUNCTION__, >> - inet_strfamily(e->ai_family), >> - e->ai_canonname, uaddr, uport, strerror(errno)); >> - closesocket(sock); >> - continue; >> + do { >> + err = 0; >> + if (connect(sock, e->ai_addr, e->ai_addrlen)< 0) { >> + err = -socket_error(); >> + } >> +#ifndef _WIN32 >> + } while (err == -EINTR || err == -EWOULDBLOCK); >> +#else >> + } while (err == -EINTR); >> +#endif > > We shouldn't retry to connect for a blocking socket, please add a check for !block. > According to msn docs in WIN32 if we get EWOULDBLOCK , we should do select > http://msdn.microsoft.com/en-us/library/windows/desktop/ms737625(v=vs.85).aspx > so I think we only need to retry for -EINTR. Hi Orit, thanks for your review. In http://msdn.microsoft.com/en-us/library/windows/desktop/ms740668%28v=vs.85%29.aspx, for 'EWOULDBLOCK': the operation should be retried later." However, the two methods all works. I would not re-try for EWOULDBLOCK (posix & win32). >> + >> + if (err>= 0) { >> + goto success; >> + } else if (!block&& err == -EINPROGRESS) { >> + goto success; >> +#ifdef _WIN32 >> + } else if (!block&& err == -WSAEALREADY) { > > Also EWOULDBLOCK > This is more a style comment as I feel to code doesn't need the go to. Ok. > Check for an error path so the rest of the function looks like: > > if (err< 0) { > if ( block || > #ifndef __WIN32 > err != -EINPROGRESS ) { > #else > (err != -EWOULDBLOCK&& err != -WASALREADY) ) { > #endif EINPROGRESS and EWOULDBLOCK are for posix and win32, if (err < 0) { if (block || (err != -EINPROGRESS && err != -EWOULDBLOCK #ifndef __WIN32 )) { #else && err != -WASALREADY)) { #endif > if (NULL == e->ai_next) { > fprintf(stderr, "%s: connect(%s,%s,%s,%s): %s\n", __FUNCTION__, > inet_strfamily(e->ai_family), > e->ai_canonname, uaddr, uport, strerror(errno)); > } > closesocket(sock); > sock = -1; > } > > freeaddrinfo(res); > return sock; > } ...
On Wed, Apr 18, 2012 at 10:49:02AM +0800, Amos Kong wrote: > On 18/04/12 10:10, Michael Roth wrote: > >On Tue, Apr 17, 2012 at 10:54:01PM +0800, Amos Kong wrote: > >>Add a bool argument to inet_connect() to assign if set socket > >>to block/nonblock, and delete original argument 'socktype' > >>that is unused. > >> > >>Retry to connect when following errors are got: > >> -EINTR > >> -EWOULDBLOCK (win32) > >>Connect's successful for nonblock socket when following > >>errors are got, user should wait for connecting by select(): > >> -EINPROGRESS > >> -WSAEALREADY (win32) > > > >How does the user access these? Via errno/socket_error()? > > Yes, connect error is got by socket_error() > > > I'm not sure > >how that would work, I don't think we can rely on the value of errno > >unless a function indicates an error, so for the case where we're > >successful but have an EINPROGRESS, I don't think we have a way of > >reliably obtaining that (errno value might be stale) except maybe > >something like getsockopt(). > > >I think it would be better to just merge this with your patch #3, which > >handles all this nicely via Error. > > We use Error to pass meaningful error, but we need to process > original socket error, > which is got by socket_error(). > > In qemu, we used socket_error() to get socket error in many place. > If you are confused > with how that would work, we need to fix socket_error. I don't want > to fix this in the > migration patchset. I don't really think it needs fixing any more than errno needs fixing, but it should be used only in situations where it makes sense to use errno, namely: 1) You have a reliable indicator of when errno has been set, such that you can check it's value and be assured that it's not a stale value from some unrelated error that occurred in the past. 2) You don't do anything that can clobber the errno value before the user has a chance to check it. inet_connect() violates 1) because we rely on the caller to check errno/socket_error() for EINPROGRESS even though we pass back a valid fd that has no indication of an error. It may be that we already successfully connected, and that some unrelated path set EINPROGRESS, which may cause us to call select()/getsockopt() unecessary. it also violates 2), because we call close() in the error path, which can clobber errno/socket_error(). > > What's your opinion? I think you already have the solution in your patches #2/#3: rely on Error **errp to pass back these errors. So my suggestion is to just take this patch and merge it with those. Alternatively, if you'd like to keep the introduction of non-blocking support in a seperate patch, do it *after* you introduce Error. So basically move this after your patch #4 so you never have to rely on socket_error() in your series. > > >>Change nbd, vnc to use new interface. > >> > >>Signed-off-by: Amos Kong<akong@redhat.com> > >>--- > >> nbd.c | 2 +- > >> qemu-sockets.c | 58 +++++++++++++++++++++++++++++++++++++++++++------------- > >> qemu_socket.h | 2 +- > >> ui/vnc.c | 2 +- > >> 4 files changed, 48 insertions(+), 16 deletions(-) > >> > >>diff --git a/nbd.c b/nbd.c > >>index 406e555..b4e68a9 100644 > >>--- a/nbd.c > >>+++ b/nbd.c > >>@@ -146,7 +146,7 @@ int tcp_socket_outgoing(const char *address, uint16_t port) > >> > >> int tcp_socket_outgoing_spec(const char *address_and_port) > >> { > >>- return inet_connect(address_and_port, SOCK_STREAM); > >>+ return inet_connect(address_and_port, true); > >> } > >> > >> int tcp_socket_incoming(const char *address, uint16_t port) > >>diff --git a/qemu-sockets.c b/qemu-sockets.c > >>index 6bcb8e3..e886195 100644 > >>--- a/qemu-sockets.c > >>+++ b/qemu-sockets.c > >>@@ -51,6 +51,9 @@ static QemuOptsList dummy_opts = { > >> },{ > >> .name = "ipv6", > >> .type = QEMU_OPT_BOOL, > >>+ },{ > >>+ .name = "block", > >>+ .type = QEMU_OPT_BOOL, > >> }, > >> { /* end if list */ } > >> }, > >>@@ -201,7 +204,8 @@ int inet_connect_opts(QemuOpts *opts) > >> const char *port; > >> char uaddr[INET6_ADDRSTRLEN+1]; > >> char uport[33]; > >>- int sock,rc; > >>+ int sock, rc, err; > >>+ bool block; > >> > >> memset(&ai,0, sizeof(ai)); > >> ai.ai_flags = AI_CANONNAME | AI_ADDRCONFIG; > >>@@ -210,6 +214,7 @@ int inet_connect_opts(QemuOpts *opts) > >> > >> addr = qemu_opt_get(opts, "host"); > >> port = qemu_opt_get(opts, "port"); > >>+ block = qemu_opt_get_bool(opts, "block", 0); > >> if (addr == NULL || port == NULL) { > >> fprintf(stderr, "inet_connect: host and/or port not specified\n"); > >> return -1; > >>@@ -241,21 +246,44 @@ int inet_connect_opts(QemuOpts *opts) > >> continue; > >> } > >> setsockopt(sock,SOL_SOCKET,SO_REUSEADDR,(void*)&on,sizeof(on)); > >>- > >>+ if (!block) { > >>+ socket_set_nonblock(sock); > >>+ } > >> /* connect to peer */ > >>- if (connect(sock,e->ai_addr,e->ai_addrlen)< 0) { > >>- if (NULL == e->ai_next) > >>- fprintf(stderr, "%s: connect(%s,%s,%s,%s): %s\n", __FUNCTION__, > >>- inet_strfamily(e->ai_family), > >>- e->ai_canonname, uaddr, uport, strerror(errno)); > >>- closesocket(sock); > >>- continue; > >>+ do { > >>+ err = 0; > >>+ if (connect(sock, e->ai_addr, e->ai_addrlen)< 0) { > >>+ err = -socket_error(); > > > connect error is got here > > > >>+ } > >>+#ifndef _WIN32 > >>+ } while (err == -EINTR || err == -EWOULDBLOCK); > >>+#else > >>+ } while (err == -EINTR); > >>+#endif > >>+ > >>+ if (err>= 0) { > >>+ goto success; > >>+ } else if (!block&& err == -EINPROGRESS) { > >>+ goto success; > >>+#ifdef _WIN32 > >>+ } else if (!block&& err == -WSAEALREADY) { > >>+ goto success; > >>+#endif > >> } > >>- freeaddrinfo(res); > >>- return sock; > >>+ > >>+ if (NULL == e->ai_next) { > >>+ fprintf(stderr, "%s: connect(%s,%s,%s,%s): %s\n", __FUNCTION__, > >>+ inet_strfamily(e->ai_family), > >>+ e->ai_canonname, uaddr, uport, strerror(errno)); > >>+ } > >>+ closesocket(sock); > >> } > >> freeaddrinfo(res); > >> return -1; > >>+ > >>+success: > >>+ freeaddrinfo(res); > >>+ return sock; > >> } > >> > >> int inet_dgram_opts(QemuOpts *opts) > >>@@ -449,14 +477,18 @@ int inet_listen(const char *str, char *ostr, int olen, > >> return sock; > >> } > >> > >>-int inet_connect(const char *str, int socktype) > >>+int inet_connect(const char *str, bool block) > >> { > >> QemuOpts *opts; > >> int sock = -1; > >> > >> opts = qemu_opts_create(&dummy_opts, NULL, 0); > >>- if (inet_parse(opts, str) == 0) > >>+ if (inet_parse(opts, str) == 0) { > >>+ if (block) { > >>+ qemu_opt_set(opts, "block", "on"); > >>+ } > >> sock = inet_connect_opts(opts); > >>+ } > >> qemu_opts_del(opts); > >> return sock; > >> } > >>diff --git a/qemu_socket.h b/qemu_socket.h > >>index a5d0a84..f73e26d 100644 > >>--- a/qemu_socket.h > >>+++ b/qemu_socket.h > >>@@ -41,7 +41,7 @@ int inet_listen_opts(QemuOpts *opts, int port_offset); > >> int inet_listen(const char *str, char *ostr, int olen, > >> int socktype, int port_offset); > >> int inet_connect_opts(QemuOpts *opts); > >>-int inet_connect(const char *str, int socktype); > >>+int inet_connect(const char *str, bool block); > >> int inet_dgram_opts(QemuOpts *opts); > >> const char *inet_strfamily(int family); > >> > >>diff --git a/ui/vnc.c b/ui/vnc.c > >>index deb9ecd..4a96153 100644 > >>--- a/ui/vnc.c > >>+++ b/ui/vnc.c > >>@@ -3068,7 +3068,7 @@ int vnc_display_open(DisplayState *ds, const char *display) > >> if (strncmp(display, "unix:", 5) == 0) > >> vs->lsock = unix_connect(display+5); > >> else > >>- vs->lsock = inet_connect(display, SOCK_STREAM); > >>+ vs->lsock = inet_connect(display, true); > >> if (-1 == vs->lsock) { > >> g_free(vs->display); > >> vs->display = NULL; > >> > >> > > > > -- > Amos. >
On Wed, Apr 18, 2012 at 10:49:02AM +0800, Amos Kong wrote: > On 18/04/12 10:10, Michael Roth wrote: > >On Tue, Apr 17, 2012 at 10:54:01PM +0800, Amos Kong wrote: > >>Add a bool argument to inet_connect() to assign if set socket > >>to block/nonblock, and delete original argument 'socktype' > >>that is unused. > >> > >>Retry to connect when following errors are got: > >> -EINTR > >> -EWOULDBLOCK (win32) > >>Connect's successful for nonblock socket when following > >>errors are got, user should wait for connecting by select(): > >> -EINPROGRESS > >> -WSAEALREADY (win32) > > > >How does the user access these? Via errno/socket_error()? > > Yes, connect error is got by socket_error() > > > I'm not sure > >how that would work, I don't think we can rely on the value of errno > >unless a function indicates an error, so for the case where we're > >successful but have an EINPROGRESS, I don't think we have a way of > >reliably obtaining that (errno value might be stale) except maybe > >something like getsockopt(). > > >I think it would be better to just merge this with your patch #3, which > >handles all this nicely via Error. > > We use Error to pass meaningful error, but we need to process > original socket error, > which is got by socket_error(). Don't use Error as an indicator to check socket_error(). If we want to pass back something akin to errno, it should be done via Error. Paolo had some suggestions on how to do this, but I think the Errors you introduced in patch #2 are sufficient for now. qmp_migrate() currently just sets QERR_UNDEFINED_ERROR and ignores the actual value of socket_error(), we only use it for DPRINTF(), so if you still want to dump the output of socket_error()/errno to stderr, do it early in inet_connect()/inet_listen() so we don't have to propagate anything back tcp_start_outgoing_migration() other than **errp. > > In qemu, we used socket_error() to get socket error in many place. > If you are confused > with how that would work, we need to fix socket_error. I don't want > to fix this in the > migration patchset. > > What's your opinion? > > >>Change nbd, vnc to use new interface. > >> > >>Signed-off-by: Amos Kong<akong@redhat.com> > >>--- > >> nbd.c | 2 +- > >> qemu-sockets.c | 58 +++++++++++++++++++++++++++++++++++++++++++------------- > >> qemu_socket.h | 2 +- > >> ui/vnc.c | 2 +- > >> 4 files changed, 48 insertions(+), 16 deletions(-) > >> > >>diff --git a/nbd.c b/nbd.c > >>index 406e555..b4e68a9 100644 > >>--- a/nbd.c > >>+++ b/nbd.c > >>@@ -146,7 +146,7 @@ int tcp_socket_outgoing(const char *address, uint16_t port) > >> > >> int tcp_socket_outgoing_spec(const char *address_and_port) > >> { > >>- return inet_connect(address_and_port, SOCK_STREAM); > >>+ return inet_connect(address_and_port, true); > >> } > >> > >> int tcp_socket_incoming(const char *address, uint16_t port) > >>diff --git a/qemu-sockets.c b/qemu-sockets.c > >>index 6bcb8e3..e886195 100644 > >>--- a/qemu-sockets.c > >>+++ b/qemu-sockets.c > >>@@ -51,6 +51,9 @@ static QemuOptsList dummy_opts = { > >> },{ > >> .name = "ipv6", > >> .type = QEMU_OPT_BOOL, > >>+ },{ > >>+ .name = "block", > >>+ .type = QEMU_OPT_BOOL, > >> }, > >> { /* end if list */ } > >> }, > >>@@ -201,7 +204,8 @@ int inet_connect_opts(QemuOpts *opts) > >> const char *port; > >> char uaddr[INET6_ADDRSTRLEN+1]; > >> char uport[33]; > >>- int sock,rc; > >>+ int sock, rc, err; > >>+ bool block; > >> > >> memset(&ai,0, sizeof(ai)); > >> ai.ai_flags = AI_CANONNAME | AI_ADDRCONFIG; > >>@@ -210,6 +214,7 @@ int inet_connect_opts(QemuOpts *opts) > >> > >> addr = qemu_opt_get(opts, "host"); > >> port = qemu_opt_get(opts, "port"); > >>+ block = qemu_opt_get_bool(opts, "block", 0); > >> if (addr == NULL || port == NULL) { > >> fprintf(stderr, "inet_connect: host and/or port not specified\n"); > >> return -1; > >>@@ -241,21 +246,44 @@ int inet_connect_opts(QemuOpts *opts) > >> continue; > >> } > >> setsockopt(sock,SOL_SOCKET,SO_REUSEADDR,(void*)&on,sizeof(on)); > >>- > >>+ if (!block) { > >>+ socket_set_nonblock(sock); > >>+ } > >> /* connect to peer */ > >>- if (connect(sock,e->ai_addr,e->ai_addrlen)< 0) { > >>- if (NULL == e->ai_next) > >>- fprintf(stderr, "%s: connect(%s,%s,%s,%s): %s\n", __FUNCTION__, > >>- inet_strfamily(e->ai_family), > >>- e->ai_canonname, uaddr, uport, strerror(errno)); > >>- closesocket(sock); > >>- continue; > >>+ do { > >>+ err = 0; > >>+ if (connect(sock, e->ai_addr, e->ai_addrlen)< 0) { > >>+ err = -socket_error(); > > > connect error is got here > > > >>+ } > >>+#ifndef _WIN32 > >>+ } while (err == -EINTR || err == -EWOULDBLOCK); > >>+#else > >>+ } while (err == -EINTR); > >>+#endif > >>+ > >>+ if (err>= 0) { > >>+ goto success; > >>+ } else if (!block&& err == -EINPROGRESS) { > >>+ goto success; > >>+#ifdef _WIN32 > >>+ } else if (!block&& err == -WSAEALREADY) { > >>+ goto success; > >>+#endif > >> } > >>- freeaddrinfo(res); > >>- return sock; > >>+ > >>+ if (NULL == e->ai_next) { > >>+ fprintf(stderr, "%s: connect(%s,%s,%s,%s): %s\n", __FUNCTION__, > >>+ inet_strfamily(e->ai_family), > >>+ e->ai_canonname, uaddr, uport, strerror(errno)); > >>+ } > >>+ closesocket(sock); > >> } > >> freeaddrinfo(res); > >> return -1; > >>+ > >>+success: > >>+ freeaddrinfo(res); > >>+ return sock; > >> } > >> > >> int inet_dgram_opts(QemuOpts *opts) > >>@@ -449,14 +477,18 @@ int inet_listen(const char *str, char *ostr, int olen, > >> return sock; > >> } > >> > >>-int inet_connect(const char *str, int socktype) > >>+int inet_connect(const char *str, bool block) > >> { > >> QemuOpts *opts; > >> int sock = -1; > >> > >> opts = qemu_opts_create(&dummy_opts, NULL, 0); > >>- if (inet_parse(opts, str) == 0) > >>+ if (inet_parse(opts, str) == 0) { > >>+ if (block) { > >>+ qemu_opt_set(opts, "block", "on"); > >>+ } > >> sock = inet_connect_opts(opts); > >>+ } > >> qemu_opts_del(opts); > >> return sock; > >> } > >>diff --git a/qemu_socket.h b/qemu_socket.h > >>index a5d0a84..f73e26d 100644 > >>--- a/qemu_socket.h > >>+++ b/qemu_socket.h > >>@@ -41,7 +41,7 @@ int inet_listen_opts(QemuOpts *opts, int port_offset); > >> int inet_listen(const char *str, char *ostr, int olen, > >> int socktype, int port_offset); > >> int inet_connect_opts(QemuOpts *opts); > >>-int inet_connect(const char *str, int socktype); > >>+int inet_connect(const char *str, bool block); > >> int inet_dgram_opts(QemuOpts *opts); > >> const char *inet_strfamily(int family); > >> > >>diff --git a/ui/vnc.c b/ui/vnc.c > >>index deb9ecd..4a96153 100644 > >>--- a/ui/vnc.c > >>+++ b/ui/vnc.c > >>@@ -3068,7 +3068,7 @@ int vnc_display_open(DisplayState *ds, const char *display) > >> if (strncmp(display, "unix:", 5) == 0) > >> vs->lsock = unix_connect(display+5); > >> else > >>- vs->lsock = inet_connect(display, SOCK_STREAM); > >>+ vs->lsock = inet_connect(display, true); > >> if (-1 == vs->lsock) { > >> g_free(vs->display); > >> vs->display = NULL; > >> > >> > > > > -- > Amos. >
diff --git a/nbd.c b/nbd.c index 406e555..b4e68a9 100644 --- a/nbd.c +++ b/nbd.c @@ -146,7 +146,7 @@ int tcp_socket_outgoing(const char *address, uint16_t port) int tcp_socket_outgoing_spec(const char *address_and_port) { - return inet_connect(address_and_port, SOCK_STREAM); + return inet_connect(address_and_port, true); } int tcp_socket_incoming(const char *address, uint16_t port) diff --git a/qemu-sockets.c b/qemu-sockets.c index 6bcb8e3..e886195 100644 --- a/qemu-sockets.c +++ b/qemu-sockets.c @@ -51,6 +51,9 @@ static QemuOptsList dummy_opts = { },{ .name = "ipv6", .type = QEMU_OPT_BOOL, + },{ + .name = "block", + .type = QEMU_OPT_BOOL, }, { /* end if list */ } }, @@ -201,7 +204,8 @@ int inet_connect_opts(QemuOpts *opts) const char *port; char uaddr[INET6_ADDRSTRLEN+1]; char uport[33]; - int sock,rc; + int sock, rc, err; + bool block; memset(&ai,0, sizeof(ai)); ai.ai_flags = AI_CANONNAME | AI_ADDRCONFIG; @@ -210,6 +214,7 @@ int inet_connect_opts(QemuOpts *opts) addr = qemu_opt_get(opts, "host"); port = qemu_opt_get(opts, "port"); + block = qemu_opt_get_bool(opts, "block", 0); if (addr == NULL || port == NULL) { fprintf(stderr, "inet_connect: host and/or port not specified\n"); return -1; @@ -241,21 +246,44 @@ int inet_connect_opts(QemuOpts *opts) continue; } setsockopt(sock,SOL_SOCKET,SO_REUSEADDR,(void*)&on,sizeof(on)); - + if (!block) { + socket_set_nonblock(sock); + } /* connect to peer */ - if (connect(sock,e->ai_addr,e->ai_addrlen) < 0) { - if (NULL == e->ai_next) - fprintf(stderr, "%s: connect(%s,%s,%s,%s): %s\n", __FUNCTION__, - inet_strfamily(e->ai_family), - e->ai_canonname, uaddr, uport, strerror(errno)); - closesocket(sock); - continue; + do { + err = 0; + if (connect(sock, e->ai_addr, e->ai_addrlen) < 0) { + err = -socket_error(); + } +#ifndef _WIN32 + } while (err == -EINTR || err == -EWOULDBLOCK); +#else + } while (err == -EINTR); +#endif + + if (err >= 0) { + goto success; + } else if (!block && err == -EINPROGRESS) { + goto success; +#ifdef _WIN32 + } else if (!block && err == -WSAEALREADY) { + goto success; +#endif } - freeaddrinfo(res); - return sock; + + if (NULL == e->ai_next) { + fprintf(stderr, "%s: connect(%s,%s,%s,%s): %s\n", __FUNCTION__, + inet_strfamily(e->ai_family), + e->ai_canonname, uaddr, uport, strerror(errno)); + } + closesocket(sock); } freeaddrinfo(res); return -1; + +success: + freeaddrinfo(res); + return sock; } int inet_dgram_opts(QemuOpts *opts) @@ -449,14 +477,18 @@ int inet_listen(const char *str, char *ostr, int olen, return sock; } -int inet_connect(const char *str, int socktype) +int inet_connect(const char *str, bool block) { QemuOpts *opts; int sock = -1; opts = qemu_opts_create(&dummy_opts, NULL, 0); - if (inet_parse(opts, str) == 0) + if (inet_parse(opts, str) == 0) { + if (block) { + qemu_opt_set(opts, "block", "on"); + } sock = inet_connect_opts(opts); + } qemu_opts_del(opts); return sock; } diff --git a/qemu_socket.h b/qemu_socket.h index a5d0a84..f73e26d 100644 --- a/qemu_socket.h +++ b/qemu_socket.h @@ -41,7 +41,7 @@ int inet_listen_opts(QemuOpts *opts, int port_offset); int inet_listen(const char *str, char *ostr, int olen, int socktype, int port_offset); int inet_connect_opts(QemuOpts *opts); -int inet_connect(const char *str, int socktype); +int inet_connect(const char *str, bool block); int inet_dgram_opts(QemuOpts *opts); const char *inet_strfamily(int family); diff --git a/ui/vnc.c b/ui/vnc.c index deb9ecd..4a96153 100644 --- a/ui/vnc.c +++ b/ui/vnc.c @@ -3068,7 +3068,7 @@ int vnc_display_open(DisplayState *ds, const char *display) if (strncmp(display, "unix:", 5) == 0) vs->lsock = unix_connect(display+5); else - vs->lsock = inet_connect(display, SOCK_STREAM); + vs->lsock = inet_connect(display, true); if (-1 == vs->lsock) { g_free(vs->display); vs->display = NULL;
Add a bool argument to inet_connect() to assign if set socket to block/nonblock, and delete original argument 'socktype' that is unused. Retry to connect when following errors are got: -EINTR -EWOULDBLOCK (win32) Connect's successful for nonblock socket when following errors are got, user should wait for connecting by select(): -EINPROGRESS -WSAEALREADY (win32) Change nbd, vnc to use new interface. Signed-off-by: Amos Kong <akong@redhat.com> --- nbd.c | 2 +- qemu-sockets.c | 58 +++++++++++++++++++++++++++++++++++++++++++------------- qemu_socket.h | 2 +- ui/vnc.c | 2 +- 4 files changed, 48 insertions(+), 16 deletions(-)