| Message ID | 1349275025-5093-9-git-send-email-pbonzini@redhat.com |
|---|---|
| State | New |
| Headers | show |
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(¶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); >
Il 04/10/2012 20:24, Luiz Capitulino ha scritto: > 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. This one is not dropped, it becomes the reported error message. >> > goto err_after_popen; >> > } >> > >> > s->fd = fileno(f); >> > if (s->fd == -1) { >> > - DPRINTF("Unable to retrieve file descriptor for popen'd handle\n"); This one is dropped, but I wanted to delete the check altogether. fileno() should only fail if it detects somehow that its argument is not a valid stream, which is obviously not the case. Would that be better? It would also fix the clobbering of errno. >> > 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. Paolo
On Fri, 05 Oct 2012 08:25:46 +0200 Paolo Bonzini <pbonzini@redhat.com> wrote: > Il 04/10/2012 20:24, Luiz Capitulino ha scritto: > > 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. > > This one is not dropped, it becomes the reported error message. What I meant is that the error/debug message won't be printed the same way it was before. This is an improvement, but it's a good idea to mention it. > >> > goto err_after_popen; > >> > } > >> > > >> > s->fd = fileno(f); > >> > if (s->fd == -1) { > >> > - DPRINTF("Unable to retrieve file descriptor for popen'd handle\n"); > > This one is dropped, but I wanted to delete the check altogether. > fileno() should only fail if it detects somehow that its argument is not > a valid stream, which is obviously not the case. > > Would that be better? It would also fix the clobbering of errno. I guess so. Is it possible for popen() to return success but then set the FILE's fd to -1? Maybe change to an assert() instead?
Il 05/10/2012 14:41, Luiz Capitulino ha scritto: >>> > > 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. >> > >> > This one is not dropped, it becomes the reported error message. > What I meant is that the error/debug message won't be printed the same way > it was before. This is an improvement, but it's a good idea to mention it. > I mentioned it for fd, but the popen() function only returns NULL if the fork(2) or pipe(2) calls fail, or if it cannot allocate memory, so I have no idea how to trigger it. >> It would also fix the clobbering of errno. > > I guess so. Is it possible for popen() to return success but then set the > FILE's fd to -1? No. I will add the assert(). Paolo
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"); 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"); } 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) { @@ -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);
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 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(-)