Re: [PATCH v3 04/26] migration: Always report an error in ram_save_setup()

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

On 3/4/24 22:30, Fabiano Rosas wrote:
> Cédric Le Goater <clg@redhat.com> writes:
> 
>> This will prepare ground for futur changes adding an Error** argument
>> to the save_setup() handler. We need to make sure that on failure,
>> ram_save_setup() sets a new error.
>>
>> Signed-off-by: Cédric Le Goater <clg@redhat.com>
>> ---
>>   migration/ram.c | 11 ++++++++++-
>>   1 file changed, 10 insertions(+), 1 deletion(-)
>>
>> diff --git a/migration/ram.c b/migration/ram.c
>> index 45a00b45edd429ef0568adce09b7459883e00167..dbd04d8ee2167881007c836a6bbc79c1aced72d0 100644
>> --- a/migration/ram.c
>> +++ b/migration/ram.c
>> @@ -2937,12 +2937,14 @@ static int ram_save_setup(QEMUFile *f, void *opaque)
>>       int ret;
>>   
>>       if (compress_threads_save_setup()) {
>> +        error_report("%s: failed to start compress threads", __func__);
>>           return -1;
>>       }
>>   
>>       /* migration has already setup the bitmap, reuse it. */
>>       if (!migration_in_colo_state()) {
>>           if (ram_init_all(rsp) != 0) {
>> +            error_report("%s: failed to setup RAM for migration", __func__);
>>               compress_threads_save_cleanup();
>>               return -1;
>>           }
>> @@ -2969,12 +2971,14 @@ static int ram_save_setup(QEMUFile *f, void *opaque)
>>   
>>       ret = rdma_registration_start(f, RAM_CONTROL_SETUP);
>>       if (ret < 0) {
>> +        error_report("%s: failed to start RDMA registration", __func__);
>>           qemu_file_set_error(f, ret);
>>           return ret;
>>       }
>>   
>>       ret = rdma_registration_stop(f, RAM_CONTROL_SETUP);
>>       if (ret < 0) {
>> +        error_report("%s: failed to stop RDMA registration", __func__);
>>           qemu_file_set_error(f, ret);
>>           return ret;
>>       }
>> @@ -2986,6 +2990,7 @@ static int ram_save_setup(QEMUFile *f, void *opaque)
>>       ret = multifd_send_sync_main();
>>       bql_lock();
>>       if (ret < 0) {
>> +        error_report("%s: multifd synchronization failed", __func__);
>>           return ret;
>>       }
>>   
>> @@ -2994,7 +2999,11 @@ static int ram_save_setup(QEMUFile *f, void *opaque)
>>       }
>>   
>>       qemu_put_be64(f, RAM_SAVE_FLAG_EOS);
>> -    return qemu_fflush(f);
>> +    ret = qemu_fflush(f);
>> +    if (ret) {
>> +        error_report("%s failed : %s", __func__, strerror(ret));
> 
> Here it should be -ret
> 
> The qemu_fflush function returns the QEMUFile error (f->last_error) and
> that is expected to be a -errno value.

oh yes. I should have know that since qemu_file_set_error() takes a -errno.

  
> I started making sure all callers of qemu_file_set_error() use a
> negative value, but got lost in the vmstate code and never finished:
> 
> https://lore.kernel.org/r/20230706195201.18595-1-farosas@suse.de

Thanks for the pointer.

Thanks,

C.