Re: [PATCH v3 01/26] s390/stattrib: Add Error** argument to set_migrationmode() handler

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

On 3/4/24 21:49, 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,
>> set_migrationmode() always sets a new error. See the Rules section in
>> qapi/error.h.
>>
>> Cc: Halil Pasic <pasic@linux.ibm.com>
>> Cc: Christian Borntraeger <borntraeger@linux.ibm.com>
>> Cc: Thomas Huth <thuth@redhat.com>
>> Signed-off-by: Cédric Le Goater <clg@redhat.com>
>> ---
>>   include/hw/s390x/storage-attributes.h |  2 +-
>>   hw/s390x/s390-stattrib-kvm.c          | 12 ++++++++++--
>>   hw/s390x/s390-stattrib.c              | 14 +++++++++-----
>>   3 files changed, 20 insertions(+), 8 deletions(-)
>>
>> diff --git a/include/hw/s390x/storage-attributes.h b/include/hw/s390x/storage-attributes.h
>> index 5239eb538c1b087797867a247abfc14551af6a4d..8921a04d514bf64a3113255ee10ed33fc598ae06 100644
>> --- a/include/hw/s390x/storage-attributes.h
>> +++ b/include/hw/s390x/storage-attributes.h
>> @@ -39,7 +39,7 @@ struct S390StAttribClass {
>>       int (*set_stattr)(S390StAttribState *sa, uint64_t start_gfn,
>>                         uint32_t count, uint8_t *values);
>>       void (*synchronize)(S390StAttribState *sa);
>> -    int (*set_migrationmode)(S390StAttribState *sa, bool value);
>> +    int (*set_migrationmode)(S390StAttribState *sa, bool value, Error **errp);
>>       int (*get_active)(S390StAttribState *sa);
>>       long long (*get_dirtycount)(S390StAttribState *sa);
>>   };
>> diff --git a/hw/s390x/s390-stattrib-kvm.c b/hw/s390x/s390-stattrib-kvm.c
>> index 24cd01382e2d74d62c2d7e980eb6aca1077d893d..357cea2c987213b867c81b0e258f7d0c293fe665 100644
>> --- a/hw/s390x/s390-stattrib-kvm.c
>> +++ b/hw/s390x/s390-stattrib-kvm.c
>> @@ -17,6 +17,7 @@
>>   #include "sysemu/kvm.h"
>>   #include "exec/ram_addr.h"
>>   #include "kvm/kvm_s390x.h"
>> +#include "qapi/error.h"
>>   
>>   Object *kvm_s390_stattrib_create(void)
>>   {
>> @@ -137,14 +138,21 @@ static void kvm_s390_stattrib_synchronize(S390StAttribState *sa)
>>       }
>>   }
>>   
>> -static int kvm_s390_stattrib_set_migrationmode(S390StAttribState *sa, bool val)
>> +static int kvm_s390_stattrib_set_migrationmode(S390StAttribState *sa, bool val,
>> +                                               Error **errp)
>>   {
>>       struct kvm_device_attr attr = {
>>           .group = KVM_S390_VM_MIGRATION,
>>           .attr = val,
>>           .addr = 0,
>>       };
>> -    return kvm_vm_ioctl(kvm_state, KVM_SET_DEVICE_ATTR, &attr);
>> +    int r;
>> +
>> +    r = kvm_vm_ioctl(kvm_state, KVM_SET_DEVICE_ATTR, &attr);
>> +    if (r) {
>> +        error_setg_errno(errp, -r, "KVM_S390_SET_CMMA_BITS failed");
> 
> Did you mean KVM_SET_DEVICE_ATTR?

Drat. Copy paste :)


Thanks,

C.



> 
>> +    }
>> +    return r;
>>   }
>>   
>>   static long long kvm_s390_stattrib_get_dirtycount(S390StAttribState *sa)
>> diff --git a/hw/s390x/s390-stattrib.c b/hw/s390x/s390-stattrib.c
>> index c483b62a9b5f71772639fc180bdad15ecb6711cb..e99de190332a82363b1388bbc450013149295bc0 100644
>> --- a/hw/s390x/s390-stattrib.c
>> +++ b/hw/s390x/s390-stattrib.c
>> @@ -60,11 +60,12 @@ void hmp_migrationmode(Monitor *mon, const QDict *qdict)
>>       S390StAttribState *sas = s390_get_stattrib_device();
>>       S390StAttribClass *sac = S390_STATTRIB_GET_CLASS(sas);
>>       uint64_t what = qdict_get_int(qdict, "mode");
>> +    Error *local_err = NULL;
>>       int r;
>>   
>> -    r = sac->set_migrationmode(sas, what);
>> +    r = sac->set_migrationmode(sas, what, &local_err);
>>       if (r < 0) {
>> -        monitor_printf(mon, "Error: %s", strerror(-r));
>> +        monitor_printf(mon, "Error: %s", error_get_pretty(local_err));
>>       }
>>   }
>>   
>> @@ -170,13 +171,15 @@ static int cmma_save_setup(QEMUFile *f, void *opaque)
>>   {
>>       S390StAttribState *sas = S390_STATTRIB(opaque);
>>       S390StAttribClass *sac = S390_STATTRIB_GET_CLASS(sas);
>> +    Error *local_err = NULL;
>>       int res;
>>       /*
>>        * Signal that we want to start a migration, thus needing PGSTE dirty
>>        * tracking.
>>        */
>> -    res = sac->set_migrationmode(sas, 1);
>> +    res = sac->set_migrationmode(sas, true, &local_err);
>>       if (res) {
>> +        error_report_err(local_err);
>>           return res;
>>       }
>>       qemu_put_be64(f, STATTR_FLAG_EOS);
>> @@ -260,7 +263,7 @@ static void cmma_save_cleanup(void *opaque)
>>   {
>>       S390StAttribState *sas = S390_STATTRIB(opaque);
>>       S390StAttribClass *sac = S390_STATTRIB_GET_CLASS(sas);
>> -    sac->set_migrationmode(sas, 0);
>> +    sac->set_migrationmode(sas, false, NULL);
>>   }
>>   
>>   static bool cmma_active(void *opaque)
>> @@ -293,7 +296,8 @@ static long long qemu_s390_get_dirtycount_stub(S390StAttribState *sa)
>>   {
>>       return 0;
>>   }
>> -static int qemu_s390_set_migrationmode_stub(S390StAttribState *sa, bool value)
>> +static int qemu_s390_set_migrationmode_stub(S390StAttribState *sa, bool value,
>> +                                            Error **errp)
>>   {
>>       return 0;
>>   }
>