Re: [PATCH 13/14] migration: Use migrate_has_error() in close_return_path_on_source()

[Peter Xu]

On Thu, Feb 08, 2024 at 10:07:44AM -0300, Fabiano Rosas wrote:
> > diff --git a/migration/migration.c b/migration/migration.c
> > index 
> > d5f705ceef4c925589aa49335969672c0d761fa2..5f55af3d7624750ca416c4177781241b3e291e5d
> >  100644
> > --- a/migration/migration.c
> > +++ b/migration/migration.c
> > @@ -2372,8 +2372,7 @@ static bool 
> > close_return_path_on_source(MigrationState *ms)
> >       * cause it to unblock if it's stuck waiting for the destination.
> >       */
> >      WITH_QEMU_LOCK_GUARD(&ms->qemu_file_lock) {
> > -        if (ms->to_dst_file && ms->rp_state.from_dst_file &&
> > -            qemu_file_get_error(ms->to_dst_file)) {
> > +        if (migrate_has_error(ms) && ms->rp_state.from_dst_file) {
> >              qemu_file_shutdown(ms->rp_state.from_dst_file);
> >          }
> >      }
> 
> Hm, maybe Peter can help defend this, but this assumes that every
> function that takes an 'f' and sets the file error also sets
> migrate_set_error(). I'm not sure we have determined that, have we?

[apologies on getting back to this thread late.. I saw there's yet another
 proposal in the other email, will look at that soon]

I think that should be set, or otherwise we lose an error?  After all
s->error is the only thing we report, if there is a qemufile error that is
not reported into s->error it can be lost then.

On src QEMU we have both migration thread and return path thread.  For
migration thread the file error should always be collected by
migration_detect_error() by the qemu_file_get_error_obj_any() (it also
looks after postcopy_qemufile_src).  For return path thread it's always
collected when the loop quits.

Would migrate_has_error() be safer than qemu_file_get_error() in some
cases?  I'm considering when there is an error outside of qemufile itself,
that's the case where qemu_file_get_error(ms->to_dst_file) can return
false, however we may still need a kick to the from_dst_file?

-- 
Peter Xu