On Wed, 3 Oct 2012 16:36:55 +0200 Paolo Bonzini <pbonz...@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 <pbonz...@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(¶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); >