Re: [PATCH for-9.1 v5 08/14] migration: Add Error** argument to .load_setup() handler

["Cédric Le Goater" <clg@redhat.com>]

On 3/20/24 09:02, Markus Armbruster wrote:
> Cédric Le Goater <clg@redhat.com> writes:
> 
>> This will be useful to report errors at a higher level, mostly in VFIO
>> today.
>>
>> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>> Reviewed-by: Peter Xu <peterx@redhat.com>
>> Signed-off-by: Cédric Le Goater <clg@redhat.com>
>> ---
> 
> [...]
> 
>> diff --git a/migration/savevm.c b/migration/savevm.c
>> index 535ad5a32d67057dd172ce25d561a66a07172e97..8f42999a15d1685957de9ed517d6bc9ba49c3f11 100644
>> --- a/migration/savevm.c
>> +++ b/migration/savevm.c
>> @@ -2747,8 +2747,9 @@ static void qemu_loadvm_state_switchover_ack_needed(MigrationIncomingState *mis)
>>       trace_loadvm_state_switchover_ack_needed(mis->switchover_ack_pending_num);
>>   }
>>   
>> -static int qemu_loadvm_state_setup(QEMUFile *f)
>> +static int qemu_loadvm_state_setup(QEMUFile *f, Error **errp)
>>   {
>> +    ERRP_GUARD(); /* error_prepend use */
> 
> The comment is correct, but prone to go stale.  No other use of
> ERRP_GUARD() is commented.  Suggest to drop it.

OK. I found it interesting, for me at least. Will drop in the VFIO
patches also.

Thanks,

C.


> 
>>       SaveStateEntry *se;
>>       int ret;
>>   
>> @@ -2763,10 +2764,11 @@ static int qemu_loadvm_state_setup(QEMUFile *f)
>>               }
>>           }
>>   
>> -        ret = se->ops->load_setup(f, se->opaque);
>> +        ret = se->ops->load_setup(f, se->opaque, errp);
>>           if (ret < 0) {
>> +            error_prepend(errp, "Load state of device %s failed: ",
>> +                          se->idstr);
>>               qemu_file_set_error(f, ret);
>> -            error_report("Load state of device %s failed", se->idstr);
>>               return ret;
>>           }
>>       }
>> @@ -2947,7 +2949,8 @@ int qemu_loadvm_state(QEMUFile *f)
>>           return ret;
>>       }
>>   
>> -    if (qemu_loadvm_state_setup(f) != 0) {
>> +    if (qemu_loadvm_state_setup(f, &local_err) != 0) {
>> +        error_report_err(local_err);
>>           return -EINVAL;
>>       }
>