Re: [Qemu-devel] [PATCH 2/2] migration: to_dst_file at that point is NULL

[Juan Quintela <quintela@redhat.com>]

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.