On 25/04/2017 14:59, Juan Quintela wrote:
> Laurent Vivier <lviv...@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 <quint...@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 <lviv...@redhat.com>
Thanks,
Laurent
