[15/18] qemu-sockets: add error propagation to inet_connect_addr

perror and fprintf can be removed because all clients can now consume
Errors properly.  However, we need to change the non-blocking connect
handlers to take an Error, in order to improve error handling for
migration with the TCP protocol.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 qemu-sockets.c | 15 ++++++---------
 1 file modificato, 6 inserzioni(+), 9 rimozioni(-)

On Wed,  3 Oct 2012 16:37:02 +0200
Paolo Bonzini <pbonzini@redhat.com> wrote:

> perror and fprintf can be removed because all clients can now consume
> Errors properly.  However, we need to change the non-blocking connect
> handlers to take an Error, in order to improve error handling for
> migration with the TCP protocol.
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  qemu-sockets.c | 15 ++++++---------
>  1 file modificato, 6 inserzioni(+), 9 rimozioni(-)
> 
> diff --git a/qemu-sockets.c b/qemu-sockets.c
> index 8ab2d0c..b096f49 100644
> --- a/qemu-sockets.c
> +++ b/qemu-sockets.c
> @@ -216,7 +216,7 @@ typedef struct ConnectState {
>  } ConnectState;
>  
>  static int inet_connect_addr(struct addrinfo *addr, bool *in_progress,
> -                             ConnectState *connect_state);
> +                             ConnectState *connect_state, Error **errp);
>  
>  static void wait_for_connect(void *opaque)
>  {
> @@ -246,7 +246,7 @@ static void wait_for_connect(void *opaque)
>      if (s->current_addr) {
>          while (s->current_addr->ai_next != NULL && s->fd < 0) {
>              s->current_addr = s->current_addr->ai_next;
> -            s->fd = inet_connect_addr(s->current_addr, &in_progress, s);
> +            s->fd = inet_connect_addr(s->current_addr, &in_progress, s, NULL);
>              /* connect in progress */
>              if (in_progress) {
>                  return;
> @@ -264,7 +264,7 @@ static void wait_for_connect(void *opaque)
>  }
>  
>  static int inet_connect_addr(struct addrinfo *addr, bool *in_progress,
> -                             ConnectState *connect_state)
> +                             ConnectState *connect_state, Error **errp)
>  {
>      int sock, rc;
>  
> @@ -272,8 +272,7 @@ static int inet_connect_addr(struct addrinfo *addr, bool *in_progress,
>  
>      sock = qemu_socket(addr->ai_family, addr->ai_socktype, addr->ai_protocol);
>      if (sock < 0) {
> -        fprintf(stderr, "%s: socket(%s): %s\n", __func__,
> -                inet_strfamily(addr->ai_family), strerror(errno));
> +        error_set_errno(errp, errno, QERR_SOCKET_CREATE_FAILED);
>          return -1;
>      }
>      setsockopt(sock, SOL_SOCKET, SO_REUSEADDR, &on, sizeof(on));
> @@ -294,6 +293,7 @@ static int inet_connect_addr(struct addrinfo *addr, bool *in_progress,
>                               connect_state);
>          *in_progress = true;
>      } else if (rc < 0) {
> +        error_set_errno(errp, errno, QERR_SOCKET_CONNECT_FAILED);

The patch look fine, but as I said in my previous email I really dislike
seeing QERR_ macros usage in new code. If the problem here is to duplicate
the error message, then maybe we could put this connect() block in a wrapper.

>          closesocket(sock);
>          return -1;
>      }
> @@ -376,7 +376,7 @@ int inet_connect_opts(QemuOpts *opts, Error **errp,
>          if (connect_state != NULL) {
>              connect_state->current_addr = e;
>          }
> -        sock = inet_connect_addr(e, &in_progress, connect_state);
> +        sock = inet_connect_addr(e, &in_progress, connect_state, errp);
>          if (in_progress) {
>              return sock;
>          } else if (sock >= 0) {
> @@ -387,9 +387,6 @@ int inet_connect_opts(QemuOpts *opts, Error **errp,
>              break;
>          }
>      }
> -    if (sock < 0) {
> -        error_set(errp, QERR_SOCKET_CONNECT_FAILED);
> -    }
>      g_free(connect_state);
>      freeaddrinfo(res);
>      return sock;

Il 09/10/2012 16:58, Luiz Capitulino ha scritto:
>> > +        error_set_errno(errp, errno, QERR_SOCKET_CONNECT_FAILED);
> The patch look fine, but as I said in my previous email I really dislike
> seeing QERR_ macros usage in new code. If the problem here is to duplicate
> the error message, then maybe we could put this connect() block in a wrapper.

Again: one thing at a time.

The only obvious step is to remove QERR_ constants that are used just
once.  Everything else should be done carefully because if later you
decide to add something to the errors (for example the bind or connect
argument) you have no protection against typos, etc.

Adding new QERR_ constants is somewhat harmful, but using the old ones
absolutely is not.

Paolo

On Tue, 09 Oct 2012 17:02:02 +0200
Paolo Bonzini <pbonzini@redhat.com> wrote:

> Il 09/10/2012 16:58, Luiz Capitulino ha scritto:
> >> > +        error_set_errno(errp, errno, QERR_SOCKET_CONNECT_FAILED);
> > The patch look fine, but as I said in my previous email I really dislike
> > seeing QERR_ macros usage in new code. If the problem here is to duplicate
> > the error message, then maybe we could put this connect() block in a wrapper.
> 
> Again: one thing at a time.

Right.

> The only obvious step is to remove QERR_ constants that are used just
> once. 

I agree.

> Everything else should be done carefully because if later you
> decide to add something to the errors (for example the bind or connect
> argument) you have no protection against typos, etc.

I don't fully agree. The best way to do this is to add new wrappers
to bind() or connect() that take an Error ** argument and not extend
the QERR_ macros. But I agree that we can do this later.

> Adding new QERR_ constants is somewhat harmful, but using the old ones
> absolutely is not.

Yes:

 Reviewed-by: Luiz Capitulino <lcapitulino@redhat.com>