Peter Xu <pet...@redhat.com> writes: > On Mon, May 13, 2024 at 04:17:02PM +0200, Markus Armbruster wrote: >> Functions that use an Error **errp parameter to return errors should >> not also report them to the user, because reporting is the caller's >> job. When the caller does, the error is reported twice. When it >> doesn't (because it recovered from the error), there is no error to >> report, i.e. the report is bogus. >> >> qmp_xen_save_devices_state() and qmp_xen_load_devices_state() violate >> this principle: they call qemu_save_device_state() and >> qemu_loadvm_state(), which call error_report_err(). >> >> I wish I could clean this up now, but migration's error reporting is >> too complicated (confused?) for me to mess with it. > > :-(
If I understood how it's *supposed* to work, I might have a chance... I can see a mixture of reporting errors directly (with error_report() & friends), passing them to callers (via Error **errp), and storing them in / retrieving them from MigrationState member @error. This can't be right. I think a necessary first step towards getting it right is a shared understanding how errors are to be handled in migration code. This includes how error data should flow from error source to error sink, and what the possible sinks are. >> Instead, I'm merely improving the error reported by >> qmp_xen_load_devices_state() and qmp_xen_load_devices_state() to the >> QMP core from >> >> An IO error has occurred >> >> to >> saving Xen device state failed >> >> and >> >> loading Xen device state failed >> >> respectively. >> >> Signed-off-by: Markus Armbruster <arm...@redhat.com> > > Acked-by: Peter Xu <pet...@redhat.com> Thanks!
