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

login
register
mail settings
Submitter Paolo Bonzini
Date Oct. 10, 2012, 2:02 p.m.
Message ID <1349877786-23514-16-git-send-email-pbonzini@redhat.com>
Download mbox | patch
Permalink /patch/190666/
State New
Headers show

Comments

Paolo Bonzini - Oct. 10, 2012, 2:02 p.m.
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.

Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 qemu-sockets.c | 15 ++++++---------
 1 file modificato, 6 inserzioni(+), 9 rimozioni(-)
Luiz Capitulino - Oct. 16, 2012, 12:53 p.m.
On Wed, 10 Oct 2012 16:02:56 +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.
> 
> Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>

Are you guys related? :)

Do you want me to clean this up before applying or doesn't matter?

> ---
>  qemu-sockets.c | 15 ++++++---------
>  1 file modificato, 6 inserzioni(+), 9 rimozioni(-)
> 
> diff --git a/qemu-sockets.c b/qemu-sockets.c
> index 3c19463..d0e1a41 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;
> @@ -263,7 +263,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;
>  
> @@ -271,8 +271,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;
>      }
>      qemu_setsockopt(sock, SOL_SOCKET, SO_REUSEADDR, &on, sizeof(on));
> @@ -293,6 +292,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);
>          closesocket(sock);
>          return -1;
>      }
> @@ -375,7 +375,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) {
> @@ -386,9 +386,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;
Markus Armbruster - Oct. 17, 2012, 3:40 p.m.
Paolo Bonzini <pbonzini@redhat.com> writes:

> 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.

The second sentence is about future work, isn't it?

If yes, suggest "we'll need to change" to make it obvious.

>
> Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>
> 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 3c19463..d0e1a41 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);

Doesn't this drop error messages?

If yes, but it's healed later in the series, the temporary breakage
still needs to be spelled out in the commit message.

>              /* connect in progress */
>              if (in_progress) {
>                  return;
> @@ -263,7 +263,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;
>  
> @@ -271,8 +271,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;
>      }
>      qemu_setsockopt(sock, SOL_SOCKET, SO_REUSEADDR, &on, sizeof(on));
> @@ -293,6 +292,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);

Looks like this reports a previously unreported error condition.

If it does, it's a fix worth mentioning in commit message.

>          closesocket(sock);
>          return -1;
>      }
> @@ -375,7 +375,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) {
> @@ -386,9 +386,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;
Paolo Bonzini - Oct. 17, 2012, 3:50 p.m.
Il 17/10/2012 17:40, Markus Armbruster ha scritto:
>> >      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);
> Doesn't this drop error messages?

Depends on what you mean by drop.  The error messages previously were
sent to stdio, hidden in a log file or just invisible, depending on how
QEMU is run.  (This is just for outgoing migration currently, so you
cannot rely on stdio as you can for command-line parsing).

> If yes, but it's healed later in the series, the temporary breakage
> still needs to be spelled out in the commit message.

No, it's not healed, which is why it's mentioned in the commit message
that future work is needed.

Paolo
Markus Armbruster - Oct. 19, 2012, 7:59 a.m.
Paolo Bonzini <pbonzini@redhat.com> writes:

> Il 17/10/2012 17:40, Markus Armbruster ha scritto:
>>> >      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);
>> Doesn't this drop error messages?
>
> Depends on what you mean by drop.  The error messages previously were
> sent to stdio, hidden in a log file or just invisible, depending on how
> QEMU is run.  (This is just for outgoing migration currently, so you
> cannot rely on stdio as you can for command-line parsing).

I agree the existing error reporting is very bad, but even very bad
reporting is (slightly) better than no error reporting.

Moreover, there's a use case where errors are reported in the correct
place now: -monitor stdio.  I'm afraid your patch hurts it.

>> If yes, but it's healed later in the series, the temporary breakage
>> still needs to be spelled out in the commit message.
>
> No, it's not healed, which is why it's mentioned in the commit message
> that future work is needed.

I really hate "reward" submissions of tons of useful work with demands
for more even work...  but here goes anyway: what's your take on
completing the job?  Straightfoward, or another ton of work?
Paolo Bonzini - Oct. 19, 2012, 8:50 a.m.
> >> If yes, but it's healed later in the series, the temporary
> >> breakage still needs to be spelled out in the commit message.
> >
> > No, it's not healed, which is why it's mentioned in the commit
> > message that future work is needed.
> 
> I really hate "reward" submissions of tons of useful work with demands
> for more even work...  but here goes anyway: what's your take on
> completing the job?  Straightfoward, or another ton of work?

Straightforward, also because we have exactly one user of asynchronous
connection (migration).  But I'll let Orit do it. :)

Paolo

Patch

diff --git a/qemu-sockets.c b/qemu-sockets.c
index 3c19463..d0e1a41 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;
@@ -263,7 +263,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;
 
@@ -271,8 +271,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;
     }
     qemu_setsockopt(sock, SOL_SOCKET, SO_REUSEADDR, &on, sizeof(on));
@@ -293,6 +292,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);
         closesocket(sock);
         return -1;
     }
@@ -375,7 +375,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) {
@@ -386,9 +386,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;