| Message ID | 20240304122844.1888308-2-clg@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | migration: Improve error reporting | expand |
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? > + } > + 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; > }
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; >> } >
On 04/03/2024 14:28, Cédric Le Goater wrote: > External email: Use caution opening links or attachments > > > 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"); > + } > + 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)); I think we need to free the error here: error_free(local_err); Thanks. > } > } > > @@ -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; > } > -- > 2.44.0 >
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"); + } + 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; }
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(-)