| Message ID | 20170425101758.3944-3-quintela@redhat.com |
|---|---|
| State | New |
| Headers | show |
On 25/04/2017 12:17, Juan Quintela wrote: > We have just arrived as: > > migration.c: qemu_migrate() > .... > s = migrate_init() <- puts it to NULL > .... > {tcp,unix}_start_outgoing_migration -> > socket_outgoing_migration > migration_channel_connect() > sets to_dst_file > > if tls is enabled, we do another round through > migrate_channel_tls_connect(), but we only set it up if there is no > error. So we don't need the assignation. I am removing it to remove > in the follwing patches the knowledge about MigrationState in that two > files. > > Signed-off-by: Juan Quintela <quintela@redhat.com> > --- > migration/socket.c | 1 - > migration/tls.c | 1 - > 2 files changed, 2 deletions(-) > > diff --git a/migration/socket.c b/migration/socket.c > index 13966f1..dc88812 100644 > --- a/migration/socket.c > +++ b/migration/socket.c > @@ -79,7 +79,6 @@ static void socket_outgoing_migration(QIOTask *task, > > if (qio_task_propagate_error(task, &err)) { > trace_migration_socket_outgoing_error(error_get_pretty(err)); > - data->s->to_dst_file = NULL; > migrate_fd_error(data->s, err); > error_free(err); > } else { > diff --git a/migration/tls.c b/migration/tls.c > index 45bec44..a33ecb7 100644 > --- a/migration/tls.c > +++ b/migration/tls.c > @@ -116,7 +116,6 @@ static void migration_tls_outgoing_handshake(QIOTask *task, > > if (qio_task_propagate_error(task, &err)) { > trace_migration_tls_outgoing_handshake_error(error_get_pretty(err)); > - s->to_dst_file = NULL; > migrate_fd_error(s, err); > error_free(err); > } else { > In migrate_fd_error(), we have "assert(s->to_dst_file == NULL);", so you break the function with this change. Laurent
Laurent Vivier <lvivier@redhat.com> wrote: > On 25/04/2017 12:17, Juan Quintela wrote: >> We have just arrived as: >> >> migration.c: qemu_migrate() >> .... >> s = migrate_init() <- puts it to NULL >> .... >> {tcp,unix}_start_outgoing_migration -> >> socket_outgoing_migration >> migration_channel_connect() >> sets to_dst_file >> >> if tls is enabled, we do another round through >> migrate_channel_tls_connect(), but we only set it up if there is no >> error. So we don't need the assignation. I am removing it to remove >> in the follwing patches the knowledge about MigrationState in that two >> files. >> >> Signed-off-by: Juan Quintela <quintela@redhat.com> >> --- >> migration/socket.c | 1 - >> migration/tls.c | 1 - >> 2 files changed, 2 deletions(-) >> >> diff --git a/migration/socket.c b/migration/socket.c >> index 13966f1..dc88812 100644 >> --- a/migration/socket.c >> +++ b/migration/socket.c >> @@ -79,7 +79,6 @@ static void socket_outgoing_migration(QIOTask *task, >> >> if (qio_task_propagate_error(task, &err)) { >> trace_migration_socket_outgoing_error(error_get_pretty(err)); >> - data->s->to_dst_file = NULL; >> migrate_fd_error(data->s, err); >> error_free(err); >> } else { >> diff --git a/migration/tls.c b/migration/tls.c >> index 45bec44..a33ecb7 100644 >> --- a/migration/tls.c >> +++ b/migration/tls.c >> @@ -116,7 +116,6 @@ static void migration_tls_outgoing_handshake(QIOTask *task, >> >> if (qio_task_propagate_error(task, &err)) { >> trace_migration_tls_outgoing_handshake_error(error_get_pretty(err)); >> - s->to_dst_file = NULL; >> migrate_fd_error(s, err); >> error_free(err); >> } else { >> > > In migrate_fd_error(), we have "assert(s->to_dst_file == NULL);", so you > break the function with this change. I missundertood something, or everyway to arrive here, to_dst_file is always NULL. See the comment of the file, the only distintion if we go through tls is that we do "yet" another round through the init functions, but it is still NULL. At least I can't see how this can be NULL at this point. Forget about tls for now, centrate in the normal socket.c case: we are inside socket_outgoing_migration() nothing here touch any componetes of data so go to the caller: socket_start_outgoing_migration(). It init all data values to NULL/0 (g_new0). we call qio_channel_set_name() -> no "data" here. qio_channel_socket_connect_async() It touches "neworking" things here, but nothing related to data, the first function that we call when we receive a connection is socket_outgoing_migration(), so we are good. Back to the tls case: migration_tls_outgoing_handshake () -> nothing touch "s" until we call migrate_fd_error(). what calls migration_tls_outgoing_handshake? migration_tls_outgoing_connect(). But that only calls it using qio_channel_tls_handshake(). Notice that they pass us here MigrationState, so, who calls this function? migration_channel_connect(), this is the function that calls tls, and tls ends calling this function without the tls stuff. So, at the point when we setup s->to_dst_file = f here, it is the 1st time that we have set that field, too late to affect the migrate_fd_error() that we were talking about, no? Later, Juan.
On 25/04/2017 14:59, Juan Quintela wrote: > Laurent Vivier <lvivier@redhat.com> wrote: >> On 25/04/2017 12:17, Juan Quintela wrote: >>> We have just arrived as: >>> >>> migration.c: qemu_migrate() >>> .... >>> s = migrate_init() <- puts it to NULL >>> .... >>> {tcp,unix}_start_outgoing_migration -> >>> socket_outgoing_migration >>> migration_channel_connect() >>> sets to_dst_file >>> >>> if tls is enabled, we do another round through >>> migrate_channel_tls_connect(), but we only set it up if there is no >>> error. So we don't need the assignation. I am removing it to remove >>> in the follwing patches the knowledge about MigrationState in that two >>> files. >>> >>> Signed-off-by: Juan Quintela <quintela@redhat.com> >>> --- >>> migration/socket.c | 1 - >>> migration/tls.c | 1 - >>> 2 files changed, 2 deletions(-) >>> >>> diff --git a/migration/socket.c b/migration/socket.c >>> index 13966f1..dc88812 100644 >>> --- a/migration/socket.c >>> +++ b/migration/socket.c >>> @@ -79,7 +79,6 @@ static void socket_outgoing_migration(QIOTask *task, >>> >>> if (qio_task_propagate_error(task, &err)) { >>> trace_migration_socket_outgoing_error(error_get_pretty(err)); >>> - data->s->to_dst_file = NULL; >>> migrate_fd_error(data->s, err); >>> error_free(err); >>> } else { >>> diff --git a/migration/tls.c b/migration/tls.c >>> index 45bec44..a33ecb7 100644 >>> --- a/migration/tls.c >>> +++ b/migration/tls.c >>> @@ -116,7 +116,6 @@ static void migration_tls_outgoing_handshake(QIOTask *task, >>> >>> if (qio_task_propagate_error(task, &err)) { >>> trace_migration_tls_outgoing_handshake_error(error_get_pretty(err)); >>> - s->to_dst_file = NULL; >>> migrate_fd_error(s, err); >>> error_free(err); >>> } else { >>> >> >> In migrate_fd_error(), we have "assert(s->to_dst_file == NULL);", so you >> break the function with this change. > > I missundertood something, or everyway to arrive here, to_dst_file is > always NULL. See the comment of the file, the only distintion if we go > through tls is that we do "yet" another round through the init > functions, but it is still NULL. At least I can't see how this can be > NULL at this point. > > Forget about tls for now, centrate in the normal socket.c case: > > we are inside socket_outgoing_migration() > nothing here touch any componetes of data > > so go to the caller: > > socket_start_outgoing_migration(). > > It init all data values to NULL/0 (g_new0). > > we call qio_channel_set_name() -> no "data" here. > qio_channel_socket_connect_async() > > It touches "neworking" things here, but nothing related to data, the > first function that we call when we receive a connection is > socket_outgoing_migration(), so we are good. > > > Back to the tls case: > > migration_tls_outgoing_handshake () -> nothing touch "s" until we call > migrate_fd_error(). > > what calls migration_tls_outgoing_handshake? > migration_tls_outgoing_connect(). > > But that only calls it using qio_channel_tls_handshake(). Notice that > they pass us here MigrationState, so, who calls this function? > > migration_channel_connect(), this is the function that calls tls, and > tls ends calling this function without the tls stuff. So, at the point > when we setup s->to_dst_file = f here, it is the 1st time that we have > set that field, too late to affect the migrate_fd_error() that we were > talking about, no? Yes, you're right... I should have read your message more carefully... Reviewed-by: Laurent Vivier <lvivier@redhat.com> Thanks, Laurent
On Tue, Apr 25, 2017 at 12:17:58PM +0200, Juan Quintela wrote: > We have just arrived as: > > migration.c: qemu_migrate() > .... > s = migrate_init() <- puts it to NULL > .... > {tcp,unix}_start_outgoing_migration -> > socket_outgoing_migration > migration_channel_connect() > sets to_dst_file > > if tls is enabled, we do another round through > migrate_channel_tls_connect(), but we only set it up if there is no > error. So we don't need the assignation. I am removing it to remove > in the follwing patches the knowledge about MigrationState in that two > files. Yes, the logic is quite subtle, but your analysis looks correct. > > Signed-off-by: Juan Quintela <quintela@redhat.com> Reviewed-by: Daniel P. Berrange <berrange@redhat.com> > --- > migration/socket.c | 1 - > migration/tls.c | 1 - > 2 files changed, 2 deletions(-) > > diff --git a/migration/socket.c b/migration/socket.c > index 13966f1..dc88812 100644 > --- a/migration/socket.c > +++ b/migration/socket.c > @@ -79,7 +79,6 @@ static void socket_outgoing_migration(QIOTask *task, > > if (qio_task_propagate_error(task, &err)) { > trace_migration_socket_outgoing_error(error_get_pretty(err)); > - data->s->to_dst_file = NULL; > migrate_fd_error(data->s, err); > error_free(err); > } else { > diff --git a/migration/tls.c b/migration/tls.c > index 45bec44..a33ecb7 100644 > --- a/migration/tls.c > +++ b/migration/tls.c > @@ -116,7 +116,6 @@ static void migration_tls_outgoing_handshake(QIOTask *task, > > if (qio_task_propagate_error(task, &err)) { > trace_migration_tls_outgoing_handshake_error(error_get_pretty(err)); > - s->to_dst_file = NULL; > migrate_fd_error(s, err); > error_free(err); > } else { Regards, Daniel
On Tue, Apr 25, 2017 at 12:17:58PM +0200, Juan Quintela wrote: > We have just arrived as: > > migration.c: qemu_migrate() > .... > s = migrate_init() <- puts it to NULL > .... > {tcp,unix}_start_outgoing_migration -> > socket_outgoing_migration > migration_channel_connect() > sets to_dst_file > > if tls is enabled, we do another round through > migrate_channel_tls_connect(), but we only set it up if there is no > error. So we don't need the assignation. I am removing it to remove > in the follwing patches the knowledge about MigrationState in that two > files. > > Signed-off-by: Juan Quintela <quintela@redhat.com> Reviewed-by: Peter Xu <peterx@redhat.com> > --- > migration/socket.c | 1 - > migration/tls.c | 1 - > 2 files changed, 2 deletions(-) > > diff --git a/migration/socket.c b/migration/socket.c > index 13966f1..dc88812 100644 > --- a/migration/socket.c > +++ b/migration/socket.c > @@ -79,7 +79,6 @@ static void socket_outgoing_migration(QIOTask *task, > > if (qio_task_propagate_error(task, &err)) { > trace_migration_socket_outgoing_error(error_get_pretty(err)); > - data->s->to_dst_file = NULL; > migrate_fd_error(data->s, err); > error_free(err); > } else { > diff --git a/migration/tls.c b/migration/tls.c > index 45bec44..a33ecb7 100644 > --- a/migration/tls.c > +++ b/migration/tls.c > @@ -116,7 +116,6 @@ static void migration_tls_outgoing_handshake(QIOTask *task, > > if (qio_task_propagate_error(task, &err)) { > trace_migration_tls_outgoing_handshake_error(error_get_pretty(err)); > - s->to_dst_file = NULL; > migrate_fd_error(s, err); > error_free(err); > } else { > -- > 2.9.3 >
diff --git a/migration/socket.c b/migration/socket.c index 13966f1..dc88812 100644 --- a/migration/socket.c +++ b/migration/socket.c @@ -79,7 +79,6 @@ static void socket_outgoing_migration(QIOTask *task, if (qio_task_propagate_error(task, &err)) { trace_migration_socket_outgoing_error(error_get_pretty(err)); - data->s->to_dst_file = NULL; migrate_fd_error(data->s, err); error_free(err); } else { diff --git a/migration/tls.c b/migration/tls.c index 45bec44..a33ecb7 100644 --- a/migration/tls.c +++ b/migration/tls.c @@ -116,7 +116,6 @@ static void migration_tls_outgoing_handshake(QIOTask *task, if (qio_task_propagate_error(task, &err)) { trace_migration_tls_outgoing_handshake_error(error_get_pretty(err)); - s->to_dst_file = NULL; migrate_fd_error(s, err); error_free(err); } else {
We have just arrived as: migration.c: qemu_migrate() .... s = migrate_init() <- puts it to NULL .... {tcp,unix}_start_outgoing_migration -> socket_outgoing_migration migration_channel_connect() sets to_dst_file if tls is enabled, we do another round through migrate_channel_tls_connect(), but we only set it up if there is no error. So we don't need the assignation. I am removing it to remove in the follwing patches the knowledge about MigrationState in that two files. Signed-off-by: Juan Quintela <quintela@redhat.com> --- migration/socket.c | 1 - migration/tls.c | 1 - 2 files changed, 2 deletions(-)