Re: [Qemu-devel] [PATCH 08/25] migration (outgoing): add error propagation for all protocols

[Markus Armbruster]

Paolo Bonzini <address@hidden> writes:

> Error propagation is already there for socket backends, even though
> it is (and remains) incomplete because no Error is passed to the
> NonBlockingConnectHandler.

Why is that a problem?

>                             Add it to other protocols, simplifying
> code that tests for errors that will never happen.
>
> With all protocols understanding Error, the code can be simplified
> further 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
>
> Signed-off-by: Paolo Bonzini <address@hidden>
> ---
>         v1->v2: turn bizarre DPRINTF into an assertion failure or just
>         drop it for the failure test of O_NONBLOCK. Clean up after this
>         change.
>
>  migration-fd.c   | 19 ++++---------------
>  migration-tcp.c  | 13 ++-----------
>  migration-unix.c | 11 ++---------
>  migration.c      | 17 ++++++-----------
>  migration.h      |  9 ++++-----
>  6 file modificati, 22 inserzioni(+), 65 rimozioni(-)
>
> diff --git a/migration-exec.c b/migration-exec.c
> index 6c97db9..5f3f4b2 100644
> --- a/migration-exec.c
> +++ b/migration-exec.c
> @@ -60,22 +60,18 @@ 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");
> -        goto err_after_popen;
> +        error_setg_errno(errp, errno, "failed to popen the migration 
> target");
> +        return;
>      }
>  
>      s->fd = fileno(f);
> -    if (s->fd == -1) {
> -        DPRINTF("Unable to retrieve file descriptor for popen'd handle\n");
> -        goto err_after_open;
> -    }
> -
> +    assert(s->fd != -1);

Okay, because fileno() can fail only if its argument is not a valid
stream, which really shouldn't be possible here.

>      socket_set_nonblock(s->fd);
>  
>      s->opaque = qemu_popen(f, "w");
> @@ -85,12 +81,6 @@ int exec_start_outgoing_migration(MigrationState *s, const 
> char *command)
>      s->write = file_write;
>  
>      migrate_fd_connect(s);
> -    return 0;
> -
> -err_after_open:
> -    pclose(f);
> -err_after_popen:
> -    return -1;
>  }
>  
>  static void exec_accept_incoming_migration(void *opaque)
> diff --git a/migration-fd.c b/migration-fd.c
> index 7335167..a7c800a 100644
> --- a/migration-fd.c
> +++ b/migration-fd.c
> @@ -73,30 +73,19 @@ 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;
> -    }
> -
> -    if (fcntl(s->fd, F_SETFL, O_NONBLOCK) == -1) {
> -        DPRINTF("Unable to set nonblocking mode on file descriptor\n");
> -        goto err_after_open;
> +        return;
>      }
>  
> +    fcntl(s->fd, F_SETFL, O_NONBLOCK);

Sure this can't fail?

>      s->get_error = fd_errno;
>      s->write = fd_write;
>      s->close = fd_close;
>  
>      migrate_fd_connect(s);
> -    return 0;
> -
> -err_after_open:
> -    close(s->fd);
> -err_after_get_fd:
> -    return -1;
>  }
>  
>  static void fd_accept_incoming_migration(void *opaque)
[...]