[v6,3/5] sockets: use error class to pass connect error

Add a new argument in inet_connect()/inet_connect_opts()
to pass back connect error.

Change nbd, vnc to use new interface.

Signed-off-by: Amos Kong <akong@redhat.com>
---
 nbd.c          |    2 +-
 qemu-char.c    |    2 +-
 qemu-sockets.c |   13 ++++++++++---
 qemu_socket.h  |    6 ++++--
 ui/vnc.c       |    2 +-
 5 files changed, 17 insertions(+), 8 deletions(-)

On Tue, Apr 17, 2012 at 10:54:20PM +0800, Amos Kong wrote:
> Add a new argument in inet_connect()/inet_connect_opts()
> to pass back connect error.
> 
> Change nbd, vnc to use new interface.

Looks good!

Reviewed-by: Michael Roth <mdroth@linux.vnet.ibm.com>

> 
> Signed-off-by: Amos Kong <akong@redhat.com>
> ---
>  nbd.c          |    2 +-
>  qemu-char.c    |    2 +-
>  qemu-sockets.c |   13 ++++++++++---
>  qemu_socket.h  |    6 ++++--
>  ui/vnc.c       |    2 +-
>  5 files changed, 17 insertions(+), 8 deletions(-)
> 
> diff --git a/nbd.c b/nbd.c
> index b4e68a9..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, true);
> +    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..09f990a 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_err(opts, NULL);
>          }
>      }
>      if (fd < 0) {
> diff --git a/qemu-sockets.c b/qemu-sockets.c
> index e886195..2bd87fa 100644
> --- a/qemu-sockets.c
> +++ b/qemu-sockets.c
> @@ -197,7 +197,7 @@ 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;
> @@ -217,6 +217,7 @@ int inet_connect_opts(QemuOpts *opts)
>      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;
>      }
> 
> @@ -229,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;
>      }
> 
> @@ -254,6 +256,7 @@ int inet_connect_opts(QemuOpts *opts)
>              err = 0;
>              if (connect(sock, e->ai_addr, e->ai_addrlen) < 0) {
>                  err = -socket_error();
> +                error_set(errp, QERR_SOCKET_CONNECT_FAILED);
>              }
>  #ifndef _WIN32
>          } while (err == -EINTR || err == -EWOULDBLOCK);
> @@ -264,9 +267,11 @@ int inet_connect_opts(QemuOpts *opts)
>          if (err >= 0) {
>              goto success;
>          } else if (!block && err == -EINPROGRESS) {
> +            error_set(errp, QERR_SOCKET_CONNECT_IN_PROGRESS);
>              goto success;
>  #ifdef _WIN32
>          } else if (!block && err == -WSAEALREADY) {
> +            error_set(errp, QERR_SOCKET_CONNECT_IN_PROGRESS);
>              goto success;
>  #endif
>          }
> @@ -477,7 +482,7 @@ int inet_listen(const char *str, char *ostr, int olen,
>      return sock;
>  }
> 
> -int inet_connect(const char *str, bool block)
> +int inet_connect(const char *str, bool block, Error **errp)
>  {
>      QemuOpts *opts;
>      int sock = -1;
> @@ -487,7 +492,9 @@ int inet_connect(const char *str, bool block)
>          if (block) {
>              qemu_opt_set(opts, "block", "on");
>          }
> -        sock = inet_connect_opts(opts);
> +        sock = inet_connect_opts(opts, errp);
> +    } else {
> +        error_set(errp, QERR_SOCKET_CREATE_FAILED);
>      }
>      qemu_opts_del(opts);
>      return sock;
> diff --git a/qemu_socket.h b/qemu_socket.h
> index f73e26d..26998ef 100644
> --- a/qemu_socket.h
> +++ b/qemu_socket.h
> @@ -27,6 +27,8 @@ int inet_aton(const char *cp, struct in_addr *ia);
>  #endif /* !_WIN32 */
> 
>  #include "qemu-option.h"
> +#include "error.h"
> +#include "qerror.h"
> 
>  /* misc helpers */
>  int qemu_socket(int domain, int type, int protocol);
> @@ -40,8 +42,8 @@ int send_all(int fd, const void *buf, int len1);
>  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, bool block);
> +int inet_connect_opts(QemuOpts *opts, Error **errp);
> +int inet_connect(const char *str, bool block, Error **errp);
>  int inet_dgram_opts(QemuOpts *opts);
>  const char *inet_strfamily(int family);
> 
> diff --git a/ui/vnc.c b/ui/vnc.c
> index 4a96153..3ae7704 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, true);
> +            vs->lsock = inet_connect(display, true, NULL);
>          if (-1 == vs->lsock) {
>              g_free(vs->display);
>              vs->display = NULL;
>

On 04/17/2012 05:54 PM, Amos Kong wrote:
> Add a new argument in inet_connect()/inet_connect_opts()
> to pass back connect error.
> 
> Change nbd, vnc to use new interface.
> 
> Signed-off-by: Amos Kong <akong@redhat.com>
> ---
>  nbd.c          |    2 +-
>  qemu-char.c    |    2 +-
>  qemu-sockets.c |   13 ++++++++++---
>  qemu_socket.h  |    6 ++++--
>  ui/vnc.c       |    2 +-
>  5 files changed, 17 insertions(+), 8 deletions(-)
> 
> diff --git a/nbd.c b/nbd.c
> index b4e68a9..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, true);
> +    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..09f990a 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_err(opts, NULL);
>          }
>      }
>      if (fd < 0) {
> diff --git a/qemu-sockets.c b/qemu-sockets.c
> index e886195..2bd87fa 100644
> --- a/qemu-sockets.c
> +++ b/qemu-sockets.c
> @@ -197,7 +197,7 @@ 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;
> @@ -217,6 +217,7 @@ int inet_connect_opts(QemuOpts *opts)
>      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);

More details on the error (see previous patch comment) you can add the string in the fprintf.

>          return -1;
>      }
>  
> @@ -229,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);

same here , you can add gai_strerror(rc) string.

>  	return -1;
>      }
>  
> @@ -254,6 +256,7 @@ int inet_connect_opts(QemuOpts *opts)
>              err = 0;
>              if (connect(sock, e->ai_addr, e->ai_addrlen) < 0) {
>                  err = -socket_error();
> +                error_set(errp, QERR_SOCKET_CONNECT_FAILED);

This can cause a leak later in case of nonblocking socket when you call error_set again.
why not do the check here and set the correct error.

you can check later in the function to check if we are in connect in progress
something like (add also the WIN32):

if (!block && err == -EINPROGRESS ) {
	error_set(errp,QERR_CONNECT_IN_PROGRESS).
} else {
 	error_set(errp, QERR_SOCKET_CONNECT_FAILED);
}

>              }
>  #ifndef _WIN32
>          } while (err == -EINTR || err == -EWOULDBLOCK);
> @@ -264,9 +267,11 @@ int inet_connect_opts(QemuOpts *opts)
>          if (err >= 0) {
>              goto success;
>          } else if (!block && err == -EINPROGRESS) {
> +            error_set(errp, QERR_SOCKET_CONNECT_IN_PROGRESS);
>              goto success;
>  #ifdef _WIN32
>          } else if (!block && err == -WSAEALREADY) {
> +            error_set(errp, QERR_SOCKET_CONNECT_IN_PROGRESS);
>              goto success;
>  #endif
>          }

This will be removed and if you changed the code to:

if (error_is_set(errp) && !error_is_type(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
}

freeaddrinfo(res);
return sock;

Orit

> @@ -477,7 +482,7 @@ int inet_listen(const char *str, char *ostr, int olen,
>      return sock;
>  }
>  
> -int inet_connect(const char *str, bool block)
> +int inet_connect(const char *str, bool block, Error **errp)
>  {
>      QemuOpts *opts;
>      int sock = -1;
> @@ -487,7 +492,9 @@ int inet_connect(const char *str, bool block)
>          if (block) {
>              qemu_opt_set(opts, "block", "on");
>          }
> -        sock = inet_connect_opts(opts);
> +        sock = inet_connect_opts(opts, errp);
> +    } else {
> +        error_set(errp, QERR_SOCKET_CREATE_FAILED);
>      }
>      qemu_opts_del(opts);
>      return sock;
> diff --git a/qemu_socket.h b/qemu_socket.h
> index f73e26d..26998ef 100644
> --- a/qemu_socket.h
> +++ b/qemu_socket.h
> @@ -27,6 +27,8 @@ int inet_aton(const char *cp, struct in_addr *ia);
>  #endif /* !_WIN32 */
>  
>  #include "qemu-option.h"
> +#include "error.h"
> +#include "qerror.h"
>  
>  /* misc helpers */
>  int qemu_socket(int domain, int type, int protocol);
> @@ -40,8 +42,8 @@ int send_all(int fd, const void *buf, int len1);
>  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, bool block);
> +int inet_connect_opts(QemuOpts *opts, Error **errp);
> +int inet_connect(const char *str, bool block, Error **errp);
>  int inet_dgram_opts(QemuOpts *opts);
>  const char *inet_strfamily(int family);
>  
> diff --git a/ui/vnc.c b/ui/vnc.c
> index 4a96153..3ae7704 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, true);
> +            vs->lsock = inet_connect(display, true, NULL);
>          if (-1 == vs->lsock) {
>              g_free(vs->display);
>              vs->display = NULL;
>