Re: [Qemu-devel] [PATCH 08/18] migration (outgoing): add error propagation for fd and exec protocols

[Luiz Capitulino <lcapitulino@redhat.com>]

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

> Error propagation is already there for socket backends, but it
> is (and remains) incomplete because no Error is passed to the
> NonBlockingConnectHandler.
> 
> With all protocols understanding Error, the code can be simplified
> by removing the return value.
> 
> Before:
> 
>     (qemu) migrate fd:ffff
>     migrate: An undefined error has occurred
>     (qemu) info migrate
>     (qemu)
> 
> After:
> 
>     (qemu) migrate fd:ffff
>     migrate: File descriptor named 'ffff' has not been found
>     (qemu) info migrate
>     capabilities: xbzrle: off
>     Migration status: failed
>     total time: 0 milliseconds

This is really good, we're missing having good errors in the migrate
command for ages!

But I have comments :)

> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  migration-exec.c |  8 +++-----
>  migration-fd.c   | 11 ++++-------
>  migration-tcp.c  | 13 ++-----------
>  migration-unix.c | 11 ++---------
>  migration.c      | 17 ++++++-----------
>  migration.h      |  9 ++++-----
>  6 file modificati, 21 inserzioni(+), 48 rimozioni(-)
> 
> diff --git a/migration-exec.c b/migration-exec.c
> index 6c97db9..f3baf85 100644
> --- a/migration-exec.c
> +++ b/migration-exec.c
> @@ -60,19 +60,17 @@ static int exec_close(MigrationState *s)
>      return ret;
>  }
>  
> -int exec_start_outgoing_migration(MigrationState *s, const char *command)
> +void exec_start_outgoing_migration(MigrationState *s, const char *command, Error **errp)
>  {
>      FILE *f;
>  
>      f = popen(command, "w");
>      if (f == NULL) {
> -        DPRINTF("Unable to popen exec target\n");

That DPRINTF() usage is really bizarre, it seems its purpose is to report
an error to the user, but that's a debugging call.

I'd let it there and replace it later with proper tracing code, but that's
quite minor for me. Please, at least mention you're dropping it in the log.

>          goto err_after_popen;
>      }
>  
>      s->fd = fileno(f);
>      if (s->fd == -1) {
> -        DPRINTF("Unable to retrieve file descriptor for popen'd handle\n");
>          goto err_after_open;
>      }
>  
> @@ -85,12 +83,12 @@ int exec_start_outgoing_migration(MigrationState *s, const char *command)
>      s->write = file_write;
>  
>      migrate_fd_connect(s);
> -    return 0;
> +    return;
>  
>  err_after_open:
>      pclose(f);
>  err_after_popen:
> -    return -1;
> +    error_setg_errno(errp, errno, "failed to popen the migration target");

The pclose() call will override errno.

Otherwise looks good.

>  }
>  
>  static void exec_accept_incoming_migration(void *opaque)
> diff --git a/migration-fd.c b/migration-fd.c
> index 7335167..938a109 100644
> --- a/migration-fd.c
> +++ b/migration-fd.c
> @@ -73,12 +73,11 @@ static int fd_close(MigrationState *s)
>      return 0;
>  }
>  
> -int fd_start_outgoing_migration(MigrationState *s, const char *fdname)
> +void fd_start_outgoing_migration(MigrationState *s, const char *fdname, Error **errp)
>  {
> -    s->fd = monitor_get_fd(cur_mon, fdname, NULL);
> +    s->fd = monitor_get_fd(cur_mon, fdname, errp);
>      if (s->fd == -1) {
> -        DPRINTF("fd_migration: invalid file descriptor identifier\n");
> -        goto err_after_get_fd;
> +        return;
>      }
>  
>      if (fcntl(s->fd, F_SETFL, O_NONBLOCK) == -1) {

You should set errp here too.

> @@ -91,12 +90,10 @@ int fd_start_outgoing_migration(MigrationState *s, const char *fdname)
>      s->close = fd_close;
>  
>      migrate_fd_connect(s);
> -    return 0;
> +    return;
>  
>  err_after_open:
>      close(s->fd);
> -err_after_get_fd:
> -    return -1;
>  }
>  
>  static void fd_accept_incoming_migration(void *opaque)
> diff --git a/migration-tcp.c b/migration-tcp.c
> index e8bc76a..5e54e68 100644
> --- a/migration-tcp.c
> +++ b/migration-tcp.c
> @@ -68,22 +68,13 @@ static void tcp_wait_for_connect(int fd, void *opaque)
>      }
>  }
>  
> -int tcp_start_outgoing_migration(MigrationState *s, const char *host_port,
> -                                 Error **errp)
> +void tcp_start_outgoing_migration(MigrationState *s, const char *host_port, Error **errp)
>  {
> -    Error *local_err = NULL;
> -
>      s->get_error = socket_errno;
>      s->write = socket_write;
>      s->close = tcp_close;
>  
> -    s->fd = inet_nonblocking_connect(host_port, tcp_wait_for_connect, s, &local_err);
> -    if (local_err != NULL) {
> -        error_propagate(errp, local_err);
> -        return -1;
> -    }
> -
> -    return 0;
> +    s->fd = inet_nonblocking_connect(host_port, tcp_wait_for_connect, s, errp);
>  }
>  
>  static void tcp_accept_incoming_migration(void *opaque)
> diff --git a/migration-unix.c b/migration-unix.c
> index 5387c21..34a413a 100644
> --- a/migration-unix.c
> +++ b/migration-unix.c
> @@ -68,20 +68,13 @@ static void unix_wait_for_connect(int fd, void *opaque)
>      }
>  }
>  
> -int unix_start_outgoing_migration(MigrationState *s, const char *path, Error **errp)
> +void unix_start_outgoing_migration(MigrationState *s, const char *path, Error **errp)
>  {
> -    Error *local_err = NULL;
> -
>      s->get_error = unix_errno;
>      s->write = unix_write;
>      s->close = unix_close;
>  
> -    s->fd = unix_nonblocking_connect(path, unix_wait_for_connect, s, &local_err);
> -    if (local_err != NULL) {
> -        error_propagate(errp, local_err);
> -        return -1;
> -    }
> -    return 0;
> +    s->fd = unix_nonblocking_connect(path, unix_wait_for_connect, s, errp);
>  }
>  
>  static void unix_accept_incoming_migration(void *opaque)
> diff --git a/migration.c b/migration.c
> index 767e297..f7f0138 100644
> --- a/migration.c
> +++ b/migration.c
> @@ -485,7 +485,6 @@ void qmp_migrate(const char *uri, bool has_blk, bool blk,
>      MigrationState *s = migrate_get_current();
>      MigrationParams params;
>      const char *p;
> -    int ret;
>  
>      params.blk = blk;
>      params.shared = inc;
> @@ -507,27 +506,23 @@ void qmp_migrate(const char *uri, bool has_blk, bool blk,
>      s = migrate_init(&params);
>  
>      if (strstart(uri, "tcp:", &p)) {
> -        ret = tcp_start_outgoing_migration(s, p, &local_err);
> +        tcp_start_outgoing_migration(s, p, &local_err);
>  #if !defined(WIN32)
>      } else if (strstart(uri, "exec:", &p)) {
> -        ret = exec_start_outgoing_migration(s, p);
> +        exec_start_outgoing_migration(s, p, &local_err);
>      } else if (strstart(uri, "unix:", &p)) {
> -        ret = unix_start_outgoing_migration(s, p, &local_err);
> +        unix_start_outgoing_migration(s, p, &local_err);
>      } else if (strstart(uri, "fd:", &p)) {
> -        ret = fd_start_outgoing_migration(s, p);
> +        fd_start_outgoing_migration(s, p, &local_err);
>  #endif
>      } else {
>          error_set(errp, QERR_INVALID_PARAMETER_VALUE, "uri", "a valid migration protocol");
>          return;
>      }
>  
> -    if (ret < 0 || local_err) {
> +    if (local_err) {
>          migrate_fd_error(s);
> -        if (!local_err) {
> -            error_set_errno(errp, -ret, QERR_UNDEFINED_ERROR);
> -        } else {
> -            error_propagate(errp, local_err);
> -        }
> +        error_propagate(errp, local_err);
>          return;
>      }
>  
> diff --git a/migration.h b/migration.h
> index e0612a3..275d2d8 100644
> --- a/migration.h
> +++ b/migration.h
> @@ -56,20 +56,19 @@ void do_info_migrate(Monitor *mon, QObject **ret_data);
>  
>  int exec_start_incoming_migration(const char *host_port);
>  
> -int exec_start_outgoing_migration(MigrationState *s, const char *host_port);
> +void exec_start_outgoing_migration(MigrationState *s, const char *host_port, Error **errp);
>  
>  int tcp_start_incoming_migration(const char *host_port, Error **errp);
>  
> -int tcp_start_outgoing_migration(MigrationState *s, const char *host_port,
> -                                 Error **errp);
> +void tcp_start_outgoing_migration(MigrationState *s, const char *host_port, Error **errp);
>  
>  int unix_start_incoming_migration(const char *path, Error **errp);
>  
> -int unix_start_outgoing_migration(MigrationState *s, const char *path, Error **errp);
> +void unix_start_outgoing_migration(MigrationState *s, const char *path, Error **errp);
>  
>  int fd_start_incoming_migration(const char *path);
>  
> -int fd_start_outgoing_migration(MigrationState *s, const char *fdname);
> +void fd_start_outgoing_migration(MigrationState *s, const char *fdname, Error **errp);
>  
>  void migrate_fd_error(MigrationState *s);
>