Re: [PATCH v2 05/10] migration: Make multifd_load_setup() get an Error parameter

[Juan Quintela]

"Dr. David Alan Gilbert" <address@hidden> wrote:
> * Juan Quintela (address@hidden) wrote:
>> We need to change the full chain to pass the Error parameter.
>> 
>> Signed-off-by: Juan Quintela <address@hidden>
>> ---
>>  migration/migration.c | 10 +++++-----
>>  migration/migration.h |  2 +-
>>  migration/ram.c       |  2 +-
>>  migration/ram.h       |  2 +-
>>  migration/rdma.c      |  2 +-
>>  5 files changed, 9 insertions(+), 9 deletions(-)
>> 
>> diff --git a/migration/migration.c b/migration/migration.c
>> index 5a56bd0c91..cf6cec5fb6 100644
>> --- a/migration/migration.c
>> +++ b/migration/migration.c
>> @@ -518,11 +518,11 @@ fail:
>>      exit(EXIT_FAILURE);
>>  }
>>  
>> -static void migration_incoming_setup(QEMUFile *f)
>> +static void migration_incoming_setup(QEMUFile *f, Error **errp)
>>  {
>>      MigrationIncomingState *mis = migration_incoming_get_current();
>>  
>> -    if (multifd_load_setup() != 0) {
>> +    if (multifd_load_setup(errp) != 0) {
>>          /* We haven't been able to create multifd threads
>>             nothing better to do */
>
> But if you're taking an errp and the load fails, don't you want to
> report the error before you exit? (with an error_get_pretty or
> something?)

error_report_err() that is.

>
>>          exit(EXIT_FAILURE);
>> @@ -572,13 +572,13 @@ static bool postcopy_try_recover(QEMUFile *f)
>>      return false;
>>  }
>>  
>> -void migration_fd_process_incoming(QEMUFile *f)
>> +void migration_fd_process_incoming(QEMUFile *f, Error **errp)
>>  {
>>      if (postcopy_try_recover(f)) {
>>          return;
>>      }
>>  
>> -    migration_incoming_setup(f);
>> +    migration_incoming_setup(f, errp);
>>      migration_incoming_process();
>
> and if you're making incoming_setup able to fail, don't you need
> to.... hmm, skip the incoming_process?

Changing it to test for errors, thanks.

>>  }
>>  
>> @@ -596,7 +596,7 @@ void migration_ioc_process_incoming(QIOChannel *ioc, 
>> Error **errp)
>>              return;
>>          }
>>  
>> -        migration_incoming_setup(f);
>> +        migration_incoming_setup(f, errp);
>
> Don't you need to make that use a local_err and propagate, like in the
> other half of the if/else?

Changed the whole business.  It appears that each time that I use an
Error ** I have to relearn how to use it.  Changed all places to use a
local error and propagate/error_report_err() as apropiate.

>>      rdma->migration_started_on_destination = 1;
>> -    migration_fd_process_incoming(f);
>> +    migration_fd_process_incoming(f, errp);
>
> Heck, the errp handling in rdma_accept_incoming_migration is very very
> broken; I don't see how the errp ever gets reported or freed.
> (But that's an existing problem)

Leaving this for next series.

Thanks, Juan.