| Message ID | 20170906115143.27451-8-quintela@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | Multifd | expand |
On 09/06/2017 06:51 AM, Juan Quintela wrote: > So far, we had to free the error after each caller, so just do it > here. Once there, tls.c was leaking the error. You mention tls.c, > > Signed-off-by: Juan Quintela <quintela@redhat.com> > --- > migration/channel.c | 1 - > migration/migration.c | 10 ++++------ > migration/migration.h | 4 ++-- > migration/socket.c | 1 - > 4 files changed, 6 insertions(+), 10 deletions(-) but don't touch it. Am I missing something? > > -void migrate_fd_error(MigrationState *s, const Error *error) > +void migrate_fd_error(MigrationState *s, Error *error) > { No comments at definition, > +++ b/migration/migration.h > @@ -163,8 +163,8 @@ bool migration_has_all_channels(void); > > uint64_t migrate_max_downtime(void); > > -void migrate_set_error(MigrationState *s, const Error *error); > -void migrate_fd_error(MigrationState *s, const Error *error); > +void migrate_set_error(MigrationState *s, Error *error); > +void migrate_fd_error(MigrationState *s, Error *error); or at declaration. That would be worth adding at some point, but this patch isn't making it worse. The code looks okay in isolation, so if it is only the commit message that needs fixing, Reviewed-by: Eric Blake <eblake@redhat.com>
Eric Blake <eblake@redhat.com> wrote: D> On 09/06/2017 06:51 AM, Juan Quintela wrote: >> So far, we had to free the error after each caller, so just do it >> here. Once there, tls.c was leaking the error. > > You mention tls.c, > >> >> Signed-off-by: Juan Quintela <quintela@redhat.com> >> --- >> migration/channel.c | 1 - >> migration/migration.c | 10 ++++------ >> migration/migration.h | 4 ++-- >> migration/socket.c | 1 - >> 4 files changed, 6 insertions(+), 10 deletions(-) > > but don't touch it. Am I missing something? > It was missing error_free(); So it leaked the Error * variable. I will improve the message for next version. >> >> -void migrate_fd_error(MigrationState *s, const Error *error) >> +void migrate_fd_error(MigrationState *s, Error *error) >> { > > No comments at definition, We free it inside now, so it can't be const. >> +++ b/migration/migration.h >> @@ -163,8 +163,8 @@ bool migration_has_all_channels(void); >> >> uint64_t migrate_max_downtime(void); >> >> -void migrate_set_error(MigrationState *s, const Error *error); >> -void migrate_fd_error(MigrationState *s, const Error *error); >> +void migrate_set_error(MigrationState *s, Error *error); >> +void migrate_fd_error(MigrationState *s, Error *error); > > or at declaration. That would be worth adding at some point, but this > patch isn't making it worse. will add them, thanks. > The code looks okay in isolation, so if it is only the commit message > that needs fixing, > Reviewed-by: Eric Blake <eblake@redhat.com> Thanks.
* Eric Blake (eblake@redhat.com) wrote: > On 09/06/2017 06:51 AM, Juan Quintela wrote: > > So far, we had to free the error after each caller, so just do it > > here. Once there, tls.c was leaking the error. > > You mention tls.c, > > > > > Signed-off-by: Juan Quintela <quintela@redhat.com> > > --- > > migration/channel.c | 1 - > > migration/migration.c | 10 ++++------ > > migration/migration.h | 4 ++-- > > migration/socket.c | 1 - > > 4 files changed, 6 insertions(+), 10 deletions(-) > > but don't touch it. Am I missing something? hmm well I see in migration/tls.c: if (qio_task_propagate_error(task, &err)) { trace_migration_tls_outgoing_handshake_error(error_get_pretty(err)); migrate_fd_error(s, err); error_free(err); } else { trace_migration_tls_outgoing_handshake_complete(); migration_channel_connect(s, ioc, NULL); } so I think that error_free has to go? Dave > > > > -void migrate_fd_error(MigrationState *s, const Error *error) > > +void migrate_fd_error(MigrationState *s, Error *error) > > { > > No comments at definition, > > > +++ b/migration/migration.h > > @@ -163,8 +163,8 @@ bool migration_has_all_channels(void); > > > > uint64_t migrate_max_downtime(void); > > > > -void migrate_set_error(MigrationState *s, const Error *error); > > -void migrate_fd_error(MigrationState *s, const Error *error); > > +void migrate_set_error(MigrationState *s, Error *error); > > +void migrate_fd_error(MigrationState *s, Error *error); > > or at declaration. That would be worth adding at some point, but this > patch isn't making it worse. > > The code looks okay in isolation, so if it is only the commit message > that needs fixing, > Reviewed-by: Eric Blake <eblake@redhat.com> > > -- > Eric Blake, Principal Software Engineer > Red Hat, Inc. +1-919-301-3266 > Virtualization: qemu.org | libvirt.org > -- Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
diff --git a/migration/channel.c b/migration/channel.c index 70ec7ea3b7..1dd2ae1530 100644 --- a/migration/channel.c +++ b/migration/channel.c @@ -71,7 +71,6 @@ void migration_channel_connect(MigrationState *s, migration_tls_channel_connect(s, ioc, hostname, &local_err); if (local_err) { migrate_fd_error(s, local_err); - error_free(local_err); } } else { QEMUFile *f = qemu_fopen_channel_output(ioc); diff --git a/migration/migration.c b/migration/migration.c index 5e17168c1c..6191551e5a 100644 --- a/migration/migration.c +++ b/migration/migration.c @@ -1021,16 +1021,14 @@ static void migrate_fd_cleanup(void *opaque) block_cleanup_parameters(s); } -void migrate_set_error(MigrationState *s, const Error *error) +void migrate_set_error(MigrationState *s, Error *error) { qemu_mutex_lock(&s->error_mutex); - if (!s->error) { - s->error = error_copy(error); - } + error_propagate(&s->error, error); qemu_mutex_unlock(&s->error_mutex); } -void migrate_fd_error(MigrationState *s, const Error *error) +void migrate_fd_error(MigrationState *s, Error *error) { trace_migrate_fd_error(error_get_pretty(error)); assert(s->to_dst_file == NULL); @@ -1304,7 +1302,7 @@ void qmp_migrate(const char *uri, bool has_blk, bool blk, } if (local_err) { - migrate_fd_error(s, local_err); + migrate_fd_error(s, error_copy(local_err)); error_propagate(errp, local_err); return; } diff --git a/migration/migration.h b/migration/migration.h index 9a81b8a70a..7ddb0cdc1a 100644 --- a/migration/migration.h +++ b/migration/migration.h @@ -163,8 +163,8 @@ bool migration_has_all_channels(void); uint64_t migrate_max_downtime(void); -void migrate_set_error(MigrationState *s, const Error *error); -void migrate_fd_error(MigrationState *s, const Error *error); +void migrate_set_error(MigrationState *s, Error *error); +void migrate_fd_error(MigrationState *s, Error *error); void migrate_fd_connect(MigrationState *s); diff --git a/migration/socket.c b/migration/socket.c index dee869044a..2d70747a1a 100644 --- a/migration/socket.c +++ b/migration/socket.c @@ -80,7 +80,6 @@ static void socket_outgoing_migration(QIOTask *task, if (qio_task_propagate_error(task, &err)) { trace_migration_socket_outgoing_error(error_get_pretty(err)); migrate_fd_error(data->s, err); - error_free(err); } else { trace_migration_socket_outgoing_connected(data->hostname); migration_channel_connect(data->s, sioc, data->hostname);
So far, we had to free the error after each caller, so just do it here. Once there, tls.c was leaking the error. Signed-off-by: Juan Quintela <quintela@redhat.com> --- migration/channel.c | 1 - migration/migration.c | 10 ++++------ migration/migration.h | 4 ++-- migration/socket.c | 1 - 4 files changed, 6 insertions(+), 10 deletions(-)