Message ID | 20240306133441.2351700-6-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 future 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> Reviewed-by: Fabiano Rosas <farosas@suse.de>
On Wed, Mar 06, 2024 at 02:34:20PM +0100, Cédric Le Goater wrote: > This will prepare ground for future 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> Since this whole set is mostly migration relevant, I plan to include this patch in next Monday's pull. S390 maintainers, please let me know if you have comments / objections before that, thanks! > --- > > Changes in v4: > > - Fixed state name printed out in error returned by vfio_save_setup() > - Fixed test on error returned by qemu_file_get_error() > > include/hw/s390x/storage-attributes.h | 2 +- > hw/s390x/s390-stattrib-kvm.c | 12 ++++++++++-- > hw/s390x/s390-stattrib.c | 15 ++++++++++----- > 3 files changed, 21 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..eeaa8110981c970e91a8948f027e398c34637321 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, "setting KVM_S390_VM_MIGRATION 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..b743e8a2fee84c7374460ccea6df1cf447cda44b 100644 > --- a/hw/s390x/s390-stattrib.c > +++ b/hw/s390x/s390-stattrib.c > @@ -60,11 +60,13 @@ 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)); > + error_free(local_err); > } > } > > @@ -170,13 +172,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 +264,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 +297,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 >
On 06/03/2024 14.34, Cédric Le Goater wrote: > This will prepare ground for future 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> > --- Reviewed-by: Thomas Huth <thuth@redhat.com>
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..eeaa8110981c970e91a8948f027e398c34637321 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, "setting KVM_S390_VM_MIGRATION 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..b743e8a2fee84c7374460ccea6df1cf447cda44b 100644 --- a/hw/s390x/s390-stattrib.c +++ b/hw/s390x/s390-stattrib.c @@ -60,11 +60,13 @@ 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)); + error_free(local_err); } } @@ -170,13 +172,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 +264,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 +297,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 future 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> --- Changes in v4: - Fixed state name printed out in error returned by vfio_save_setup() - Fixed test on error returned by qemu_file_get_error() include/hw/s390x/storage-attributes.h | 2 +- hw/s390x/s390-stattrib-kvm.c | 12 ++++++++++-- hw/s390x/s390-stattrib.c | 15 ++++++++++----- 3 files changed, 21 insertions(+), 8 deletions(-)