Re: [Qemu-devel] [PATCH v8 2/4] sockets: change inet_connect() to support nonblock socket

2012-04-24 Thread Michael Roth
On Fri, Apr 20, 2012 at 12:40:24PM +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.
 Add a new argument to inet_connect()/inet_connect_opts(),
 to pass back connect error by error class.
 
 Retry to connect when -EINTR is got. Connect's successful
 for nonblock socket when following errors are got, user
 should wait for connecting by select():
   -EINPROGRESS
   -EWOULDBLOCK (win32)
   -WSAEALREADY (win32)
 
 Change nbd, vnc to use new interface.
 
 Changes from v7:
 - posix: let EWOULDBLOCK fall through to CONNECT_FAILED path
 - fix typo
 
 Signed-off-by: Amos Kong ak...@redhat.com
 ---
  nbd.c  |2 +-
  qemu-char.c|2 +-
  qemu-sockets.c |   46 +++---
  qemu_socket.h  |6 --
  ui/vnc.c   |2 +-
  5 files changed, 46 insertions(+), 12 deletions(-)
 
 diff --git a/nbd.c b/nbd.c
 index 406e555..bb71f00 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, NULL);
  }
 
  int tcp_socket_incoming(const char *address, uint16_t port)
 diff --git a/qemu-char.c b/qemu-char.c
 index 74c60e1..aeee2e8 100644
 --- a/qemu-char.c
 +++ b/qemu-char.c
 @@ -2444,7 +2444,7 @@ static CharDriverState *qemu_chr_open_socket(QemuOpts 
 *opts)
  if (is_listen) {
  fd = inet_listen_opts(opts, 0);
  } else {
 -fd = inet_connect_opts(opts);
 +fd = inet_connect_opts(opts, NULL);
  }
  }
  if (fd  0) {
 diff --git a/qemu-sockets.c b/qemu-sockets.c
 index 6bcb8e3..66799fc 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 */ }
  },
 @@ -194,14 +197,15 @@ listen:
  return slisten;
  }
 
 -int inet_connect_opts(QemuOpts *opts)
 +int inet_connect_opts(QemuOpts *opts, Error **errp)
  {
  struct addrinfo ai,*res,*e;
  const char *addr;
  const char *port;
  char uaddr[INET6_ADDRSTRLEN+1];
  char uport[33];
 -int sock,rc;
 +int sock, rc, err;

I'd just keep using rc since it's already here, and less easy to confuse
with errp.

 +bool block;
 
  memset(ai,0, sizeof(ai));
  ai.ai_flags = AI_CANONNAME | AI_ADDRCONFIG;
 @@ -210,8 +214,10 @@ 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);
 +error_set(errp, QERR_SOCKET_CREATE_FAILED);
  return -1;
  }
 
 @@ -224,6 +230,7 @@ int inet_connect_opts(QemuOpts *opts)
  if (0 != (rc = getaddrinfo(addr, port, ai, res))) {
  fprintf(stderr,getaddrinfo(%s,%s): %s\n, addr, port,
  gai_strerror(rc));
 +error_set(errp, QERR_SOCKET_CREATE_FAILED);
   return -1;
  }
 
 @@ -241,19 +248,38 @@ 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) {
 +do {
 +err = 0;
 +if (connect(sock, e-ai_addr, e-ai_addrlen)  0) {
 +err = -socket_error();
 +}
 +} while (err == -EINTR);
 +
 +  #ifdef _WIN32
 +if (!block  (err == -EINPROGRESS || err == -EWOULDBLOCK
 +   || err == -WSAEALREADY)) {
 +  #else
 +if (!block  (err == -EINPROGRESS)) {
 +  #endif
 +error_set(errp, QERR_SOCKET_CONNECT_IN_PROGRESS);
 +}
 +if (err  0  !error_is_type(*errp, 
 QERR_SOCKET_CONNECT_IN_PROGRESS)) {

You can get a NULL pointer dereference here ^ if they call this function
with errp == NULL

  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;
  continue;
  }
  freeaddrinfo(res);
  return sock;
  }
 +error_set(errp, QERR_SOCKET_CONNECT_FAILED);
  freeaddrinfo(res);
  return -1;
  }
 @@ -449,14 +475,20 @@ int inet_listen(const char *str, char *ostr, int olen,
  return sock;
  }
 
 -int 

Re: [Qemu-devel] [PATCH v8 2/4] sockets: change inet_connect() to support nonblock socket

2012-04-20 Thread Orit Wasserman
On 04/20/2012 07:40 AM, 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.
 Add a new argument to inet_connect()/inet_connect_opts(),
 to pass back connect error by error class.
 
 Retry to connect when -EINTR is got. Connect's successful
 for nonblock socket when following errors are got, user
 should wait for connecting by select():
   -EINPROGRESS
   -EWOULDBLOCK (win32)
   -WSAEALREADY (win32)
 
 Change nbd, vnc to use new interface.
 
 Changes from v7:
 - posix: let EWOULDBLOCK fall through to CONNECT_FAILED path
 - fix typo
 
 Signed-off-by: Amos Kong ak...@redhat.com
 ---
  nbd.c  |2 +-
  qemu-char.c|2 +-
  qemu-sockets.c |   46 +++---
  qemu_socket.h  |6 --
  ui/vnc.c   |2 +-
  5 files changed, 46 insertions(+), 12 deletions(-)
 
 diff --git a/nbd.c b/nbd.c
 index 406e555..bb71f00 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, NULL);
  }
  
  int tcp_socket_incoming(const char *address, uint16_t port)
 diff --git a/qemu-char.c b/qemu-char.c
 index 74c60e1..aeee2e8 100644
 --- a/qemu-char.c
 +++ b/qemu-char.c
 @@ -2444,7 +2444,7 @@ static CharDriverState *qemu_chr_open_socket(QemuOpts 
 *opts)
  if (is_listen) {
  fd = inet_listen_opts(opts, 0);
  } else {
 -fd = inet_connect_opts(opts);
 +fd = inet_connect_opts(opts, NULL);
  }
  }
  if (fd  0) {
 diff --git a/qemu-sockets.c b/qemu-sockets.c
 index 6bcb8e3..66799fc 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 */ }
  },
 @@ -194,14 +197,15 @@ listen:
  return slisten;
  }
  
 -int inet_connect_opts(QemuOpts *opts)
 +int inet_connect_opts(QemuOpts *opts, Error **errp)
  {
  struct addrinfo ai,*res,*e;
  const char *addr;
  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,8 +214,10 @@ 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);
 +error_set(errp, QERR_SOCKET_CREATE_FAILED);
  return -1;
  }
  
 @@ -224,6 +230,7 @@ int inet_connect_opts(QemuOpts *opts)
  if (0 != (rc = getaddrinfo(addr, port, ai, res))) {
  fprintf(stderr,getaddrinfo(%s,%s): %s\n, addr, port,
  gai_strerror(rc));
 +error_set(errp, QERR_SOCKET_CREATE_FAILED);
   return -1;
  }
  
 @@ -241,19 +248,38 @@ 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) {
 +do {
 +err = 0;
 +if (connect(sock, e-ai_addr, e-ai_addrlen)  0) {
 +err = -socket_error();
 +}
 +} while (err == -EINTR);
 +
 +  #ifdef _WIN32
 +if (!block  (err == -EINPROGRESS || err == -EWOULDBLOCK
 +   || err == -WSAEALREADY)) {
 +  #else
 +if (!block  (err == -EINPROGRESS)) {
 +  #endif
 +error_set(errp, QERR_SOCKET_CONNECT_IN_PROGRESS);
 +}
 +if (err  0  !error_is_type(*errp, 
 QERR_SOCKET_CONNECT_IN_PROGRESS)) {
  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;
  continue;
  }
  freeaddrinfo(res);
  return sock;
  }
 +error_set(errp, QERR_SOCKET_CONNECT_FAILED);
  freeaddrinfo(res);
  return -1;
  }
 @@ -449,14 +475,20 @@ 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, Error **errp)
  {
  QemuOpts *opts;
  int sock = -1;
  
  opts = 

[Qemu-devel] [PATCH v8 2/4] sockets: change inet_connect() to support nonblock socket

2012-04-19 Thread Amos Kong
Add a bool argument to inet_connect() to assign if set socket
to block/nonblock, and delete original argument 'socktype'
that is unused.
Add a new argument to inet_connect()/inet_connect_opts(),
to pass back connect error by error class.

Retry to connect when -EINTR is got. Connect's successful
for nonblock socket when following errors are got, user
should wait for connecting by select():
  -EINPROGRESS
  -EWOULDBLOCK (win32)
  -WSAEALREADY (win32)

Change nbd, vnc to use new interface.

Changes from v7:
- posix: let EWOULDBLOCK fall through to CONNECT_FAILED path
- fix typo

Signed-off-by: Amos Kong ak...@redhat.com
---
 nbd.c  |2 +-
 qemu-char.c|2 +-
 qemu-sockets.c |   46 +++---
 qemu_socket.h  |6 --
 ui/vnc.c   |2 +-
 5 files changed, 46 insertions(+), 12 deletions(-)

diff --git a/nbd.c b/nbd.c
index 406e555..bb71f00 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, NULL);
 }
 
 int tcp_socket_incoming(const char *address, uint16_t port)
diff --git a/qemu-char.c b/qemu-char.c
index 74c60e1..aeee2e8 100644
--- a/qemu-char.c
+++ b/qemu-char.c
@@ -2444,7 +2444,7 @@ static CharDriverState *qemu_chr_open_socket(QemuOpts 
*opts)
 if (is_listen) {
 fd = inet_listen_opts(opts, 0);
 } else {
-fd = inet_connect_opts(opts);
+fd = inet_connect_opts(opts, NULL);
 }
 }
 if (fd  0) {
diff --git a/qemu-sockets.c b/qemu-sockets.c
index 6bcb8e3..66799fc 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 */ }
 },
@@ -194,14 +197,15 @@ listen:
 return slisten;
 }
 
-int inet_connect_opts(QemuOpts *opts)
+int inet_connect_opts(QemuOpts *opts, Error **errp)
 {
 struct addrinfo ai,*res,*e;
 const char *addr;
 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,8 +214,10 @@ 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);
+error_set(errp, QERR_SOCKET_CREATE_FAILED);
 return -1;
 }
 
@@ -224,6 +230,7 @@ int inet_connect_opts(QemuOpts *opts)
 if (0 != (rc = getaddrinfo(addr, port, ai, res))) {
 fprintf(stderr,getaddrinfo(%s,%s): %s\n, addr, port,
 gai_strerror(rc));
+error_set(errp, QERR_SOCKET_CREATE_FAILED);
return -1;
 }
 
@@ -241,19 +248,38 @@ 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) {
+do {
+err = 0;
+if (connect(sock, e-ai_addr, e-ai_addrlen)  0) {
+err = -socket_error();
+}
+} while (err == -EINTR);
+
+  #ifdef _WIN32
+if (!block  (err == -EINPROGRESS || err == -EWOULDBLOCK
+   || err == -WSAEALREADY)) {
+  #else
+if (!block  (err == -EINPROGRESS)) {
+  #endif
+error_set(errp, QERR_SOCKET_CONNECT_IN_PROGRESS);
+}
+if (err  0  !error_is_type(*errp, QERR_SOCKET_CONNECT_IN_PROGRESS)) 
{
 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;
 continue;
 }
 freeaddrinfo(res);
 return sock;
 }
+error_set(errp, QERR_SOCKET_CONNECT_FAILED);
 freeaddrinfo(res);
 return -1;
 }
@@ -449,14 +475,20 @@ 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, Error **errp)
 {
 QemuOpts *opts;
 int sock = -1;
 
 opts = qemu_opts_create(dummy_opts, NULL, 0);
-if (inet_parse(opts, str) == 0)
-sock = inet_connect_opts(opts);
+if (inet_parse(opts, str) == 0) {
+if (block) {
+qemu_opt_set(opts, block, on);
+