Re: [PATCH 5/6] migration: Rephrase message on failure to save / load Xen device state

[Markus Armbruster]

Peter Xu <peterx@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 <armbru@redhat.com>
>
> Acked-by: Peter Xu <peterx@redhat.com>

Thanks!