[Top][All Lists]
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH 08/18] migration (outgoing): add error propagati
From: |
Luiz Capitulino |
Subject: |
Re: [Qemu-devel] [PATCH 08/18] migration (outgoing): add error propagation for fd and exec protocols |
Date: |
Thu, 4 Oct 2012 15:24:35 -0300 |
On Wed, 3 Oct 2012 16:36:55 +0200
Paolo Bonzini <address@hidden> 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 <address@hidden>
> ---
> 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(¶ms);
>
> 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);
>
- Re: [Qemu-devel] [PATCH 05/18] migration: avoid using error_is_set, (continued)
- [Qemu-devel] [PATCH 04/18] qemu-sockets: add nonblocking connect for Unix sockets, Paolo Bonzini, 2012/10/03
- [Qemu-devel] [PATCH 06/18] migration: centralize call to migrate_fd_error(), Paolo Bonzini, 2012/10/03
- [Qemu-devel] [PATCH 07/18] migration: use qemu-sockets to establish Unix sockets, Paolo Bonzini, 2012/10/03
- [Qemu-devel] [PATCH 09/18] migration (incoming): add error propagation for fd and exec protocols, Paolo Bonzini, 2012/10/03
- [Qemu-devel] [PATCH 08/18] migration (outgoing): add error propagation for fd and exec protocols, Paolo Bonzini, 2012/10/03
- Re: [Qemu-devel] [PATCH 08/18] migration (outgoing): add error propagation for fd and exec protocols,
Luiz Capitulino <=
[Qemu-devel] [PATCH 11/18] nbd: ask and print error information from qemu-sockets, Paolo Bonzini, 2012/10/03
[Qemu-devel] [PATCH 12/18] qemu-ga: ask and print error information from qemu-sockets, Paolo Bonzini, 2012/10/03
[Qemu-devel] [PATCH 10/18] qemu-char: ask and print error information from qemu-sockets, Paolo Bonzini, 2012/10/03