Re: [PATCH v2 7/7] migration: Provide explicit error message for file shutdowns

[Peter Xu <peterx@redhat.com>]

On Thu, Jul 06, 2023 at 10:50:34AM -0300, Fabiano Rosas wrote:
> Peter Xu <peterx@redhat.com> writes:
> 
> > On Wed, Jul 05, 2023 at 07:05:13PM -0300, Fabiano Rosas wrote:
> >> Peter Xu <peterx@redhat.com> writes:
> >> 
> >> > Provide an explicit reason for qemu_file_shutdown()s, which can be
> >> > displayed in query-migrate when used.
> >> >
> >> 
> >> Can we consider this to cover the TODO:
> >> 
> >>  * TODO: convert to propagate Error objects instead of squashing
> >>  * to a fixed errno value
> >> 
> >> or would that need something fancier?
> >
> > The TODO seems to say we want to allow qemu_file_shutdown() to report an
> > Error* when anything wrong happened (e.g. shutdown() failed)?  While this
> > patch was trying to store a specific error string so when query migration
> > later it'll show up to the user.  If so, IMHO they're two things.
> >
> 
> Ok, just making sure.
> 
> >> 
> >> > This will make e.g. migrate-pause to display explicit error descriptions,
> >> > from:
> >> >
> >> > "error-desc": "Channel error: Input/output error"
> >> >
> >> > To:
> >> >
> >> > "error-desc": "Channel is explicitly shutdown by the user"
> >> >
> >> > in query-migrate.
> >> >
> >> > Signed-off-by: Peter Xu <peterx@redhat.com>
> >> > ---
> >> >  migration/qemu-file.c | 5 ++++-
> >> >  1 file changed, 4 insertions(+), 1 deletion(-)
> >> >
> >> > diff --git a/migration/qemu-file.c b/migration/qemu-file.c
> >> > index 419b4092e7..ff605027de 100644
> >> > --- a/migration/qemu-file.c
> >> > +++ b/migration/qemu-file.c
> >> > @@ -87,7 +87,10 @@ int qemu_file_shutdown(QEMUFile *f)
> >> >       *      --> guest crash!
> >> >       */
> >> >      if (!f->last_error) {
> >> > -        qemu_file_set_error(f, -EIO);
> >> > +        Error *err = NULL;
> >> > +
> >> > +        error_setg(&err, "Channel is explicitly shutdown by the user");
> >> 
> >> It is good that we can grep this message. However, I'm confused about
> >> who the "user" is meant to be here and how are they implicated in this
> >> error.
> >
> > Ah, here the user is who sends the "migrate-pause" command, according to
> > the example of the commit message.
> >
> 
> That's where I'm confused. There are 15 callsites for
> qemu_file_shutdown(). Only 2 of them are from migrate-pause. So I'm
> missing the logical step that links migrate-pause with this
> error_setg().
> Are you assuming that the race described will only happen
> with migrate-pause and the other invocations would have set an error
> already?

It's not a race, but I think you're right. I thought it was always the case
to shut but actually not: we do shutdown() also in a few places where we
don't really fail, either for COLO or for completion of migration.  With
the 1st patch, it'll even show in query-migrate.  Thanks for spotting it -
I could have done better.

Let's drop this patch.. sorry for the noise.

-- 
Peter Xu